Skip to content
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

add env option to pass environment like RUSTFLAGS #36

Closed
mokurin000 opened this issue Jan 7, 2025 · 9 comments
Closed

add env option to pass environment like RUSTFLAGS #36

mokurin000 opened this issue Jan 7, 2025 · 9 comments

Comments

@mokurin000
Copy link
Contributor

mokurin000 commented Jan 7, 2025

For example, to profile build time with RUSTFLAGS="-Z self-profile" (and collect *.mm_profdata),
setting RUSTFLAGS is required

Update: for this case, setting build.rustflags in .cargo/config.toml could fit

@autarch
Copy link
Member

autarch commented Jan 11, 2025

You can set rustflags globally in .cargo/config.toml, but you couldn't do it for just one build. It looks like the ideal way to do this would be per-profile rustflags, but that's not stable yet. There's a tracking issue for this at rust-lang/cargo#10271

In the meantime, it wouldn't be very hard to add support for this to the action.

@mokurin000
Copy link
Contributor Author

mokurin000 commented Jan 11, 2025

You can set rustflags globally in .cargo/config.toml,

Yes, I workaround things by overwriting it for now

but you couldn't do it for just one build. It looks like the ideal way to do this would be per-profile rustflags, but that's not stable yet.

That's ok for this scenario, while -Z's are nightly-only feature.

However, adding the env option will support CARGO_* well too

@autarch
Copy link
Member

autarch commented Jan 20, 2025

I was thinking about how I'd actually implement this, and I'm not sure it's worth doing. All of the inputs that an action defines must be strings. The caller can use things like YAML boolean values, but in the action this is a string. This is why I have to write things like this in the action YAML: inputs.cache-cross-binary == 'true'

When I added caching, I wanted to accept all of the inputs that the Swatinem/rust-cache@v2 action accepts. The only reasonable way I found to do this was to accept a JSON string and then parse this internally into a list of key-value pairs that I can then pass to rust-cache. This is really gross, but I think it was the best choice, since otherwise I'd have to make actions-rust-cross accept every input accepted by rust-cache.

This "inputs must be strings" issue also comes up with env vars. What I'd like to do is accept a list of key-value pairs. But since I can only take a string, it'd either have to be JSON or a delimited list of pairs like this:

RUSTFLAGS="-Z self-profile"
CARGO_LOG=debug

And then internally, all I would do with this input is parse it and echo it out to $GITHUB_ENV like this pseudocode:

for (key, value) in pairs {
    echo "$key=$value" >> "$GITHUB_ENV"
}

So it seems to me that it'd be a lot easier for callers to just put variables in $GITHUB_ENV directly. The only question is whether variables set in that file are visible in the action. I tested this and it looks like the action does "inherit" all the env vars set this way by the caller. See https://github.com/houseabsolute/actions-rust-cross/actions/runs/12873233495/job/35890333460 for example output.

All of which is to say that I think the best approach here is for me to simply document that you can set env vars yourself this way.

The one thing that this doesn't give you is isolation. Once the vars are set, they're set for all remaining steps in the job. But you could always set them to a different value, including an empty string, after a given step. But I don't think this one downside is enough to justify the complexity of supporting this, especially given how bad the API for passing these to the action would be.

@autarch
Copy link
Member

autarch commented Jan 20, 2025

Actually, I realized there's a very way to set env vars already!

      - name: Run build command
        uses: houseabsolute/actions-rust-cross@v1
        env:
          CARGO_LOG: debug
        with:
          command: build

I will document this in the README.md.

autarch added a commit that referenced this issue Jan 20, 2025
@autarch autarch closed this as completed Jan 20, 2025
@mokurin000
Copy link
Contributor Author

Actually, I realized there's a very way to set env vars already!

      - name: Run build command
        uses: houseabsolute/actions-rust-cross@v1
        env:
          CARGO_LOG: debug
        with:
          command: build

I will document this in the README.md.

will that pass environment variables to docker?

@autarch
Copy link
Member

autarch commented Jan 20, 2025

It looks like cross does pass through all the relevant environment variables - https://github.com/cross-rs/cross/blob/main/docs/environment_variables.md

@mokurin000
Copy link
Contributor Author

It looks like cross does pass through all the relevant environment variables - https://github.com/cross-rs/cross/blob/main/docs/environment_variables.md

https://github.com/cross-rs/cross/blob/main/docs/environment_variables.md#environment-variable-passthrough

Great, they said much of cargo-related environment variables will passthrough by default
So the problem comes to other environment variables, like custom env!, or RUST_LOG e.g.

Could we access original env entry from action, add them to Cross.toml -> build.env.passthrough ?

@autarch
Copy link
Member

autarch commented Jan 20, 2025

This action should definitely not be modifying a Cross.toml file. That seems like a recipe for problems. But I don't see why that would be needed. If you want more env vars to be passed through you can create a Cross.toml for your project, right?

@mokurin000
Copy link
Contributor Author

This action should definitely not be modifying a Cross.toml file. That seems like a recipe for problems. But I don't see why that would be needed. If you want more env vars to be passed through you can create a Cross.toml for your project, right?

Yeah, I will add this note

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants