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

poetry-plugin-up: init at 0.2.1 #211231

Closed
wants to merge 1 commit into from
Closed

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jan 17, 2023

Description of changes

A plugin to update dependencies to latest even if pyproject.toml says not to.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Use pytestCheckHook to run the tests.

@@ -16383,6 +16383,8 @@ with pkgs;

poetry = callPackage ../tools/package-management/poetry { };

poetry-plugin-up = callPackage ../tools/package-management/poetry-plugin-up { };
Copy link
Member

Choose a reason for hiding this comment

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

This should be in python-packages.nix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? It's not going to be useful with any Python version but the one Poetry is built with.

Copy link
Member

Choose a reason for hiding this comment

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

It could also live at poetry.plugins or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that but I'm not sure if it's worth adding another scope when we only have one package to put under it.

Copy link
Member

Choose a reason for hiding this comment

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

It is. We can ensure that plugins use the same Python package set in two ways: the plugins argument becomes a function that is passed python.pkgs (see home-assistant) and plugins are packaged in python3Packages, or plugins live in poetry.plugins where they're automatically built with the correct Python package set.

@K900
Copy link
Contributor Author

K900 commented Jan 17, 2023

Use pytestCheckHook to run the tests.

Tests won't run because we don't have a Poetry.

@K900
Copy link
Contributor Author

K900 commented Jan 17, 2023

Applied all the style fixes.

@mweinelt
Copy link
Member

Tests won't run because we don't have a Poetry.

Add poetry to checkInputs, disable doCheck, import the package itself, create an override in passthru.tests á la poetry-plugin-up.overridePythonAttrs (_: { doCheck = true; })

@K900
Copy link
Contributor Author

K900 commented Jan 17, 2023

That's still not going to work because Poetry doesn't propagate its dependencies (see #209534).

Comment on lines +53 to +65
] ++ lib.optionals (pythonOlder "3.8") [
backports-cached-property
Copy link
Member

Choose a reason for hiding this comment

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

We can remove that, we no longer support 3.8

@K900
Copy link
Contributor Author

K900 commented Jan 22, 2023

Rebased to catch the nativeCheckInputs change.

@K900 K900 force-pushed the poetry-plugin-up branch 3 times, most recently from 9db73c1 to 8dd486f Compare January 24, 2023 11:34
@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 24, 2023
Introduce a wrapper for Poetry instead of manually unpropagating things,
which also allows using unwrapped Poetry as an input when necessary.

Also add poetry-plugin-up as an example.
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 26, 2023
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

I think this is more complicated than it needs to be. I don't see why one type of wrapping shouldn't suffice and I'm not fond of the $PYTHONPATH hackery.
Thus I tried to achieve the same thing just using overridePythonAttrs and thought that it's maybe a good idea to put the actual expression for poetry in a separate file unwrapped.nix. See #213914 for my suggestion.

passthru = {
inherit unwrapped python;
withPlugins = selector: let
pluginsSet = python.pkgs.callPackage ./plugins {
Copy link
Member

Choose a reason for hiding this comment

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

You can use import

@K900
Copy link
Contributor Author

K900 commented Feb 1, 2023

The reason I went with a double wrapper here is mostly to avoid rebuilding poetry itself, which is not particularly slow but also kinda slow.

@dotlambda
Copy link
Member

The reason I went with a double wrapper here is mostly to avoid rebuilding poetry itself, which is not particularly slow but also kinda slow.

That does make sense indeed. I guess I could copy the double wrapper in my approach if that's preferred.
I still think having poetry.plugins is a bit cleaner and makes sure all plugins use the same versions of all Python packages.

@K900
Copy link
Contributor Author

K900 commented Feb 1, 2023

Cleaner than what? Sorry, might be missing something...

@dotlambda
Copy link
Member

Cleaner than what? Sorry, might be missing something...

You're right, your approach is almost the same in this regard. So never mind.


wrapWithPlugins = ''
buildPythonPath $plugins
makeWrapper ${unwrapped}/bin/poetry $out/bin/poetry --suffix PYTHONPATH : "$program_PYTHONPATH"
Copy link
Member

Choose a reason for hiding this comment

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

This is basically a no-op if $program_PYTHONPATH is empty. Is there a specific reason you're using a different wrapper in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a tiny efficiency thing, probably not worth worrying about realistically.

@mweinelt
Copy link
Member

mweinelt commented Feb 2, 2023

#213914 looks simpler and is the more python-native approach using packageOverrides for the package set and overridePythonAttrs to propagate plugins into the poetry package.

I think I prefer it also because it relies less on a bespoke wrapping logic, like is the case here.

The downside of the override approach is that you get to rebuild poetry, which takes a minute on my desktop due to five very slow tests. We could however also just disable tests on the overridden derivation to get that penalty out of the way.

(formed this opinion while discussing the situation with K900 in a private chat)

@K900
Copy link
Contributor Author

K900 commented Feb 2, 2023

I definitely like the packageOverrides approach and probably would have done the same thing if I remembered it existed at the time (lol). I'm still not 100% convinced on the wrapper versus adding inputs part, but it's also definitely not a blocker and either way should be fine.

@dotlambda
Copy link
Member

We could however also just disable tests on the overridden derivation to get that penalty out of the way.

That's a good compromise.

I'm still not 100% convinced on the wrapper versus adding inputs part

The advantage of adding inputs is that it's going to be correct in the future even if wrapPython's logic changes. We also won't have to change the wrapper if files are addded to poetry's $out/share or whatever.

@K900
Copy link
Contributor Author

K900 commented Feb 2, 2023

Then I'm just going to close this in favor of #213914.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants