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

Overrides that can be checked in to version control #460

Closed
yazaddaruvala opened this issue May 14, 2016 · 21 comments
Closed

Overrides that can be checked in to version control #460

yazaddaruvala opened this issue May 14, 2016 · 21 comments

Comments

@yazaddaruvala
Copy link

When working with a team of developers, it is most effective to be building/testing an application consistently.

Hardcoding dependency versions "in code" (managed/shared through version control) has worked out well for all other dependency management. Primarily for reproducibility locally, and across a team.

I can't think of a reason why this same strategy wouldn't work well for toolchain dependency management.

@Diggsey
Copy link
Contributor

Diggsey commented May 14, 2016

What exactly are you proposing? I can see some benefit to being able to pin the rustc version, although I'd hope that the backwards compatibility guarantees would make that unnecessary.

However, if you plan to pin the toolchain, that seems very unhelpful for anyone trying to build the project on a different host.

@yazaddaruvala
Copy link
Author

I'm proposing to move the configuration of "overrides" into source control.

The only difference between the current overrides and this, would be that every developer working on the library or package, are forced to conform and build consistently.

that seems very unhelpful for anyone trying to build the project on a different host.

Different hosts can have conforming build environments; Hence, Docker et al.

Meanwhile you're right:

that seems very unhelpful for anyone trying to build the project on a different build environment.

  1. Given that I've never had to worry about this for any of my projects (I've always had conformed build environments) -- I'm probably the wrong person to propose a solution here, although if needed I could take a stab at it.
  2. Given the workaround is simple: Locally change the Rustup.toml, add Rustup.toml to .gitignore -- Is there even a need to solve this?

@Diggsey
Copy link
Contributor

Diggsey commented May 30, 2016

If you're going to use some form of virtualization/containerization (eg. docker) to get a consistent build environment regardless of the host then it seems like it would make sense to include the rustup/rust installation in that container?

Given that I've never had to worry about this for any of my projects (I've always had conformed build environments)

Pretty much any open source project will have people contributing from a variety of build environments (with the exception of virtualized/containerized builds) and in the remaining cases you can always have a setup script which installs the appropriate override.

Basically I don't see this being widely applicable, and I think it's more likely to be misunderstood as a feature, with people inadvertently checking in their rustup.toml files and causing havoc for anyone else trying to build the project. Given how little effort it is to add an override in the first place, it just doesn't seem worthwhile.

@yazaddaruvala
Copy link
Author

Pretty much any open source project will have people contributing from a variety of build environments

Totally agreed!

Is rustup only meant for open source projects?

@Diggsey
Copy link
Contributor

Diggsey commented Jun 1, 2016

