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

RFC: Support versioned dependencies on build tools, such as cargo and rustc #1707

Closed
wants to merge 8 commits into from

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Aug 9, 2016

@nrc nrc added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Aug 9, 2016
@Ericson2314
Copy link
Contributor

Heh, I was just about to write something like this. I think rather than working on tools, this should work on languages, i.e. the Cargo.toml language and Rust itself.

Since Cargo is pre 1.0 (officially), we don't have much leeway to leverage Cargo's own semver (no non-breaking minor changes) But since crates.io is live it's definitely safe to say w are on >=1.0 for the Cargo.toml language.

Likewise, other Rust compilers may want to be versioned differently, so it's best to separate language and compiler version. See #1133 for how @brson wanted to have compilers specify the language version.

@Ericson2314
Copy link
Contributor

My motivation, and initial idea of implementation, were sufficiently different that I wrote a separate RFC #1709 . Feel free to take the motivation and adapt it for this one however.

@KalitaAlexey
Copy link

How about some crate that simplifies the task of checking that in build.rs?

@joshtriplett
Copy link
Member Author

@Ericson2314 I like the concept of RFC #1709, but I'd suggest using the dependency syntax as this RFC does, for the same reason discussed in the alternatives section here: doing so allows it to appear anywhere a dependency can appear, such as in dev-dependencies, or platform-specific dependencies, or named-feature-specific dependencies. So, what about lang:rust or similar?

@joshtriplett
Copy link
Member Author

@KalitaAlexey I'd like something completely declarative that Cargo can check in advance, rather than something checked by build.rs at build time. A declarative dependency on tool:cargo, for instance, could easily become a Debian Build-Depends on the cargo package.

@nrc
Copy link
Member

nrc commented Dec 21, 2016

ping @rust-lang/tools - nominating for discussion at next meeting since there's been no movement since August.

@nrc nrc self-assigned this Dec 28, 2016
@nrc
Copy link
Member

nrc commented Dec 30, 2016

Thanks for the PR! We discussed this at the tools meeting this week. We agree there is merit in being able to tie projects to working versions of tools; this has been brought up in the past particularly in the case of nightly, where crates will sometimes only build with certain versions of Rust. However, we saw several downsides to this approach:

  • in general, Cargo won't be able to install versions of tools, so all the user gets is an error message
  • since the relationship between tools can be complex, users could easily make mistakes identifying which tools and versions matter
  • its a lot of implementation work to effectively just show an error message
  • this gets somewhat ugly in the extreme of specifying multiple tools and versions
  • there seems much more motivation for specifying the 'Rust' version (including the language, libs, compiler, and Cargo), than any other tools.

Therefore we prefer the approach in #1709 and I propose closing this PR in favour of that one.

@rfcbot: fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 30, 2016

Team member @nrc has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@japaric
Copy link
Member

japaric commented Dec 30, 2016

@rfcbot reviewed

@joshtriplett
Copy link
Member Author

joshtriplett commented Dec 30, 2016

@nrc I'd very much like to have support for specifying the version of Rust as well. However, the Rust version does not inherently imply a specific Cargo version, as distributions package them separately. So, we still need separate versions for Rust and for Cargo.

In addition, I'm not arguing for just "showing an error message"; while that'd be useful, I'd primarily like this metadata to 1) provide appropriate minimum dependencies, 2) document and test minimum version requirements, and most importantly 3) allow older versions of Cargo/Rust (after this point) to detect that they can't handle a certain version of a library, and go looking for an older version that supports that version of Cargo/Rust.

On top of that, the approach in #1709 fails to parse with existing Cargo (unlike this approach), doesn't support any other tool, and doesn't integrate with all the existing Cargo facilities to make dependencies optional. For instance, consider a crate that needs Rust 1.14 with a given feature enabled, but Rust 1.10 with that feature disabled. The approach in #1709 can't support that; the approach in this RFC can.

If the primary concern is that you care more about specifying the version of Rust than the version of Cargo, how about if I withdraw this RFC from consideration for now, and submit a new version of it that specifies how to handle Rust, Cargo, and potentially other tools?

@lambda
Copy link
Contributor

lambda commented Jan 1, 2017

I think that dismissing this approach as a complex way of just showing an error message is missing the key advantage this provides.

One of the big unresolved questions in the Rust ecosystem right now is how to deal with increasing the required version of Rust in crates. For example, @BurntSushi spent a lot of effort ensuring that regex 0.1.x continued to compile with Rust 1.3, as updating the language required would potentially break users; if you were using an older compiler, and did cargo update, it would resolve to using a newer version of regex that required the newer compiler, and your project would fail to build.

With this in place, Cargo should essentially treat it as if you have an implicit exact version dependency on the tools that you are running. That would conflict with the greater version dependency in the newer version of regex, which means the resolver should be able to pick the highest version of regex that still supports the compiler you are running. That would make updating the compiler version requirement in regex no longer a breaking change; cargo update would still be able to resolve to the highest version that still works, and you would only have an issue if you needed to update your regex dependency for newer features, but I think it shouldn't be considered semver breaking to require a newer compiler for newer feature, it should just be considered semver breaking if running cargo update for a project causes it to stop compiling.

since the relationship between tools can be complex, users could easily make mistakes identifying which tools and versions matter

I think that the same is true for versioned dependencies on libraries; you may not actually test against the minimum versions that you specify, or newer versions may turn out to be accidentally incompatible. For applications, this is helped by lockfiles, but it's still possible for libraries to make mistakes and someone using the library to be broken by a cargo update if the version ranges in dependencies are not actually correct.

I don't think that this is a reason to reject this feature, as the problem already exists for version ranges specified by libraries, it's just an impetus for other tooling improvements to, say, do CI testing that tries out various different sets of packages that all satisfy the version dependencies, both for tools and libraries. This feature would give the ability to conscientious maintainers, like @BurntSushi, to specify the version of rustc required and avoid breaking downstream users while still being able to use newer versions in newer minor releases. There will be people who don't specify this properly, and so may cause crates to fail to compile rather than resolving correctly, but that problem already exists in library dependency specifications.

I agree with @joshtriplett that this proposal seems more compatible with existing versions of Cargo, and more flexible than the one in #1709.

I think that this proposal should probably be rewritten to cover its use for specifying the version of Rust required, and in particular, flesh out whether that's done by a tool:rustc, or some kind of abstract tool:rust that encompasses rustc and the standard library, or whether the standard library is treated separately as a regular crate. But I think that it should also mention that it can be used for tool:cargo, and potentially other tools in the future, since as @joshtriplett mentions those may be distributed separately in distro packages.

@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 1, 2017

@lambda Thank you for your detailed comment.

I agree that this could make it much easier to maintain support for older versions of Rust and Cargo. For instance, a tool could detect if a crate that declares compatibility with a given version has a dependency (direct or indirect) on a crate that doesn't.

We could also teach Travis and similar CI to understand these declarations, to build and test with the declared minimum versions supported, making sure the declared versions work.

At the moment, versions of the standard library and rustc go hand-in-hand, and using a newer feature from either one means you depend on that version of Rust/rustc, so I don't think it makes sense to declare versions on them separately. However, the same framework would work for some future mechanism to declare an explicit dependency on "std" or parts of it, such as with xargo.

So, for now I'd opt for tool:cargo and tool:rustc. I'll work on an updated version of this RFC.

@joshtriplett joshtriplett changed the title RFC: Support versioned dependencies on build tools, such as cargo RFC: Support versioned dependencies on build tools, such as cargo and rustc Jan 2, 2017
@joshtriplett
Copy link
Member Author

OK, I've updated the RFC to include support for tool:rustc. I also added some clarifications for Cargo behavior and for motivation (much more than just error messages), and emphasized benefits for non-Cargo tools as well.

Given the new version of the RFC, would it make sense to defer FCP to let people re-review the new version, particularly given the overlap with the holiday season?

@seanmonstar
Copy link
Contributor

Something I've run in to plenty in the nodejs world is packages trying to force people to update their node or npm version, even if not using any new features. Usually library developers who cannot fathom there being a cost to upgrading. Thankfully, npm only views these directives as warnings.

I sincerely hope cargo only warns, but doesn't fail when tool versions don't match.

@joshtriplett
Copy link
Member Author

@seanmonstar Good call. I suspect that the Rust community will do a better job with that (given that many Rust developers consciously pay attention to what they depend on and aim to support older versions for many crates), but I'll add something to the RFC about that.

Based on feedback from Sean McArthur.
@joshtriplett
Copy link
Member Author

@manuel-woelker Yeah, I had rustup in mind when hinting at other tools doing something more with the version information.

And yes, explicit declarations like "clippy requires nightly" would help massively.

@michaelwoerister
Copy link
Member

@rfcbot reviewed

@yazaddaruvala
Copy link

Another similar request I've made in Rustup: rust-lang/rustup#460

FWIW, I would also prefer for this information to be Cargo.toml

@brson
Copy link
Contributor

brson commented Jan 20, 2017

I think version pinning and version requirements are something that will happen either officially or non-officially at some point. I believe the std-aware cargo RFC also effectively adds language-version requirements to cargo, and that would need to be taken into account before merging. I'm afraid I'm not up to speed on the details of this particular RFC because it's not a priority feature for me and since it seemed like it was going to be closed, but as with many rfcs, there has been a lot of discussion post-fcp nomination continuing to argue for it.

This feature will probably require more focused attention from @alexcrichton or maybe myself and other tools maintainers to get it through.

@joshtriplett
Copy link
Member Author

@brson I've read the std-aware cargo RFC; it helps declare versions on libraries, but not on rustc or cargo themselves. For instance, crates using no_std may still want a particular version of the Rust language itself, for non-library language features.

Regarding tools development: I will happily write a patch for Cargo to implement this, if that helps.

@alexcrichton
Copy link
Member

Looking over this again, @nrc proposed awhile ago to close this RFC in favor of #1709, which is an RFC for language version requirements rather than tool version requirements. I personally still feel like this is the correct approach if we were to implement this.

@joshtriplett you mention that we need separate versions of Rust and Cargo, but I disagree with this. Effectively Rust and Cargo have the same various as they progress forward in history. Currently Cargo bends over backwards to support multiple rustc compilers, but we're very unlikely to continue doing this into the future. Instead it will be the case that given a Rust compiler there's only one version of Cargo guaranteed to work with that compiler (the one that it was released with).

With that in mind it means that while functionally there are two versions in practice there's only one, the compiler. That aspect is covered in #1709 which is why I'm still in favor of closing this in favor of that RFC (in terms of the idea at least).

@lambda yeah you're definitely correct that it's more than just an error message if this affects Cargo's dependency resolution as well. That, however, is a whole other can of worms, but I'm ok continuing the conversation on #1709.

@joshtriplett
Copy link
Member Author

@alexcrichton

With that in mind it means that while functionally there are two versions in practice there's only one, the compiler. That aspect is covered in #1709 which is why I'm still in favor of closing this in favor of that RFC (in terms of the idea at least).

The most important difference between #1707 and #1709 isn't whether we have one version number or two, or whether one of the version numbers refers to "rust the language" or "rustc the compiler". (Though for Linux distributions it's still important to not assume tight coupling between rustc and cargo.)

