Fix contracts compilation with `CARGO_TARGET_DIR` set #2927

nazar-pc · 2024-01-15T08:18:30Z

In case CARGO_TARGET_DIR is set, build artifacts were in the wrong place and build.rs was failing. With CARGO_TARGET_DIR=/home/nazar-pc/.cache/cargo/target:

error: failed to run custom build command for `pallet-contracts-fixtures v1.0.0 (/web/subspace/polkadot-sdk/substrate/frame/contracts/fixtures)`

Caused by:
  process didn't exit successfully: `/home/nazar-pc/.cache/cargo/target/debug/build/pallet-contracts-fixtures-35d534f7ac3009e0/build-script-build` (exit status: 1)
  --- stderr
  Error: Failed to read "/tmp/.tmpiYwXfv/target/wasm32-unknown-unknown/release/dummy.wasm"

  Caused by:
      Can't read from the file: Os { code: 2, kind: NotFound, message: "No such file or directory" }

The file was actually in /home/nazar-pc/.cache/cargo/target/wasm32-unknown-unknown/release/dummy.wasm.

athei

Can you add the same line for the riscv build, too?

nazar-pc · 2024-01-15T11:01:12Z

I believe it is not necessary there due to env_clear call

athei · 2024-01-15T11:09:38Z

You are right. We should probably do the same for the wasm build then. It seems more robust to remove all variables and only forward/set what we need. But let's wait for @pgherveou opinion.

nazar-pc · 2024-01-15T11:11:20Z

I thought about that too, but with switch to huge monorepo compiling everything is extremely painful locally so I decided to not touch what seems to work fine as is. Feel free to experiment with it though.

pgherveou · 2024-01-15T11:30:43Z

You are right. We should probably do the same for the wasm build then. It seems more robust to remove all variables and only forward/set what we need. But let's wait for @pgherveou opinion.

I think I tried that and it was failing for some reason. I did not investigated much more since it was working without clearing.
let met check this quickly again

pgherveou · 2024-01-15T11:37:41Z

You are right. We should probably do the same for the wasm build then. It seems more robust to remove all variables and only forward/set what we need. But let's wait for @pgherveou opinion.

I think I tried that and it was failing for some reason. I did not investigated much more since it was working without clearing. let met check this quickly again

seems to work ¯_(ツ)_/¯, let me test in docker, push an update and see if that pass CI

nvm, does not work in CI, it seems to pick up the wrong clang binary

pgherveou · 2024-01-15T11:55:29Z

substrate/frame/contracts/fixtures/build.rs

