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

cxx: fix build with CARGO_TARGET_DIR set #2542

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

fw-immunant
Copy link
Collaborator

Fixes #2335. This build.rs file is already ours rather than matching the upstream one, so I think further changes are fine--stop me if not!

@mgeisler
Copy link
Collaborator

Fixes #2335. This build.rs file is already ours rather than matching the upstream one, so I think further changes are fine--stop me if not!

Alright, I had honestly no idea what we had customized the build file 😄

Thanks a lot for improving it, I only think we should add a comment explaining what this does.

@@ -1,9 +1,16 @@
fn main() {
let src_dir = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let src_dir = {
// Handle $CARGO_TARGET_DIR being set.
let src_dir = {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this, you could consider writing it more directly as

// Handle $CARGO_TARGET_DIR being set.
let mut src_dir = std::env::var_os("CARGO_TARGET_DIR").unwrap_or("../../../target".into());
src_dir.push("/cxxbridge/demo/src");

Also, do you know how this relates to the $OUT_DIR variable which Cargo sets?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

As I understand it, $OUT_DIR refers to a temporary directory, and is an output of Cargo; $CARGO_TARGET_DIR is an input to Cargo and might change what $OUT_DIR ends up being (but I'm not sure, I don't touch build scripts often... unless they break).

@fw-immunant fw-immunant merged commit 43e1cd6 into google:main Feb 3, 2025
35 checks passed
@fw-immunant fw-immunant deleted the fw/cargo-target-dir branch February 3, 2025 17:23
michael-kerscher pushed a commit to michael-kerscher/comprehensive-rust that referenced this pull request Feb 7, 2025
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.

cargo check fails in cxx demo if CARGO_TARGET_DIR is set
2 participants