Conversation
coffinmatician
force-pushed
the
main
branch
2 times, most recently
from
ecc9b95 to
5bc851e
Compare
Could you mention this rationale (build systems other than Cargo) in the news fragment? Thanks! |
|
I'm also a bit confused. If this PR is meant to handle the case when 'OUT_DIR' is not set, the PR title and changelog need to be revised. I don't have any experience using another build system than cargo to compile Rust crate, but do you mean that some of them don't set OUT_DIR? |
|
I'd say this is basically about replacing |
|
Ah, I see, thank you. |
| path.push("pyo3-build-config.txt"); | ||
| path | ||
| env::var_os("TARGET").and_then(|target| { | ||
| std::env::var_os("OUT_DIR").map(|out_dir| { |
There was a problem hiding this comment.
Could you add a link to the documentation of e.g. bazel or buck2 that says how they set OUT_DIR? otherwise it will be hard to determine in the future why it's written in this way and how you may or may not change it
There was a problem hiding this comment.
Uhh, I don't think it is documented anywhere explicitly. That is just a design decision of some implementations such as cargo-raze. (I'm guessing they don't document it as that sort of design decision is everywhere in Bazel and so would be obvious to someone who has used it for a while (all actions run in different environments and you can't rely on the old OUT_DIR being the new OUT_DIR)).
There was a problem hiding this comment.
I also think this strictly an improvement as we never needed compile-time access to OUT_DIR in the first place, i.e. we could have written it this way from the start. But let's wait what @konstin thinks w.r.t. your answer before merging.
coffinmatician
force-pushed
the
main
branch
2 times, most recently
from
e4b3afe to
256d259
Compare
It is as adamreichold says. |
| path.push("pyo3-build-config.txt"); | ||
| path | ||
| env::var_os("TARGET").and_then(|target| { | ||
| std::env::var_os("OUT_DIR").map(|out_dir| { |
There was a problem hiding this comment.
I also think this strictly an improvement as we never needed compile-time access to OUT_DIR in the first place, i.e. we could have written it this way from the start. But let's wait what @konstin thinks w.r.t. your answer before merging.
|
@coffinmatician I know I am to blame because I asked for the removal of |
This helps building on systems where they may be different (bazel, buck2, etc.)
|
Hmm, I think we may want to wait on this patch a bit longer. A coworker is suspicious that I have broken things subtly for other targets. I probably should have tested it more in traditional cargo to make sure it was working as I expected, but I assumed that the CI here would test some cross compilation. |
I want to say that we do, e.g. https://github.com/PyO3/pyo3/actions/runs/4692932437/jobs/8319284869, but it seems we are missing the |
Sorry I didn't get to commenting on this thread sooner. If I recall correctly, I made the choice to bake-in the I think #1758 is the right PR with the original context. At the time we needed to access the config from our proc macros. In #2243 that access was removed. It's very possible there's a solution I didn't think of, or something enabled by newer cargo functionality. |
5 participants
This helps building on systems where they may be different (bazel, buck2, etc.)