holochain-rust icon indicating copy to clipboard operation
holochain-rust copied to clipboard

don't run .bashrc for any commands fed from .hcbuild

Open pdaoust opened this issue 5 years ago • 4 comments

PR summary

Resolves #2097

testing/benchmarking notes

Profound apologies; I can't get the silly thing to compile (missing build deps for newrelic-sys and it's late). But the means for testing would be as follows:

  1. Add the line echo "hello" somewhere in your .bashrc file.
  2. Compile a zome with hc package.

On current develop this should fail with a message like:

Error: Couldn't traverse DNA in directory "/home/alice/Holochain/stub-project": artifact path /home/alice/Holochain/stub-project/zomes/stub-zome/code/***hello***
/home/alice/target/wasm32-unknown-unknown/release/stub-zome.wasm either doesn't point to a file or doesn't exist

With this fix it should succeed.

changelog

  • [x] if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

pdaoust avatar Feb 12 '20 05:02 pdaoust

@samrose Thanks for taking the time to review and recmmend! We certainly could tell devs to run nix-shell --pure in the install guide (although it is nice to have most of your comfy environment still available to you). But the concern here is that a new bash -c is spawned for every step of the zome build process, and at least one of the steps captures STDOUT and tries to do meaningful things with it. If any command in .bashrc outputs to STDOUT it borks the build process. So it's less about the host environment leaking into those build steps.

Do you know if --pure causes 'inner' invocations of bash to be pure as well, or is it just the immediate shell created by nix-shell?

pdaoust avatar Feb 12 '20 16:02 pdaoust

I presume the app-spec-tests-sim2h failure is a red herring... looks like it all happens after app-spec is already built.

pdaoust avatar Feb 12 '20 17:02 pdaoust

Hey @samrose just checking in on your change request -- given the extra details I shared, does this seem like a decent solution?

pdaoust avatar Mar 05 '20 22:03 pdaoust

@zippy bumping this PR -- I think it was languishing for a while because it was blocked by some requested changes. I don't know if those requested changes pertain to the issue this is trying to fix though.

I've updated this branch to master and the diff is still clean. It should be an easy merge. The only thing I don't know is whether this causes unintended consequences. Are there times when the inner bash that hc package spawns should actually re-run the .bashrc? Are there times when not running it could break things for the dev? Nothing comes to mind, but hey, these are computers we're talking about -- you never know.

pdaoust avatar Mar 27 '20 16:03 pdaoust