-
Notifications
You must be signed in to change notification settings - Fork 374
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
Allow passing through environment variables #91
Conversation
@japaric please critique the configuration UX (location in toml file). I'm happy to change it. |
@kamalmarhubi Comments in #77 also suggested an addition to the README describing the new variables. |
Having used this in testing where I think the whitelist functionality is working correctly, it'd be helpful if during the CI run the specific whitelisted variables and their values were output to stdout. This would make it clear that a) the values got passed through and b) they're the right values. |
@kamalmarhubi Looks good to me. I'm wondering if we should also have a @Susurrus perhaps we could print the env vars to stdout/stderr if the |
@japaric Yeah, I think that would be useful. I'd be fine deferring that to a later PR however in the interest of getting this released. |
I should be able to do this in the next day or so if that timeline is ok. Otherwise happy to make a follow-up PR. |
That'd be fine. Thanks! |
@japaric could you take a look? I'm unable to test it right now as I'm on a restricted network and downloading docker images isn't an easy option. Also, I'm not super happy with calling this |
2515454
to
367e736
Compare
README.md
Outdated
### Passing environment variables into the build environemnt | ||
|
||
By default, `cross` does not pass any environment variables into the build | ||
environment from the calling shell. Sometimes this is really useful, for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick, but "this" on this line seems to refer to the fact that cross
doesn't pass through environment variables by default when I think you want it to mean the opposite. Maybe instead say "This is chosen as a safe default as most use cases will not want the calling environment leaking into the inner execution environment. But in the instances that you do want to pass through environment variables, this can be done through the use of a whitelist."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Susurrus here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, given what came up in japaric/trust#73, I suggest it also be made clear that Mac doesn't use docker and so all environment variables are automatically "passed through" and this whitelist is only used for docker-targets (currently just Linux).
README.md
Outdated
```toml | ||
[build.env] | ||
whitelist = [ | ||
"TRAVIS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#74 specifically talks about RUST_BACKTRACE
. Maybe add that here as well as a hint to users that that might be useful?
@kamalmarhubi I slilghtly prefer |
I too think that passthrough fits better. @kamalmarhubi Do you think you could add a test for this? Perhaps create a library crate that has a unit test that checks for the presence of some env variable, set that var on the host, place a Cross.toml in that crate and then run |
@kamalmarhubi just wanted to ping you on finishing this up. Do you have some time soon here to wrap up this PR? |
I'm sorry for the delay on this. It's easiest for me to work on non-work stuff at the weekends. I'll get to it this weekend! |
Any updates on this @kamalmarhubi? |
I'm having a bit of trouble testing this, as I can't get
|
I can try to reproduce on my end, what's your test environment? |
Oh I think I see the issue: to use docker I have to be root, but rust and rustup are installed in my home directory. |
Temporarily added myself to the docker group, and it's working. I'll have to test in the evening though. Work beckons! |
367e736
to
e50b7e2
Compare
Apologies for letting this languish. I've rebased and pushed up stuff addressing the review comments. I have tests in progress but not ready to push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise this LGTM.
src/main.rs
Outdated
@@ -461,6 +461,46 @@ impl Toml { | |||
Ok(None) | |||
} | |||
} | |||
|
|||
/// Returns the list of environment variables to pass through for `target`, | |||
/// including variables sepcified under `build` and under `target`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misspelled specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eep, fixed!
@japaric can you take a look and let me know if you need any changes? Also, let me know if I should try to wrap up the tests or if it can be merged as is. |
Thanks for updating this, @kamalmarhubi. It looks good to me. Let's land it. @homunkulus r+ |
📌 Commit 40a3f1c has been approved by |
💔 Test failed - status-travis |
I'll looking into the failure in a couple hours. |
This addresses comments on cross-rs#77.
Add support for environment variables to be specified in `Cross.toml` under the `target.<triple>.env.whitelist` key.
Use nested matching to make it easier to see what's going on.
- rename `whitelist` to `passthrough` - reword description of feature in readme - add `RUST_BACKTRACE` to the example in readme
40a3f1c
to
280917f
Compare
Pushed a build fix, should be good to try again. |
@homunkulus r+ |
📌 Commit 280917f has been approved by |
☀️ Test successful - status-travis |
Awesome work guys, thanks! |
This adapts @dflemstr's work in #77, removing the
whitelist_all
functionality as requested in review.fixes #76
closes #77