Skip to content

Conversation

@hameerabbasi
Copy link
Contributor

@hameerabbasi hameerabbasi commented Jan 6, 2026

Description

Fixes #1259

How Has This Been Tested?

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added sufficient tests to cover my changes.
  • I have verified that changes that would impact the JSON schema have been made in schema/model.py.

@hameerabbasi hameerabbasi changed the title Support PEP508 environment markers feat: Support PEP508 environment markers Jan 6, 2026
@hameerabbasi hameerabbasi force-pushed the support-pep508-env-markers branch 4 times, most recently from 734fa83 to 0abb254 Compare January 6, 2026 15:41
@hameerabbasi hameerabbasi marked this pull request as ready for review January 6, 2026 16:09
Copy link
Contributor

@tdejager tdejager left a comment

Choose a reason for hiding this comment

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

Good work seems easier then expected! Can you test if satisfiability works as expected, i.e changing markers re-solves and such? Also test if adding a linux marker with windows as a platform for example creates an empty dependency set. Maybe there are more edge cases we could test and report on?

@tdejager
Copy link
Contributor

tdejager commented Jan 7, 2026

Maybe we should also have some documentation updates, not sure, could you have a look?

@hameerabbasi
Copy link
Contributor Author

hameerabbasi commented Jan 7, 2026

Also test if adding a linux marker with windows as a platform for example creates an empty dependency set.

I changed the test to do this in 2a24e4b, but I have no idea how to fix it sadly. Soo many types and conversions my grepping abilities are failing me.

Edit: Added another test in 1e824d2.

@hameerabbasi hameerabbasi force-pushed the support-pep508-env-markers branch 2 times, most recently from 1737ae2 to 2bb2372 Compare January 7, 2026 09:00
@hameerabbasi hameerabbasi force-pushed the support-pep508-env-markers branch from f3004e7 to d2e632f Compare January 7, 2026 12:38
@hameerabbasi
Copy link
Contributor Author

Can you test if satisfiability works as expected, i.e changing markers re-solves and such?

Done in 06329e3

@hameerabbasi
Copy link
Contributor Author

hameerabbasi commented Jan 8, 2026

@tdejager @baszalmstra I think this is ready, review/merge appreciated.

@tdejager
Copy link
Contributor

tdejager commented Jan 9, 2026

I looked through everything and all of it makes sense! Let me do a small user-test myself.

@hameerabbasi hameerabbasi force-pushed the support-pep508-env-markers branch from 1dab9dd to 62e8d57 Compare January 12, 2026 15:20
@tdejager
Copy link
Contributor

User-tested a bunch of cases. Can't find any issues. I think @nichmor wanted a final go, if he agrees, lets merge.

@hameerabbasi hameerabbasi force-pushed the support-pep508-env-markers branch 2 times, most recently from eb37c33 to 9be9053 Compare January 13, 2026 14:04
@nichmor
Copy link
Contributor

nichmor commented Jan 13, 2026

tested it with multiple env markers/ combination of platforms/ install --locked vs install vs lock.
it works! thanks for the PR!

@nichmor nichmor enabled auto-merge (squash) January 13, 2026 14:15
auto-merge was automatically disabled January 13, 2026 14:20

Head branch was pushed to by a user without write access

@hameerabbasi hameerabbasi force-pushed the support-pep508-env-markers branch from 98bd9fd to 9d193c2 Compare January 13, 2026 14:20
@nichmor
Copy link
Contributor

nichmor commented Jan 13, 2026

hey @hameerabbasi - merging my branch into main made a small conflict with yours - I will fix it soon :)

@hameerabbasi
Copy link
Contributor Author

@nichmor Done already. 🤣

@nichmor nichmor enabled auto-merge (squash) January 13, 2026 14:24
@nichmor nichmor merged commit 2afa7cd into prefix-dev:main Jan 13, 2026
41 checks passed
@hameerabbasi hameerabbasi deleted the support-pep508-env-markers branch January 13, 2026 14:35
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.

Add Environment Markers to pypi-dependencies specification

4 participants