@@ -203,6 +203,7 @@ fn invoke_wasm_build(current_dir: &Path) -> Result<()> {

 	let build_res = Command::new(env::var("CARGO")?)
 		.current_dir(current_dir)
+		.env("CARGO_TARGET_DIR", current_dir.join("target").display().to_string())


shouldln't we just clear it if it set, instead of setting it ?
or the fixtures will end up here instead of OUT_DIR?

I haven't thought of it right away, but clearing should work as well

It has to end up wherever you expect to find them in the rest of your script. In this case current_dir/target is the same as the default. Otherwise the CI would fail. So deleting should have the same effect. But let's find out why clearing the environment doesn't work.

let's stick with the current fix

if we wish to clear env, This works as well

.env_clear() .envs([ ("PATH", env::var("PATH").unwrap_or_default()), ("CARGO_HOME", env::var("CARGO_HOME").unwrap_or_default()), ("RUSTC", env::var("RUSTC").unwrap_or_default()), ])

athei · 2024-01-15T12:06:10Z

You are right. We should probably do the same for the wasm build then. It seems more robust to remove all variables and only forward/set what we need. But let's wait for @pgherveou opinion.

I think I tried that and it was failing for some reason. I did not investigated much more since it was working without clearing. let met check this quickly again

seems to work ¯_(ツ)_/¯, let me test in docker, push an update and see if that pass CI

nvm, does not work in CI, it seems to pick up the wrong clang binary

This is weird. We don't depend on C code in our test fixtures?

pgherveou · 2024-01-15T12:19:56Z

You are right. We should probably do the same for the wasm build then. It seems more robust to remove all variables and only forward/set what we need. But let's wait for @pgherveou opinion.

I think I tried that and it was failing for some reason. I did not investigated much more since it was working without clearing. let met check this quickly again

seems to work ¯_(ツ)_/¯, let me test in docker, push an update and see if that pass CI
nvm, does not work in CI, it seems to pick up the wrong clang binary

This is weird. We don't depend on C code in our test fixtures?

that's when compiling dependencies-

  --- stderr
      Updating crates.io index
     Compiling proc-macro2 v1.0.76
     Compiling unicode-ident v1.0.12
     Compiling paste v1.0.14
     Compiling polkavm-common v0.4.0
     Compiling bitflags v1.3.2
  error: linker `clang-15` not found
    |
    = note: No such file or directory (os error 2)

  error: could not compile `paste` (build script) due to previous error
  warning: build failed, waiting for other jobs to finish...
  error: could not compile `proc-macro2` (build script) due to previous error

athei · 2024-01-15T12:24:36Z

Okay yes we pull in a lot of stuff for macro support. Try to forward the CC and CXX env vars to the command. Let's stick with opt-in until it works.

bkchr · 2024-01-15T12:51:10Z

BTW, this is all already solved here. Not sure what is that complicated to just copy this..

In case `CARGO_TARGET_DIR` is set, build artifacts were in the wrong place and `build.rs` was failing. With `CARGO_TARGET_DIR=/home/nazar-pc/.cache/cargo/target`: ``` error: failed to run custom build command for `pallet-contracts-fixtures v1.0.0 (/web/subspace/polkadot-sdk/substrate/frame/contracts/fixtures)` Caused by: process didn't exit successfully: `/home/nazar-pc/.cache/cargo/target/debug/build/pallet-contracts-fixtures-35d534f7ac3009e0/build-script-build` (exit status: 1) --- stderr Error: Failed to read "/tmp/.tmpiYwXfv/target/wasm32-unknown-unknown/release/dummy.wasm" Caused by: Can't read from the file: Os { code: 2, kind: NotFound, message: "No such file or directory" } ``` The file was actually in `/home/nazar-pc/.cache/cargo/target/wasm32-unknown-unknown/release/dummy.wasm`.

nazar-pc requested a review from athei as a code owner January 15, 2024 08:18

nazar-pc requested a review from a team January 15, 2024 08:18

paritytech-review-bot bot requested a review from a team January 15, 2024 08:19

Fix contracts compilation with CARGO_TARGET_DIR set

9a9eb77

nazar-pc force-pushed the fix-contracts-compilation-with-target-dir branch from 5b83fee to 9a9eb77 Compare January 15, 2024 08:33

athei reviewed Jan 15, 2024

View reviewed changes

athei requested a review from pgherveou January 15, 2024 11:00

pgherveou added the R0-silent Changes should not be mentioned in any release notes label Jan 15, 2024

pgherveou reviewed Jan 15, 2024

View reviewed changes

pgherveou approved these changes Jan 15, 2024

View reviewed changes

athei approved these changes Jan 15, 2024

View reviewed changes

pgherveou enabled auto-merge January 15, 2024 15:44

athei added the T7-smart_contracts This PR/Issue is related to smart contracts. label Jan 15, 2024

pgherveou removed the request for review from a team January 15, 2024 16:35

paritytech-review-bot bot requested a review from a team January 15, 2024 16:36

bkchr approved these changes Jan 18, 2024

View reviewed changes

bkchr disabled auto-merge January 18, 2024 21:18

bkchr merged commit b4b523c into paritytech:master Jan 18, 2024
137 of 140 checks passed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

nazar-pc deleted the fix-contracts-compilation-with-target-dir branch April 3, 2024 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix contracts compilation with `CARGO_TARGET_DIR` set #2927

Fix contracts compilation with `CARGO_TARGET_DIR` set #2927

nazar-pc commented Jan 15, 2024

athei left a comment

nazar-pc commented Jan 15, 2024 •

edited

Loading

athei commented Jan 15, 2024

nazar-pc commented Jan 15, 2024

pgherveou commented Jan 15, 2024

pgherveou commented Jan 15, 2024 •

edited

Loading

pgherveou Jan 15, 2024

nazar-pc Jan 15, 2024

athei Jan 15, 2024

pgherveou Jan 15, 2024 •

edited

Loading

athei commented Jan 15, 2024

pgherveou commented Jan 15, 2024

athei commented Jan 15, 2024 •

edited

Loading

bkchr commented Jan 15, 2024

Fix contracts compilation with CARGO_TARGET_DIR set #2927

Fix contracts compilation with CARGO_TARGET_DIR set #2927

Conversation

nazar-pc commented Jan 15, 2024

athei left a comment

Choose a reason for hiding this comment

nazar-pc commented Jan 15, 2024 • edited Loading

athei commented Jan 15, 2024

nazar-pc commented Jan 15, 2024

pgherveou commented Jan 15, 2024

pgherveou commented Jan 15, 2024 • edited Loading

pgherveou Jan 15, 2024

Choose a reason for hiding this comment

nazar-pc Jan 15, 2024

Choose a reason for hiding this comment

athei Jan 15, 2024

Choose a reason for hiding this comment

pgherveou Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

athei commented Jan 15, 2024

pgherveou commented Jan 15, 2024

athei commented Jan 15, 2024 • edited Loading

bkchr commented Jan 15, 2024

Fix contracts compilation with `CARGO_TARGET_DIR` set #2927

Fix contracts compilation with `CARGO_TARGET_DIR` set #2927

nazar-pc commented Jan 15, 2024 •

edited

Loading

pgherveou commented Jan 15, 2024 •

edited

Loading

pgherveou Jan 15, 2024 •

edited

Loading

athei commented Jan 15, 2024 •

edited

Loading