-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Forbid setting RUSTC_BOOTSTRAP from build scripts #6608
Conversation
r? @ehuss (rust_highfive has picked a reviewer for you, use r? to override) |
It doesn't seem productive to introduce a change whose purpose is to deliberately break a pattern that a production user of Rust (in this case Firefox) uses to make things work. Note that your change would most likely have the opposite effect of what you want: Right now, using CC @glandium. |
The effect that I want is that I don't want crates to stop building for weird reasons such as circumventing the entire stable/unstable mechanism. This patch prevents that. |
It’s hard to imagine how a |
@hsivonen The entire point of the RUSTC_BOOTSTRAP variable is to serve as a big gate to not use this in production. If crate-granular opt-in to nightly were a desirable feature, there'd be a flag for that. Especially the engineering org that produces huge parts of Rust should take great care not to spearhead hacks against the language ecosystem, which this essentially is :(. With some effort, I could definitely see cargo/rustc diagnostics that collects used |
Compare this search with this search.
I encourage considering this from the perspective of what's practical (regressing Firefox performance by disabling portable SIMD or not letting Firefox adopt features like allocator and OOM customization on the schedule Firefox needed them is not practical) and not from the perspective of whether the Firefox build system does something that contradicts how things are supposed to work. Realistically, the alternative to the |
This Could the mozilla-central repository have a patched version of |
It's a possibility, but I thought it would have been impolite to just fork |
With my Firefox build system peer hat, the whole point of the hack is to avoid setting RUSTC_BOOTSTRAP globally, which we were doing in the past! And that's worse than a few crates doing it explicitly, because /any/ crate can start requiring nightly features without anything being noticed (and IIRC that has happened when we did set RUSTC_BOOTSTRAP globally). And we clearly don't want to require nightly rust to build Firefox. With my Rust developer hat on, I have used this hack in some crates (boxext and allocator_api ; although both don't need it anymore). I don't think cargo should actively prevent people who want to do the extra work of supporting stable crates on top of unstable rust features. And I don't think that should be forbidden on crates.io either. Or if that is, following the same logic, crates that only work on nightly rust should be forbidden too. |
I agree that making this change seems like a step backwards rather than forwards, even though I would love to see no downstream users (especially as large as Firefox) using There's really nothing we can do, and perhaps nothing we even want to do, that would entirely remove this from something people can do. As such, making it as easy as possible to limit it in scope to as few crates as possible seems like something we should encourage rather than discourage. Speaking from a personal level, publishing such inherently unstable crates on crates.io feels quite a bit worse than using this from within build systems (e.g., rustbuild, whatever Firefox has...). I'm not sure how to quantify this or nail it down though -- I'm not sure what my distinction is exactly. |
I'm confused here. The left side is vastly dominated by I'm not convinced.
I don't find it practical to take a sledgehammer to a system that people have spent years to build and argue for the sledgehammer. Appeals to pragmatism are fine, but I'd also like to appeal the damage this example does.
There is a realistic alternative to set the compiler to bootstrapping mode build system wide, at the cost of slightly more tooling to vet the dependencies. I see your pain, and I'd be totally willing to argue fast-tracking improvements for your use-case, but this option isn't it. @glandium With my Rust team hat, I'm pretty angry at this behaviour and I think it should actually be banned from crates.io. It leads to de-facto stabilisation, which the whole system around having features on nightly only is built to avoid. The lid is on the cookie jar for a reason. If people want to do this locally in their code tree, fine. I'm quite saddened that we now discuss the validity of this, as it's obviously a terrible hack around systems that were put in place for a reason. |
It is no crate’s business to subvert the stable channel into being the nightly one and then cause compilation failures for people on the stable channel, if I’m using nightly I expect some breakage, if I’m using stable I’m surely not expecting things to break.
That this is even a discussion is frankly disappointing to me. Make an in-tree fork of the dependency in which you want to put this horrible hack in. Make a tool to audit which unstable features are used in which crates. The crate ecosystem should not be corrupted just because the Firefox build system doesn’t want to do otherwise.
…--
Anthony Ramine
Le 29 janv. 2019 à 01:58, Mike Hommey ***@***.***> a écrit :
With my Firefox build system peer hat, the whole point of the hack is to avoid setting RUSTC_BOOTSTRAP globally, which we were doing in the past! And that's worse than a few crates doing it explicitly, because /any/ crate can start requiring nightly features without anything being noticed. And we clearly don't want to require nightly rust to build Firefox.
With my Rust developer hat on, I have used this hack in some crates (boxext and allocator_api ; although both don't need it anymore). I don't think cargo should actively prevent people who want to do the extra work of supporting stable crates on top of unstable rust features. And I don't think that should be forbidden on crates.io either. Or if that is, following the same logic, crates that only work on nightly rust should be forbidden too.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Random "stable" crates on crates.io break, that's a matter of fact. Some have even broken with stable rust updates. That's rare but that happens. Others have broken because of semver problems and other subtleties. There are also breaks when a crate uses new stable rust features. Those are theoretically less of a problem because you can update your rust compiler, but they do happen (and no, you can't always update your rust compiler). That's just one different way things can break, and my promise when I use this hack in published crates is that semver-compatible versions will fix the problem for newer stable versions of rust. |
“Stability without stagnation” is a big part of the messaging around the Rust language. We tell people that if they are on the Stable release channel, they can (mostly) count on upgrading up to every 6 weeks being relatively painless. At the same time, we have a mechanism for experimenting with implementation of features that are not ready yet. It should be assumed that any unstable feature will break eventually. If betting on them staying unchanged starts being a widespread practice, we lose the point of separating them from stable features in the first place. So reliance of unstable features must require some form of deliberate opt-in, which comes with responsibility to deal with inevitable breakage. With crates.io, the maintainers of a library and the maintainers of an application that uses (possibly indirectly) that library can be different sets of people. It’s important that both of them deliberately opt into instability. Usually the former do so with By publishing It might or might not be the place of tooling to try to technically prevent this, but I think it should be considered socially unacceptable. This is a Tragedy of the commons waiting to happen. |
@glandium That's all fine, but the project has set up RUSTC_BOOTSTRAP for a reason (it's for bootstrapping, and for bootstrapping only). Using it in your build tooling is already sketchy, but at least local to your project. I can empathise with that. What you are doing is a dangerous boundary play around the rules we have set up on the public crate host, setting a dangerous example. Those rules are for users to keep things predictable and non-confusing. You're only adding to the confusion. People compiling with stable expect that nightly features are not available. OUR promise is something different. |
FWIW, a github search for |
Cargo shouldn't prevent users from doing whatever they way in their local crates although it could warn with something like: "RUSTC_BOOTSTRAP is unsupported for usages other than compiler bootstraping, you are on your own now". Crates.io definitely should forbid publishing crates with |
So there are three issues here:
For the first item:
Clearly, it was created for bootstrapping only, but practical experience from a notable production user of Rust has lead to the identification of a non-bootstrapping use case that it also addresses. A real-world major production user of Rust uses non-nightly compiler builds because 1) those have gone through the process of shaking out short-lived bugs that you might encounter in if you picked an arbitrary nightly build of the compiler and 2) the product is also built with Linux/BSD distros' in-archive compilers also and it's already enough of a departure from distro culture to effectively require them to make rustc and cargo rolling-release packages, so asking them to package a nightly compiler snapshot for Firefox seems even less workable. Yet, there are use cases for using nightly features in Firefox in specific cases:
So I think unlocking nightly features in a stable toolchain for non-bootstrapping purposes have practical use cases, and making things harder in practice for those facing these use cases because the use cases are not supposed to exist in theory would be entirely counterproductive. For the second item: In practice, unlocking nightly features on a per-crate basis instead of unlocking them for all crates helps minimize unintended casual proliferation of dependencies on nightly-only features. I can see how the solution is theoretically repulsive, but I suggest it's bad to double down on theory if the expectable practical consequences run counter to the goal of the theory. While these are Open Source tools and there isn't outright DRM preventing me from patching the tools and using them to address use cases that I have (although there is the issue of Linux/BSD distros having to also patch the tools if they are to use the tools to build something that needs the modifications to the tools in order to build), I'm unhappy about tools deliberately refusing to work for the person trying to operate them when practical use cases don't conform to the theory of the tools. In that sense, I hope this PR is rejected, but I'm much more committed to the first item than this second item, so I could live with Firefox going back to blanket
I don't think it's reasonable to expect the burden of writing such tools to be on the users for whom As far as writing new tooling code goes, the ability to unlock nightly features declaratively for specific crates in the dependency graph from a top-level For the third item:
I'm sympathetic to the notion that However, it's worthwhile to observe that just accepting the PRs for the trunk of these crates and publishing on crates.io was the easiest path and why that is so:
I see parallels here: An opinionated tool taking the "you are not supposed to do that" position. And in this case, the easiest way to get work done in the context of that "you are not supposed to do that" opinionated tool position is to upload Firefox peculiarities to crates.io so that you only need to run Now, do you double down on embodying "you are not supposed to do that" in tools or address use cases faced by production users of the tool? (I this case, AFAICT, being able to unlock nightly features for a particular crate in the dependency graph from within the top-level |
How does rustbuild avoid being affect by this? |
@dwijnand |
@hsivonen I fundamentally don't agree with the notion you're trying to pass here, namely that the practical use is fine and the concerns are purely theoretical. Even local opt-in has lead to serious situations in many language communities ("I love Haskell, it's a PRAGMAtic language"). Yes, there's a philosophy behind the way Rust works there and this philosophy gives people very simple ways to work. This PR wouldn't be up if you didn't actually widen the scope of your RUSTC_BOOTSTRAP use and if you didn't actually publish your crate on crates.io. You've been caught doing it and now you are arguing that this has been sound all time. It's not like "keep our use of RUSTC_BOOTSTRAP local" couldn't also be framed as a question of philosophy. Rust stable not allowing any use of nightly features is a feature of the language ecosystem. Yes, it brings pains, but these are to disable other, very real, and practical, pains.
It's not about It's not "you are not supposed to do that", it's "this is potentially harzardous" and you are using a big project to push it. FWIW: I'm also happy for working on practical solutions around that stalement, I just don't think the current path and action is feasible. |
If that is the use-case, could we modify
|
I'd be happier if there was an easier way for top-level compilation to control this on a per crate basis than having the code travel through crates.io. (To be clear: I don't think those
Being able to set a list of crates that |
Yes, cutting corner can be easier. That doesn’t make the negative consequences go away. Maybe crates.io makes it easier to make code written for Firefox available to a Firefox build. In that context, go ahead and cut all the corners you want. You’re the one who will deal with the breakage later. What’s problematic IMO is shipping this hack in a library that you publicize as fit for other projects to use, without your other users having necessarily opted into instability. This is where you’re hurting the ecosystem the most. |
As noted, the purpose of this change was to limit the negatives of blanket You are right, though, that having this in
Whoa. That's a readme editing oversight. Sorry. I'll fix that as soon I'm on a computer that can push to GitHub. |
Hmm, do you mean that encoding_rs should not be outside of Firefox? What about rust-lang/packed_simd#205 ? |
I haven't closed @hsivonen PR in IIUC using The long term solution is for Firefox to stop passing any kind of We could add a I'll prefer to wait for such a workaround to be implemented, and for Firefox to migrate from using this hack on |
I created a prototype impl in the compiler: https://github.com/rust-lang/rust/compare/master...oli-obk:hacky_mac_hackstrap?expand=1 I think we should indeed go with the cargo solution instead of a rustc solution, it seems like a cargo solution will be significantly simpler. |
No, I mean the documentation should have made it clear that the
It seems clear from the top of the readme that
In the case of SIMD, my hope is that the long-term solution is that As noted above, solving the SIMD case doesn't mean that the use case will go away. The allocator/OOM customization features were using nightly-only features for a couple or so of Rust release cycles. It seems reasonable to expect that similar cases will pop up from time to time in the future (hopefully unlike SIMD only for a couple of Rust release cycles each), so the use case of opting a crate into nightly-only features on non-nightly Rust will remain even if not continuously.
Thank you. |
If we're going to have a way for cargo to do something per crate, I think it would be more interesting to have a completely generic way to pass arbitrary environment variables per-crate, which kind of overlaps with an idea I had yesterday to help with some problems with build flags and want to send a pre-RFC for, some time today or tomorrow. |
We discussed this briefly in today's @rust-lang/core meeting, since it seemed like the questions being raised here go to the heart of Rust's stability story. We didn't decide ultimately whether to accept or not accept this PR. We plan to revisit this discussion in the meeting after the all hands (or perhaps a bit during all hands). (In particular, it'd be nice to find the "right team" to make the final call here.) Some comments: First, the decision not to allow crates to "encapsulate" the use of nightly features was definitely deliberate, and @nox is correct that using We know that a careful crate author can issue point releases to fix those that kind of breakage, but of course not all crate authors are so careful and diligent. Moreover, it may happen that crate authors don't want to go back and issue point releases for ALL prior semver versions, since often people only maintain the most recent branch of a crate and so forth. This means that upgrading rustc may cause breakage for people who happen to be using older versions even if the crate author is diligent. It is of course also true that, as @glandium notes, breakage can happen in other ways, such as bug fixes and the like. But such breakage is far less common than changes to nightly features, so there does seem to be a difference in kind. One point raised was that we don't presently have a precedent for banning similar things in build.rs, so there may be some concern about opening that door. I guess more generally there is the question of whether a large number of productive users are using |
Although the core team intends to discuss further, I think it is safe to close this PR for now - although we might well do something like this, I think we'd want to enforce this server-side on crates.io instead of/as well as in Cargo. @nox thanks for the PR and for bringing the issue to our attention |
I filed a feature request for addressing this use case in a way that doesn't involve baking application-specific configuration into general-interest crates.io crates. |
I just want to note that, tho it is not the implication of what you are saying, with my language team hat on (but speaking only for myself...), I never want to find myself in a situation where I'm forced to OK the stabilization of a feature due to |
Was this discussed at the all hands?
Could we schedule a crater run that does not allow this gather some information about how many crates are doing this? |
I don't know if it came up elsewhere, but. I chatted with some people about this at the all hands.
|
Rather than commenting on a closed PR, please add your comments on the open feature request issue. Thanks! |
Not including this patch has now had wider ecosystem consequences: https://www.reddit.com/r/rust/comments/c8bgwf/ripgrep_dependency_has_been_marked_for/ I'm going to include a comment over on the feature request as well, but didn't want the consequence to get lost. |
rust-lang/packed_simd#205