The most important difference between the two is how to handle the version number and dependencies on it: via the existing Cargo dependency mechanism that can vary with feature flags, dev-dependencies, and similar (#1707), or via a completely separate field with no ability to vary with any of those (#1709).

(And, as a side effect, whether old versions of Cargo treat the version requirement as a dependency failure (#1707) or silently ignore it and then fail later in more mysterious ways (#1709).)

If #1709 used the existing Cargo dependency mechanism, and supported integration with feature flags, dev-dependencies, and similar, then I'd withdraw this RFC. That one difference is why I'll keep poking at this.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Mar 9, 2017

@alexcrichton I see the benefits of making sure that Cargo and rustc will released together, but that to me is even more version to track the Cargo.toml schema version separately from the Cargo version, as the Cargo version will reflect rustc/rust instead.

@joshtriplett I don't think we actually disagree on much, principle-wise :), at least that's my take-away from both these threads. There's just the specifics of how to make sure old Cargo treats the new syntax correctly. I'm not wild about mashing up tools and library depends together, but I absolutely agree that integration with our existing conditionals (features, targets, etc) is a must. Main thing is I've not gotten around to updating #1709. [To be honest, with the way my other Cargo RFC has sat idle, I haven't felt much of a rush to do so. But if somebody pings me resolving either RFC is "on deck", I'll hop-to.]

@joshtriplett
Copy link
Member Author

joshtriplett commented Mar 9, 2017

@Ericson2314 To make things easier for the tools team to resolve, would you be interested in collaborating on a single revised RFC, incorporating the key learnings and feedback from both #1707 and #1709? I'd be happy to work with you on that; perhaps we could set up a time for a real-time conversation, and then perhaps put together an RFC in Etherpad?

I've also done a fair bit of testing with Cargo to evaluate how it handles various kinds of syntax extensions; we could work out what behavior we want, and make sure that whatever syntax we come up with matches that behavior (since we can't retroactively make existing versions of Cargo behave differently).

@Ericson2314
Copy link
Contributor

@joshtriplett sure!

@Ericson2314
Copy link
Contributor

@eternaleye I, at least, would appreciate if you could join too :).

@eternaleye
Copy link

@Ericson2314 I'm definitely interested

@joshtriplett
Copy link
Member Author

@Ericson2314 Looks like you're on US/Eastern time? I'm in US/Pacific. Let's meet on IRC #rust some time compatible with those, and coordinate from there. Would tomorrow (the 12th) work for you? If so, pick any time you like. If not, we can coordinate something during the following week.

@Ericson2314
Copy link
Contributor

@joshtriplett sounds good! Any time not too earlier should be good for me, but with the time difference that should fall out for free. I'll be loitering on IRC.

@joshtriplett
Copy link
Member Author

@Ericson2314 Sounds great. I'll stay logged in; ping me via /msg on irc.mozilla.org whenever you're ready. IRC nick JoshTriplett.

@joshtriplett
Copy link
Member Author

joshtriplett commented Mar 13, 2017

Summarizing the discussions today: @Ericson2314, @eternaleye, and I met on IRC, talked through the problem, and ended up drafting a successor RFC via Etherpad for both #1707 and #1709. We'll submit that RFC in the near future; that will serve as a baseline to solve the most critical core problem, and then we can build further work on top of that.

joshtriplett added a commit to joshtriplett/rfcs that referenced this pull request Mar 16, 2017
This RFC makes it possible to introduce new Cargo features that older
versions of Cargo would otherwise misunderstand, such as new types of
dependencies or dependency semantics; the new schema version prevents
those versions of Cargo from silently ignoring those features and
misbehaving, and allows dependency resolution to work appropriately for
both old and new Cargo.

This serves as a successor to both rust-lang#1707 and rust-lang#1709, and provides a basis
for other Cargo feature RFCs to build on.

Co-authored by Josh Triplett (rust-lang#1707), @Ericson2314 (rust-lang#1709), and Alex
Elsayed (@eternaleye).
@joshtriplett
Copy link
Member Author

As the original proposer of this RFC, I'd like to formally withdraw it in favor of #1953.

@Ericson2314
Copy link
Contributor

@joshtriplett don't we technically need to do another on top of that, for language and tool dependencies?

@joshtriplett
Copy link
Member Author

joshtriplett commented Mar 16, 2017

@Ericson2314 We need another one on top for Rust language and/or rustc version, and I'd like to help with that RFC as well. But my original goal with #1707 was to handle dependencies on Cargo, and #1953 does that.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Mar 16, 2017

@joshtriplett Oh right, I missed/forget that during our discussion :). I guess I might keep #1709 open for the moment then? Just don't want to come off as duplicitous doing so :). I will certainly amend it to use/require #1953.

@aturon
Copy link
Member

aturon commented Apr 29, 2017

I'm going to close this out, given that its author, @joshtriplett, has said that #1953 subsumes it.

@joshtriplett
Copy link
Member Author

#2182 subsumes both this and #1953.

@kornelski
Copy link
Contributor

#2495

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-dev-tools Relevant to the development tools team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.