No, hence the second part of my comment: there are a select few cases where this would be useful (closed source projects which support only a single build target but don't use a containerized/virtualized build system), and in those cases there are already readily applicable solutions that aren't really any more effort (for example, checking in a setup script or post-checkout script that adds the relevant rustup override).

@yazaddaruvala
Copy link
Author

for example, checking in a setup script or post-checkout script that adds the relevant rustup override

Sure, this is very possible. However, why should this setup script not be solved by the tool meant to do Rust toolchain management? Why should this relatively common usecase require each team to create a custom script?

Would you also be in favor of a setup script that managed your crate dependencies (it would be extremely simple to implement - curl into the appropriate dir), rather than Cargo.toml?

I very much agree, a script to setup Rustup overrides is a lot easier to maintain than a script to set up Crate dependencies. However, it is still one more thing each team has to maintain. At a fundamental level the same reasons to have a consistent way to manage crate dependencies across a team, should apply to a consistent way to manage toolchain dependencies across a team.

Does that make sense? If not, why do we have a disconnect? I'm assuming you prefer Cargo.toml to a custom setup script. If thats true, I'm very curious, why do you prefer Rustup users build a custom script, when Cargo users shouldn't?

@brson
Copy link
Contributor

brson commented Jun 23, 2016

Setting the override declaratively is something supported by rbenv by writing to ./rbenv/version. It's a desirable feature, but for rustup it's a project-wide policy issue. So far we have on several occasions informally decided not to enable e.g. cargo to pin Rust versions. Needs wider discussion before changing rustup to allow projects to be pinned.

@Diggsey
Copy link
Contributor

Diggsey commented Jun 23, 2016

why do you prefer Rustup users build a custom script, when Cargo users shouldn't?

Because the Cargo.toml is completely platform independent, whereas a rustup.toml would be extremely platform specific. I feel there's a strong likelihood of it being used improperly. Additionally, it would result in some new rust version being silently installed on your system, which is not something I'd want to happen automatically.

If we do this, I'd at least like to see it broken down by host architecture, so that for each host target, you specify a corresponding toolchain to use: if your host architecture is not present then it would not try to override your default toolchain. The host architecture it would match against would be that configured by the "default host" setting.

@brson
Copy link
Contributor

brson commented Jul 19, 2016

The std-aware cargo RFC looks like it might incidentally add a type of version pinning: if you write std = "x.y.z" cargo will effectively require a specific compiler.

@emk
Copy link

emk commented Sep 19, 2016

I find myself needing to re-invent this particular wheel for emk/heroku-buildpack-rust, as discussed in emk/heroku-buildpack-rust#11, which needs to know what Rustup channel to use for deploying a given git repo as a Heroku application. I was considering using Cargo.toml's [metadata] section to implement something like:

[metadata]
rustup_channel="stable"

Obviously, if multiple tools are going to need something like this, it would be better to standardize it in one place rather than re-inventing it everywhere.

@wagenet
Copy link

wagenet commented Sep 27, 2016

My big concern is making sure that different developer machines are running consistent versions. Suppose a bug is fixed in a version of Rust and I update my local copy of Rust to avoid it. Then I push out some code changes and another developer on the team accidentally forgets to update it. Then they make a build with the buggy version of Rust. I'd like to be able to at least enforce a minimum Rust version.

@yazaddaruvala
Copy link
Author

@brson kind-of off topic. I know we have had this conversation before, but given the increasing interest in version pinning in general, and version pinning through cargo, are you reconsidering having rustup and cargo be separate tools[0]?

[0] I know, rustup has a lot to do with toolchains and not just std (or compiler) version. While, the linked RFC doesn't currently discuss pinning toolchains, nor does it discuss toolchains at all. However, for me as a user: toolchain version, compiler version, and stdlib version (or picking the lack of std) feel like very similar concepts that I'd prefer to manage in one place.

@metajack
Copy link

metajack commented May 2, 2017

Overrides with a local file would solve many issues with Servo and related projects.

cc @aturon @paulrouget

@Diggsey
Copy link
Contributor

Diggsey commented May 3, 2017

My comment from the duplicate issue:

Something similar was brought up before. At the time I was against it because I didn't want to see people pinning specific compiler host triples to their projects, and making life difficult for people trying to just build it.

The other factor was that the stable compiler should be... stable, so it's unclear whether that should be handled by rustup, or by cargo (say by depending on at least a certain libstd version).

However, another important case is pinning a specific version of nightly, without pinning the host triple, which is quite important for development against nightly where there are frequent breakages. I'd be in favour of something like that, maybe with precedence lower than the existing override system, so that it's still easy to bypass if need be.

In short:

  • Only allow pinning nightly version (and/or commit if that would be useful for servo)
  • Versions specified in dependencies are ignored
  • Existing directory override system has higher precedence

@brson
Copy link
Contributor

brson commented May 12, 2017

I'm inclined to do this in the same way rbenv seems to:

  • The RUSTUP_TOOLCHAIN (or w/e it's called) env var wins
  • Then the existing override database
  • Then look in the cwd for .rust-version, then walk up to the fs root

The .rust-version file contains nothing but a channel name (I assume that's what rbenv does but haven't looked). It is intentionally non-extensible, not a toml file, to discourage expanding the scope for checking rustup customization into git in the future.

@Diggsey's point about not checking in the target triple makes sense. I'm not sure if it's important to stop people from doing that, but if the file only contains a channel name, not a toolchain name, it should be correct by construction. Channels include nightly, nightly-$date, and 1.17.0.

We need to understand how cargo publish interacts with this. I don't think it would be right to publish this configuration.

To start with, we don't add any explicit support for this to the rustup cli. Just get it out there for the cases where people really need it, but the cli could be expanded in the future to work with this file.

@brson kind-of off topic. I know we have had this conversation before, but given the increasing interest in version pinning in general, and version pinning through cargo, are you reconsidering having rustup and cargo be separate tools[0]?

Yes, right now I'm considering implementing this limited feature entirely in rustup to get it out there. The various proposals to put Rust version numbers in Cargo.toml are intertwined with lots difficult design decisions, and don't seem close to realization.

@brson brson changed the title Allowing overrides through a file: Rustup.toml Overrides that can be checked in to version control May 12, 2017
@SimonSapin
Copy link
Contributor

We need to understand how cargo publish interacts with this. I don't think it would be right to publish this configuration.

Rather, I think that cargo/rustc/etc wrapper programs should set RUSTUP_TOOLCHAIN so that nested calls (such as Cargo invoking rustc) use the same toolchain as the original command. This is critical so that dependencies are built with the same compiler as dependents.

As as side effect, even if .rust-version somehow ends up in a tarball on crates.io, it will be ignored when Cargo builds crates.io dependencies.

@pravic
Copy link

pravic commented May 12, 2017

What about .cargo/rustup or .cargo/rustup.toml? Since we already have a .cargo folder which can contain certain Rust configuration files.

@brson
Copy link
Contributor

brson commented Jun 9, 2017

As as side effect, even if .rust-version somehow ends up in a tarball on crates.io, it will be ignored when Cargo builds crates.io dependencies.

Ah that's a good point.

@brson
Copy link
Contributor

brson commented Jun 9, 2017

What about .cargo/rustup or .cargo/rustup.toml? Since we already have a .cargo folder which can contain certain Rust configuration files.

Ah that's a good idea.

It has parallels to the decision to put rustup's own config in ~/.rustup instead of ~/.cargo (which I would like to revisit). In that light it seems like it might be more consistent to keep rustup configs separate.

I wonder if other tools are using the .cargo folder.

@brson
Copy link
Contributor

brson commented Jun 12, 2017

@SimonSapin which toolchains does Servo want to use this for? It strikes me that Servo using "alternate" Rust builds that are not named release channels, and I'm not sure how or if to represent such a thing here.

@SimonSapin
Copy link
Contributor

SimonSapin commented Jun 12, 2017

@brson I think dealing with alt toolchains at all in rustup is a separate issue, namely #1099. I’ve also opened https://internals.rust-lang.org/t/disabling-llvm-assertions-in-nightly-builds/5388 (which, for everyone else, has background on what "alt" builds are). To fully answer your question:

Servo’s bootstrapping scripts (which I understand we’d like to replaced with rustup eventually) download and manage two Rust compilers, both pinned to specific versions (this is the part relevant to this issue) that we can upgrade when we feel like it. One from the Stable channel (e.g. 1.15.1) for Firefox components, and one that supports unstable features for Servo itself.

For the latter, we used to pick a date from the Nightly channel. We switched to "alt" builds when they were made available. Since they are built for every pull request, they’re effectively following the master git branch rather than the Nightly channel. This was a problem when we asked third-party dependencies to upgrade for breaking language or standard library changes that had reached master but not Nightly. Now we’re still using these builds, but we default to upgrading to the commit hash of the latest Nightly, to match the rest of the ecosystem.

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

No branches or pull requests

8 participants