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

fix: dependencies package PEP508 markers selection #1905

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamesdbrock
Copy link
Contributor

@jamesdbrock jamesdbrock commented Mar 11, 2025

Bugfix: support local .whl files with dependency markers.

Maybe resolves #1174

2 commits:

  1. Add 2 tests. Both tests are disabled for the case of x86_64-darwin.

    • Remote .whl dependency marker selection mk-poetry-packages-partition-whl-remote On this commit the test succeeds because poetry2nix can select dependency markers for remote .whl files.

    • Local .whl dependency marker selection mk-poetry-packages-partition-whl-local On this commit the test fails because poetry2nix cannot select dependency markers for local .whl files.

      Add three local watchdog.whl files (79kB, 79kB, 96kB) to the repository. We chose watchdog because it distributes separate .whl files for each platform and the files are small.

  2. fix: dependencies package PEP508 markers selection.
    With this commit the second test mk-poetry-packages-partition-whl-local succeeds because now poetry2nix can select dependency markers for local .whl files.

Contribution checklist (recommended but not always applicable/required):

  • There's an automated test for this change
  • Commit messages or code include references to related issues or PRs (including third parties)
  • Commit messages are conventional - examples from the log include "feat: add changelog files to fixup hook", "fix(contourpy): allow wheel usage", and "test: add sqlalchemy2 test"

@jamesdbrock jamesdbrock marked this pull request as draft March 11, 2025 03:38
@jamesdbrock jamesdbrock force-pushed the jbrock/markers branch 5 times, most recently from 78dba0f to 934d90c Compare March 11, 2025 23:41
Test succeeds because poetry2nix can select dependency markers for remote .whl
files:

nix-build --attr mk-poetry-packages-partition-whl-remote --keep-going --show-trace tests/default.nix

Test fails because poetry2nix cannot select dependency markers for local .whl
files:

nix-build --attr mk-poetry-packages-partition-whl-local --keep-going --show-trace tests/default.nix
This bug was causing all cases to fall through to the default selection
by checkPythonVersions only.

With this commit the local .whl test succeeds because now poetry2nix can select
dependency markers for local .whl files.

Resolves nix-community#1174
@jamesdbrock jamesdbrock marked this pull request as ready for review March 12, 2025 10:15
@jamesdbrock
Copy link
Contributor Author

Who is merging PRs on this repo these days, is it @Nebucatnetzer ? Thank you very much for your maintenance work.

This PR might be controversial because it adds 3 .whl binary files to the repo. I'm willing to negotiate about that.

@Nebucatnetzer
Copy link
Contributor

I can merge stuff but that mostly involves the pull requests for override updates and similar minor stuff.

I honestly don't know the pros and cons of including the wheels here.
Maybe they could be included with something like fetchurl?

When you write Maybe resolves #1174 what makes you unsure about?
I guess you tested it?

@Nebucatnetzer
Copy link
Contributor

Maybe @elikoga has some inputs?

@elikoga
Copy link
Contributor

elikoga commented Mar 12, 2025

I was initially a bit confused because the diff changes a read for .marker to .markers but it seems like the poetry lock format uses markers, as shown in the patch.

I'd look into why we accessed .marker before or wether that was just buggy code.

I'm not sure about adding the wheels to the repo. They are small (all less than 100kb as far as I see) so it's not that bad.

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

Successfully merging this pull request may close these issues.

Poetry2nix still resolves all dependencies with groups/markers
3 participants