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

rust-toolchain.toml support #56

Closed
Ch4s3r opened this issue Apr 29, 2022 · 12 comments · Fixed by #66
Closed

rust-toolchain.toml support #56

Ch4s3r opened this issue Apr 29, 2022 · 12 comments · Fixed by #66
Labels
type:enhancement A general enhancement

Comments

@Ch4s3r
Copy link

Ch4s3r commented Apr 29, 2022

Would it be possible to support for the rust-toolchain.toml?
This would be quite convenient, as you don't need to specify the options for the toolchain also when running pack.
I have one in my repo already so everybody who checks out that repo does not need to bother with setting the correct settings for their toolchain and also the CI runs the tests with these settings then.
For deploying it would also be awesome to have these settings taken from this standardized file instead of specifying them again in a project.toml file.

My project.toml looks like this:

[[build.buildpacks]]
uri = "docker.io/paketocommunity/rust:0.9.0"

[[build.env]]
name = "BP_RUST_PROFILE"
value = "default"

[[build.env]]
name = "BP_RUST_TOOLCHAIN"
value = "nightly"

rust-toolchain.toml

[toolchain]
channel = "nightly"
components = ["rustfmt", "clippy"]
profile = "minimal"

Yes they diverge, but that's not the point of this ticket 😉

@Ch4s3r Ch4s3r changed the title rust-toolchain.toml support rust-toolchain.toml support Apr 29, 2022
@dmikusa dmikusa added the type:enhancement A general enhancement label Apr 29, 2022
@dmikusa
Copy link
Contributor

dmikusa commented Apr 29, 2022

Absolutely. Thanks for the feature request.

@dmikusa
Copy link
Contributor

dmikusa commented May 15, 2022

@Ch4s3r - I've been playing around with this and I'm a little unclear about the recommended workflow for the rust-toolchain.toml file. I know that you can check this into source control and that rust tools look at this command to determine what versions should be used.

What's unclear is if rustup is supposed to install these for you and if so, what command should be run to make that happen.

In my testing, if I enter the directory that contains rust-toolchain.toml nothing happens. Now, if I run rustup show it not only shows me my toolchains/targets but it actually goes and downloads/installs any that are missing from rust-toolchain.toml. To me, this seems odd that running a command like rustup show would have a side effect of installing things. At the same time, commands like rustup update and rustup check do not have the behavior above. I had initially expected rustup update would do this, since it's a command that's intended to modify the state of what's installed.

That all said, I can make the buildpack do this, but I'm just trying to find somewhere that documents the expected behavior around this command so what I add into the buildpack is stable going forward.

What is your understanding of the workflow here?

@Ch4s3r
Copy link
Author

Ch4s3r commented Jun 16, 2022

Yes it's quite unexpected that rustup show modifies state.
For the overall goal, without going to much into detail which command should be used, I expected if I have a rust-toolchain.toml or a rust-toolchain file, rustup will download the expected toolchain and also set the rest of the fields mentioned in the config (channel, components, targets, profile).

There is an issue about moving away from installing/downloading toolchains via rustup show as it's not quite intuitive.
And there is also an issue searching for alternatives.
But I could also not find any alternative how to install it without rustup show.

@dmikusa
Copy link
Contributor

dmikusa commented Jul 2, 2022

Thanks for those links. I've subscribed to them, so I'll follow along with progress there.

In the meantime, I will add in a call to rustup show so that it will install any tools mentioned in these files.

@Ch4s3r
Copy link
Author

Ch4s3r commented Jul 2, 2022

Sounds like an ok solution for the time being.

dmikusa pushed a commit that referenced this issue Jul 3, 2022
This PR will optionally install Rust as specified in a 'rust-toolchain.toml' file. If a 'rust-toolchain' file is present, that file is used first. If not present, then 'rust-toolchain.toml' will be used. If both are present then 'rust-toolchain' is used.

When a 'rust-toolchain' or 'rust-toolchain.toml' is specified, then the buildpack does not use 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN'. The two are mutually exclusive. The 'BP_RUST_TARGET' is still installed, as this is necessary if you are running on the Tiny stack.

Resolves #56

Signed-off-by: Daniel Mikusa <[email protected]>
@dmikusa
Copy link
Contributor

dmikusa commented Jul 3, 2022

I've got a PR queued up for this. When it get's merged, I will cut a release and you can test it out.

It's pretty simple though. If the file exists, the buildpack will just run rustup show and defer to rustup to read rust-toolchain.toml and install what's listed in it. If the file doesn't exist, then we use the env variables to determine what is installed. It worked well in my testing.

