cargo-dist icon indicating copy to clipboard operation
cargo-dist copied to clipboard

Expose setting to set arbitrary environment variables

Open aumetra opened this issue 1 year ago • 10 comments

This PR implements the environment-variables field under [workspace.metadata.dist], allowing arbitrary environment variables to be set during the CI runs.

These are set via the top-level env field in GitHub Actions.

(Kinda) closes #513


If you look at the new snapshot, you might see that the key of the YAML is also quoted, I was thinking about how to get rid of that before remembering that YAML is horrible and actually just allows just about any horribleness you could think of.
And since YAML is quite literally a JSON superset, quoted keys should be 100% a-okay.

So yeah, that should be fine.

aumetra avatar Nov 17 '23 19:11 aumetra

for the record, the magic incantation to avoid quotes is |safe:

https://github.com/axodotdev/cargo-dist/blob/9be2d30c4ac6ca58c0aa1cdb8510358121da569b/cargo-dist/templates/ci/github_ci.yml.j2#L345

but yeah we avoid using that unless absolutely necessary because ✨reliability

Gankra avatar Nov 17 '23 19:11 Gankra

for the record, the magic incantation to avoid quotes is |safe

Oh, huh.. I never used minijinja before (or jinja in general), that's definitely good to know

aumetra avatar Nov 17 '23 19:11 aumetra

So! while you were gone we just landed a big new prototype feature for "generic" builds with all the gorey details of cflags and whatnot, so cargo-dist is More Than Ever in the business of managing env vars for you. As such, it would be great if this feature was adjusted from "sets in CI" (making it not help with local builds) to "sets it for all cargo-dist builds" (so it will work for local cargo-dist builds). As a plus, no more github_ci.yml.j2 edits for you! All the horrors get to be mine still! 🐱

Just checking, is there any reason why setting these env-vars locally, and not only-in-ci would be an issue for your usecase?

https://github.com/axodotdev/cargo-dist/blob/078d5cb7574a46750434a044bf34a65ee3756060/cargo-dist/src/generic_build.rs#L92-L102

https://github.com/axodotdev/cargo-dist/blob/078d5cb7574a46750434a044bf34a65ee3756060/cargo-dist/src/cargo_build.rs#L33

Gankra avatar Nov 17 '23 20:11 Gankra

Just checking, is there any reason why setting these env-vars locally, and not only-in-ci would be an issue for your usecase?

There shouldn't be, no. I was just very laser-focussed on getting it to work the way I patched it into my workflow right now, forgetting about Potential other solutions, heh


Just a small question about the parts you linked: the first linked piece of code builds a command that then invokes cargo-dist again, running the second part of the code, right?

I just wanna make sure I understood correctly (sorry if the question is a little silly, GitHub mobile's Code browsing UX isn't the best.. and it's 4AM for me 🧍‍♀️)

aumetra avatar Nov 18 '23 03:11 aumetra

Just a small question about the parts you linked: the first linked piece of code builds a command that then invokes cargo-dist again, running the second part of the code, right?

Sorry about the delay, also hit by "it's late and weekend". So no, the two linked pieces of code are parallel paths in cargo-dist. The first one is the new "generic build" backend, which exists for invoking things like makefiles. The second one is the original "cargo" back end which is building up an invocation of "cargo build" (and any other tools).

I'll admit it's a bit more complex to merge random env-vars here than set them in the CI itself... hmm I wonder if we can get away with mutating our own process' environment earlier on...

Gankra avatar Nov 20 '23 15:11 Gankra

Or are you also looking to set RUSTFLAGS? (And want to set that via the environment rather than via .cargo/config.toml?)

Actually, that was the main motivator behind this PR. cargo-dist sets some RUSTFLAGS (last time I checked at least) via the environment, which then overwrites the flags set in the cargo config (because cargo doesn't to unification in this case).

But cargo-dist preserves the environment variable. Hence this PR that allows me to set the RUSTFLAGS via the environment instead.

aumetra avatar Dec 06 '23 11:12 aumetra

Got it! That makes sense. In that case, what about this - on this line: https://github.com/axodotdev/cargo-dist/blob/main/cargo-dist/src/cargo_build.rs#L33

You can add a new check to query these environment variables you've added. In that case, I'd say the priorities should look like this:

  • First, prefer the environment (like now)
  • Then environment configured via Cargo.toml
  • Then the default

mistydemeo avatar Dec 07 '23 22:12 mistydemeo

Some variant of this would be good for https://github.com/axodotdev/cargo-dist/issues/788

Gankra avatar Feb 21 '24 20:02 Gankra

Sorry, I will have a look soon. My time schedule just has been a bit tight lately and I lost track of things

aumetra avatar Feb 22 '24 07:02 aumetra

oh haha, this was much more "i dropped the ball and should help you" than vice versa, no worries :)

Gankra avatar Feb 22 '24 13:02 Gankra