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

pyo3-build-config: conditionalize symbols on resolve-config feature #1859

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

indygreg
Copy link
Contributor

@indygreg indygreg commented Sep 3, 2021

PR #1856 was buggy in that the pyo3-build-config crate didn't actually
work in library mode because include_str!() was attempting to resolve
missing files as part of populating some const values.

We could change the logic of these constants to make them lazy if
we wanted to support possibly getting access to the value. But the
simple solution is to conditionalize their presence on the crate
feature.

Test coverage for building and testing the crate in insolation with the
feature disabled has been added.

Various code has been conditionalized to avoid compiler warnings.

Also, it appears cargo build|test -p pyo3-build-config --no-default-features still passes default features. This seems wrong
to me. But it is how my system behaves. Maybe it is an sccache bug?
I coded the new tests to cd pyo3-build-config first to work around.

PR PyO3#1856 was buggy in that the `pyo3-build-config` crate didn't actually
work in library mode because `include_str!()` was attempting to resolve
missing files as part of populating some `const` values.

We could change the logic of these constants to make them lazy if
we wanted to support possibly getting access to the value. But the
simple solution is to conditionalize their presence on the crate
feature.

Test coverage for building and testing the crate in insolation with the
feature disabled has been added.

Various code has been conditionalized to avoid compiler warnings.

Also, it appears `cargo build|test -p pyo3-build-config
--no-default-features` still passes default features. This seems wrong
to me. But it is how my system behaves. Maybe it is an sccache bug?
I coded the new tests to `cd pyo3-build-config` first to work around.
@davidhewitt
Copy link
Member

Thanks, I'm glad we tested this before releasing! The added CI is appreciated :)

Also, it appears cargo build|test -p pyo3-build-config --no-default-features still passes default features. This seems wrong
to me. But it is how my system behaves. Maybe it is an sccache bug?
I coded the new tests to cd pyo3-build-config first to work around.

This is probably due to cargo feature resolver "1" merging dependencies across workspace, meaning pyo3's dependency on this feature probably comes into play. Might be fixed with the next resolver version? It's going to be a long time before we can use that in our CI unfortunately. (It was added in Rust 1.51, and our next MSRV is scheduled to be Rust 1.48, so I expect we won't be there until a couple of years.)

@davidhewitt davidhewitt merged commit 6e83516 into PyO3:main Sep 3, 2021
@davidhewitt davidhewitt mentioned this pull request Sep 3, 2021
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