@Ch4s3r
Copy link
Author

Ch4s3r commented Jul 3, 2022

Would it be better to have the env variables priority in case one exists? Otherwise it would be hard to override on the cli. Not sure if this is possible easily.

@dmikusa
Copy link
Contributor

dmikusa commented Jul 4, 2022

My thought was that if the file is present, then you know what you're doing and the buildpack should use the values from the file. I could flip it around and make the env variables override the config file, but off the top of my head, I think it would have to be all or nothing so all env variables or all config file. My thought is that it would get messy trying to run the config file and then add the env variable changes on top of that.

I can play around with it though if you feel like there's a use case for it. My thought was that if you have the file and want something different, you would just edit the file and rebuild (as opposed to setting env variables). Does that seem OK, or can you see a case where setting the env variables would be a lot more convenient?

@Ch4s3r
Copy link
Author

Ch4s3r commented Jul 4, 2022

Only suggested it because that's how it's done in spring boot, where env variables override application.properties config files.

@Ch4s3r
Copy link
Author

Ch4s3r commented Jul 4, 2022

As the rust-toolchain file is anyway rather small compared to an application.properties file, I don't really see a use case and would think taking the config file as a whole first would be fine.

dmikusa pushed a commit that referenced this issue Jul 5, 2022
This PR will optionally install Rust as specified in a 'rust-toolchain.toml' file. If a 'rust-toolchain' file is present, that file is used first. If not present, then 'rust-toolchain.toml' will be used. If both are present then 'rust-toolchain' is used.

When a 'rust-toolchain' or 'rust-toolchain.toml' is included, then the buildpack will run 'rustup' and install Rust as specified in the file. If 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN' are also set (non-default values), the buildpack will install them as well. If 'rust-toolchain' or 'rust-toolchain.toml' are not present then the buildpack will install 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN'.

The additional target as set in 'BP_RUST_TARGET' is still installed, as this is necessary if you are running on the Tiny stack.

Resolves #56

Signed-off-by: Daniel Mikusa <[email protected]>
dmikusa pushed a commit that referenced this issue Jul 5, 2022
This PR will optionally install Rust as specified in a 'rust-toolchain.toml' file. If a 'rust-toolchain' file is present, that file is used first. If not present, then 'rust-toolchain.toml' will be used. If both are present then 'rust-toolchain' is used.

When a 'rust-toolchain' or 'rust-toolchain.toml' is included, then the buildpack will run 'rustup' and install Rust as specified in the file. If 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN' are also set (non-default values), the buildpack will install them as well. If 'rust-toolchain' or 'rust-toolchain.toml' are not present then the buildpack will install 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN'.

The additional target as set in 'BP_RUST_TARGET' is still installed, as this is necessary if you are running on the Tiny stack.

Resolves #56

Signed-off-by: Daniel Mikusa <[email protected]>
@dmikusa
Copy link
Contributor

dmikusa commented Jul 5, 2022

Only suggested it because that's how it's done in spring boot, where env variables override application.properties config files.

That's a fair point. In other buildpacks, we typically (always?) have the env variables be the most specific configuration option and allow them to override other configuration methods.

I played around with it and think I have it working a little better. It could help if you are trying to adjust the build but not adjust the source control commit.

  1. If the config file exists, it uses it.
  2. If you also set env, it'll run the rustup install with them. It doesn't necessarily override the file, but it'll install what's configured in the env also.
  3. If the config file does not exist, it'll rustup install what's in the envs.

The only downside is if you have the same values set in envs as in the config file, it'll run rustup twice which will on the second run just say that everything is up-to-date.

ForestEckhardt pushed a commit that referenced this issue Jul 6, 2022
This PR will optionally install Rust as specified in a 'rust-toolchain.toml' file. If a 'rust-toolchain' file is present, that file is used first. If not present, then 'rust-toolchain.toml' will be used. If both are present then 'rust-toolchain' is used.

When a 'rust-toolchain' or 'rust-toolchain.toml' is included, then the buildpack will run 'rustup' and install Rust as specified in the file. If 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN' are also set (non-default values), the buildpack will install them as well. If 'rust-toolchain' or 'rust-toolchain.toml' are not present then the buildpack will install 'BP_RUST_PROFILE' or 'BP_RUST_TOOLCHAIN'.

The additional target as set in 'BP_RUST_TARGET' is still installed, as this is necessary if you are running on the Tiny stack.

Resolves #56

Signed-off-by: Daniel Mikusa <[email protected]>
@dmikusa
Copy link
Contributor

dmikusa commented Jul 7, 2022

FYI, I just cut 1.4.0 which has this support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants