Expose setting to set arbitrary environment variables

[aumetra]

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.

[Gankra]

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

cargo-dist/cargo-dist/templates/ci/github_ci.yml.j2

Line 345 in 9be2d30

custom-{{{ job|safe }}}:

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

[aumetra]

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

[Gankra]

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?

cargo-dist/cargo-dist/src/generic_build.rs

Lines 92 to 102 in 078d5cb

	// Pass CFLAGS/LDFLAGS for C builds
	if let Some(cflags) = cflags {
	// These typically contain the same values as each other.
	// Properly speaking, CPPFLAGS is for C++ software and CFLAGS is for
	// C software, but many buildsystems treat them as interchangeable.
	command.env("CFLAGS", &cflags);
	command.env("CPPFLAGS", &cflags);
	}
	if let Some(ldflags) = ldflags {
	command.env("LDFLAGS", &ldflags);
	}

cargo-dist/cargo-dist/src/cargo_build.rs

Line 33 in 078d5cb

let mut rustflags = std::env::var("RUSTFLAGS").unwrap_or_default();

[aumetra]

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 🧍‍♀️)

[Gankra]

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...

[mistydemeo]

Sorry for the delay reviewing!

I agree with Gankra that it'd be nicer to help ensure we set these environment variables just for the subprocesses, and in a way that ensures non-CI builds get the same benefits. The good news is, we already do this in a pretty similar way in both Cargo and generic builds, so it won't require much of a change. What you'll want to do is, right under these two lines:

https://github.com/axodotdev/cargo-dist/blob/main/cargo-dist/src/cargo_build.rs#L194
https://github.com/axodotdev/cargo-dist/blob/main/cargo-dist/src/generic_build.rs#L109

Insert something like this:

    if let Some(environment) = &dist_graph.environment_variables {
        command.envs(environment);
    }

That'll add everything in that environment config to the environment that we use to call cargo build or the generic build.

Does this cover everything you're looking for? Or are you also looking to set RUSTFLAGS? (And want to set that via the environment rather than via .cargo/config.toml?)

[aumetra]

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.

[mistydemeo]

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

[Gankra]

Some variant of this would be good for #788

[aumetra]

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

[Gankra]

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