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 PYO3_CONFIG_FILE env var not requesting rebuilds #4758

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

davidhewitt
Copy link
Member

Candidate fix for #4757

I will verify this downstream and also add tests as soon as I'm satisfied this is the likely solution.

@davidhewitt
Copy link
Member Author

Both pydantic/jiter#180 and pydantic/pydantic-core@7b01901 which test out #4758 are working as they should with this fix in place. So I am satisfied that this is the fix and we should get this merged asap.

@davidhewitt davidhewitt marked this pull request as ready for review December 3, 2024 16:16
@davidhewitt
Copy link
Member Author

I pushed a crude test which should make it harder to make this mistake again. It would be nice to follow up with a more comprehensive way to test all cases of pyo3-build-config reading these env vars.

For now, is anyone available to review this? Otherwise I will merge in a few hours and start preparing the patch release.

@alexdotgov2
Copy link

(Hi, this is Alex's work github)

The fix looks prima facie reasonable to me, but I sadly don't have the time to deeply review.

I'm not a huge fan of the cfg() for tracking the env vars, but I'm not sure I have a better suggestion.

@davidhewitt
Copy link
Member Author

Thanks, I will proceed to merge and get prepping a release.

I agree the cfg stuff is pretty nasty. I think what might be helpful is to refactor this stuff to have a ctx: &BuildScriptContext external argument which can then record all environment variable accesses (and then emit them on build scripts). But that would break a lot of pyo3-build-config APIs so would be a cleanup deferred until 0.24.

@davidhewitt davidhewitt enabled auto-merge December 3, 2024 16:39
@alexdotgov2
Copy link

We should also use clippy to prohibit direct env var access and force people to go through the right internal functions -- sfackler/rust-openssl@e5bd08f is an example of this pattern.

@davidhewitt davidhewitt added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024
@davidhewitt
Copy link
Member Author

Agreed, while it wouldn't have caught this particular error, in combination with a better testing strategy it might prevent future mistakes. Opened #4761 to track the follow-ups.

@davidhewitt davidhewitt added this pull request to the merge queue Dec 3, 2024
Merged via the queue into PyO3:main with commit 48436da Dec 3, 2024
44 checks passed
@davidhewitt davidhewitt deleted the config-file-env-var branch December 3, 2024 21:20
davidhewitt added a commit that referenced this pull request Dec 3, 2024
* fix `PYO3_CONFIG_FILE` env var not requesting rebuilds

* test and newsfragment

* fix clippy

* relax assertion
@davidhewitt davidhewitt mentioned this pull request Dec 3, 2024
davidhewitt added a commit that referenced this pull request Dec 3, 2024
* fix `PYO3_CONFIG_FILE` env var not requesting rebuilds

* test and newsfragment

* fix clippy

* relax assertion
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.

2 participants