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: support relative TARGET_DIR in near-test-contracts #6181

Merged
merged 3 commits into from
Feb 1, 2022

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Jan 25, 2022

near-test-contracts builds some wasm contracts for use in testing. It
does so by recursively invoking cargo from build.rs. Before this
commit, we tried to re-use parent's CARGO_TARGET_DIR to figure out
where we should put the data. That was rather hacky, as cargo doesn't
expose that information to the build scripts in a reliable way:

rust-lang/cargo#9661 (comment)

Naturally, our hacks broken when when the CARGO_TARGET_DIR was set to
a relative path, because build.rs doesn't know where workspace root
lives.

The fix is to use OUT_DIR rather than CARGO_TARGET_DIR, which I
think is the supported way to this in the first place. Eg, the cc
crate uses OUT_DIR to store intermediate .o files, which I think
matches our use case pretty closely.

@Longarithm
Copy link
Member

It seems to be conflicting with #6171

near-test-contracts builds some wasm contracts for use in testing. It
does so by recursively invoking `cargo` from `build.rs`. Before this
commit, we tried to re-use parent's `CARGO_TARGET_DIR` to figure out
where we should put the data. That was rather hacky, as cargo doesn't
expose that information to the build scripts in a reliable way:

rust-lang/cargo#9661 (comment)

Naturally, our hacks broken when when the `CARGO_TARGET_DIR` was set to
a relative path, because build.rs doesn't know where workspace root
lives.

The fix is to use `OUT_DIR` rather than `CARGO_TARGET_DIR`, which I
think is the supported way to this in the first place. Eg, the `cc`
crate uses `OUT_DIR` to store intermediate `.o` files, which I think
matches our use case pretty closely.
@matklad
Copy link
Contributor Author

matklad commented Jan 25, 2022

Yeah, I think this PR is still an improvement over that:

@matklad matklad requested a review from nikurt January 25, 2022 11:53
@matklad
Copy link
Contributor Author

matklad commented Jan 25, 2022

@nikurt could you merge this when the time is convenient? I thing this PR is a good idea, but, if #6171 works ok, and we are in the process of releasing something (which I don't know), I don't want to add extra posibility of something's going wrong just now.

@near-bulldozer near-bulldozer bot merged commit 8d710e2 into master Feb 1, 2022
@near-bulldozer near-bulldozer bot deleted the m/unbreak-build branch February 1, 2022 12:14
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.

4 participants