Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python312Packages.pygerber: init at 2.4.2, pytest-lsp: init at 0.4.3 #359437

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

clemjvdm
Copy link
Contributor

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: python 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Nov 26, 2024
@nix-owners nix-owners bot requested a review from natsukium November 26, 2024 22:25
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 26, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Nov 29, 2024
@clemjvdm
Copy link
Contributor Author

do I need to do anything else? Sorry it's my first PR so I'm not sure. @steeleduncan

Copy link
Contributor

@steeleduncan steeleduncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar enough with python derivations to leave an approving review, but I certainly wouldn't want to block it

If you can find someone who has merged some python commits recently and ping them for a review that is probably the best approach to get it merged

@clemjvdm
Copy link
Contributor Author

I am not familiar enough with python derivations to leave an approving review, but I certainly wouldn't want to block it

If you can find someone who has merged some python commits recently and ping them for a review that is probably the best approach to get it merged

@natsukium Hello, hope it's ok for me to ping you here. I opened this PR about 3 weeks ago and I wasn't sure if I had to do anything else to get it merged.

@clemjvdm clemjvdm closed this Dec 20, 2024
@clemjvdm clemjvdm reopened this Dec 20, 2024
@FliegendeWurst FliegendeWurst changed the title pythonPackages.pygerber: init at 2.4.2 python312Packages.pygerber: init at 2.4.2 Jan 1, 2025
@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst Just committed your suggestions, fixed a little error and squashed them, hopefully this is how I was supposed to do this. Anything else I should do?

@FliegendeWurst
Copy link
Member

Yes. So far you only activated the import check. It would be good to run the real test suite, if the project has one. Add pytestCheckHook (or another hook, depending on the framework) to nativeCheckInputs for that.

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst I looked into it, but I can't test because pytest-lsp doesn't yet exist on nixpkgs :(

@FliegendeWurst
Copy link
Member

FliegendeWurst commented Jan 4, 2025

Here you go. Untested.

{
  lib,
  fetchPypi,
  buildPythonPackage,
  hatchling,
  pygls,
  pytestCheckHook,
  pytest-asyncio,
}:

buildPythonPackage rec {
  pname = "pytest-lsp";
  version = "0.4.3";
  pyproject = true;

  src = fetchPypi {
    inherit version;
    pname = "pytest_lsp";
    hash = "sha256-ND9r2i+qMg7V/Ld8lCDScDzlZdHRRP6CfjGYp9wpkRw=";
  };

  build-system = [
    hatchling
  ];

  dependencies = [
    pygls
    pytest-asyncio
  ];

  nativeCheckInputs = [
    pytestCheckHook
  ];

  pythonImportsCheck = [ "pytest_lsp" ];

  meta = {
    homepage = "https://github.com/swyddfa/lsp-devtools";
    changelog = "https://github.com/swyddfa/lsp-devtools/blob/develop/lib/pytest-lsp/CHANGES.md";
    description = "Pytest plugin for writing end-to-end tests for language servers";
    license = lib.licenses.mit;
    maintainers = with lib.maintainers; [ clemjvdm fliegendewurst ];
  };
}

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst Thanks a lot! But I'm sorry, I'm not sure how I should go about this. Open a new PR with pytest_lsp, or just write this somewhere in this PR?

@FliegendeWurst
Copy link
Member

You can put it in this PR. Ideally it would be done in a commit before the pygerber package is added.

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst Just to be sure before I do this, pygerber documentation states that to run tests one should do:

poetry shell
poe run-unit-tests

I assumed this wasn't the right way to do this here since this would imply letting poetry manage dependencies. I also tried just installing the required dependencies and doing poe run-unit-tests but this requires poetry and nix will only let me use poetry-core:

error: poetry was promoted to a top-level attribute, use poetry-core to build Python packages

Which is why I figured maybe using pytests (which poepoet refers to) directly was the right approach. But in hindsight I'm not sure.

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

I think either way we'll need pytest-lsp.

@FliegendeWurst
Copy link
Member

FliegendeWurst commented Jan 4, 2025

I think pytestCheckHook will work. You may need to add some more plugins though:

https://github.com/Argmaster/pygerber/blob/c4208a98293d2abf1101804f6166df79742b0de5/pyproject.toml#L84-L89

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst Yeah I figured, I'll try that then. Thanks :)

@clemjvdm clemjvdm force-pushed the master branch 2 times, most recently from b4182a9 to e357721 Compare January 4, 2025 18:12
@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst New issue! Yay! I was able to get everything working with pytest-lsp (thanks again). But it seems pygerber's tests pull assets from github (which isn't allowed by the nix build phase).

Is there a good way of solving this? I presume maybe we can fetch them in nix and then patch the tests but I don't have time nor am I qualified to do that I think.

@FliegendeWurst
Copy link
Member

If it's only some tests you can disable them using disabledTests = [ .... ].

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst Ok that worked! Do we care about skipped, xfailed and xpassed tests?

============= 427 passed, 2 skipped, 8 xfailed, 1 xpassed in 7.08s =============

Otherwise I'll commit.

@FliegendeWurst
Copy link
Member

That looks good to me 👍

@clemjvdm
Copy link
Contributor Author

clemjvdm commented Jan 4, 2025

@FliegendeWurst All done. Thanks you sm for the help :)

@FliegendeWurst FliegendeWurst changed the title python312Packages.pygerber: init at 2.4.2 python312Packages.pygerber: init at 2.4.2, pytest-lsp: init at 0.4.3 Jan 9, 2025
@FliegendeWurst FliegendeWurst merged commit 2e1715c into NixOS:master Jan 9, 2025
32 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants