Allow opting into nightly Rust on try

NEW
Unassigned

Status

()

Core
Build Config
7 months ago
a month ago

People

(Reporter: hsivonen, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
Please add some way to opt into using nightly Rust for a try push.

If doing it for all try tasks is hard, having it for Talos would be more important for my use case than having it for unit tests.

Use case: Quantifying the impact of currently nightly-only Rust features on our perf metrics (explicit SIMD in my case).
Doing this for all archs would be a lot of extra jobs. The rust toolchain used is described by the in-tree tooltool manifests; would you be ok with updating them manually? Through some mach command?

Have try build and then use toolchains from some in-tree spec isn't possible yet, I believe (bug 1313111) which is ultimately how we want to do things like this.
(Reporter)

Comment 2

6 months ago
I'm OK with having to manually edit some in-tree file to opt into nightly Rust.

I don't expect to use this often or on an ongoing basis. Once my patch for bug 1261841 is closer to done, I want to be able to get Talos numbers for
1) Without the patch for bug 1261841 (already possible)
2) With the patch for bug 1261841 without nightly Rust features (already possible)
3) With the patch for bug 1261841 with nightly-only Rust features
Could we just set the magic environment variable that enables nightly features in release rustc for our builds?  That would be a lot easier, and possibly even integrable into try syntax (--nightly-rustc or something).
This is the thing that lets stable rustc compile libstd (which uses unstable language features)? That's probably the quickest approach, although rust devs have told me that don't want that to become common practice for fear it will poison their stability story.

Since this is for performance comparison it should really use the same toolchain with nightly features on and off, rather than stable against nightly. Henri, are your uses of unstable features tied to a cargo 'feature' define? Apparently the recommended approach is to tie all the `#[feature]` directives to a feature defined in Cargo.toml (see https://github.com/rust-lang-nursery/lazy-static.rs/blob/master/src/lib.rs#L93 for an example) but there's no way to have the nightly compiler enforce that there are no unstable features enabled.

Since this is a one-off thing, I think the best approach is just to patch the in-tree manifests to use a nightly build uploaded to tooltool as part of the try push. I'm making such uploads regularly as part of bug 1337955.
Comment hidden (mozreview-request)
(Reporter)

Comment 6

6 months ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #4)
> Henri, are your uses of unstable features tied to a cargo 'feature'
> define?

It's currently behind a regular opt-in feature called "simd-accel":
https://github.com/hsivonen/encoding_rs/blob/master/Cargo.toml#L14

I wasn't aware that the ability to test for nightliness had finally made it into Rust.
(In reply to Henri Sivonen (:hsivonen) from comment #6)

> I wasn't aware that the ability to test for nightliness had finally made it
> into Rust.

That's a feature set by cargo, not a rust flag. I find the ad-hoc nature of features a confusing. In that example 'nightly' is defined by https://github.com/rust-lang-nursery/lazy-static.rs/blob/master/Cargo.toml#L20
(Reporter)

Comment 8

5 months ago
https://github.com/rust-lang/rust/pull/41751#issuecomment-299264150 says "we've been printing for over a year that this will become a warning, debugging with these features will continue to work on nightly, as is intended." in response to "I'm somewhat concerned that we'd break Firefox's debugging process by landing this, though, especially with no current clear path forward for debug-macros." where the latter is a reference to bug 1360497.

Regardless of what we do *by default* over in bug 1360497, I would have had a notably worse time addressing the oranges arising from bug 1261841 if it hadn't been possible for me to use -Zdebug-macros on try.

Therefore, the ability to use -Zdebug-macros on try after we update rustc to a version that bans -Zdebug-macros on the Rust release channel is another reason (in addition to enabling SIMD) why I think we need to be able to use nightly-only rustc features in our build automation.

Moving our default build automation rustc from release-channel rustc to either nightly-channel rustc or to a custom build of rustc from a release git tag but with nightly-only features enabled would work even better for me than just having the option to use nightly on try. While moving to a custom build of rustc would be more trouble for releng, I'd expect people to find it less scary than just going all the way to nightly rustc.
(Reporter)

Comment 9

5 months ago
export RUSTC_BOOTSTRAP=1 might solve this without needing a different compiler.
afaik we still don't have the machinery for taskcluster to generate toolchain packages on demand, so this is a little involved, but it doesn't require any involvement from releng. Were you able to use the 1.18 nightly I attached?
(Reporter)

Comment 11

5 months ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #10)
> afaik we still don't have the machinery for taskcluster to generate
> toolchain packages on demand, so this is a little involved, but it doesn't
> require any involvement from releng. Were you able to use the 1.18 nightly I
> attached?

Sorry, I totally missed the attachment, since mozreview comments are collapsed by default. :-(

Patching RUSTC_BOOTSTRAP=1 into config/rules.mk works for me. While I understand (per comment 4) that the Rust developers don't want RUSTC_BOOTSTRAP used that way, I think we should unlock SIMD using RUSTC_BOOTSTRAP, because it would allow Firefox to make progress without having to lose the regression fixing that the Rust release process gives relative to nightly Rust for all other Rust features. That is, I think Firefox should not be forced give up the benefits of the Rust train model for everything else in order to use one or two (SIMD and -Zdebug-macros are the ones I care about) nightly Rust features sooner than Rust considers them done.
Comment hidden (mozreview-request)
Oops, sorry I wasn't more clear. I thought you would see the attachment section.

I've updated the patch with yesterday's nightly, which should be easier to apply to your try pushes. I run these about once a week to check for upcoming issues (bug 1337955) so something recent is generally available. I haven't had a chance to automate the pushes yet though.

> I think we should unlock SIMD using RUSTC_BOOTSTRAP

How can we use this to allow simd, but still prevent other nightly-only code creeping in? I think we'd need some kind of lint for unstable language features.
(Reporter)

Comment 14

5 months ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #13)
> > I think we should unlock SIMD using RUSTC_BOOTSTRAP
> 
> How can we use this to allow simd, but still prevent other nightly-only code
> creeping in? I think we'd need some kind of lint for unstable language
> features.

Rust code still needs to opt into to specific nightly features using feature gates and those feature gates are errors in without RUSTC_BOOTSTRAP. We could catch unconditional feature gate usage by having on CI job (probably one of the analysis jobs) run without RUSTC_BOOTSTRAP.

When crates are unable to use feature gate unconditionally, the can't use them in secret in the sense that they need to be conditional on cargo features and cargo features need to be chained to the cargo features of gkrust, where they are easily visible in review.

So I suggest that we:

 1) introduce a configure flag --enable-rust-simd that sets RUSTC_BOOTSTRAP=1 and sets the 'simd-accel' cargo feature for gkrust (which in turn sets the 'simd-accel' cargo feature for encoding_rs though the encoding_c and encoding_glue intermediate crates; the name 'simd-accel' is for consistency with the Rust regex crate).

 2) make sure there's an analysis job on CI that runs without --enable-rust-simd to make unconditional use of nightly Rust features orange

 3) handle potential other conditional additions of nightly feature use as a matter of build system and gkrust review.
Blocks: 1361342
Another use case for this is to enable a clippy build variant (which requires nightly rust).
(In reply to Henri Sivonen (:hsivonen) from comment #14)

> When crates are unable to use feature gate unconditionally, the can't use
> them in secret in the sense that they need to be conditional on cargo
> features and cargo features need to be chained to the cargo features of
> gkrust, where they are easily visible in review.

Ok. I remembered you saying you didn't want encoding.rs to have a non-simd variant. If you're willing to support conditional compilation, I think this will work fine as long as we have some alt-build or lint task on integration to ensure we compile without nightly features.
No longer blocks: 1361342
(Reporter)

Comment 17

5 months ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #16)
> (In reply to Henri Sivonen (:hsivonen) from comment #14)
> 
> > When crates are unable to use feature gate unconditionally, the can't use
> > them in secret in the sense that they need to be conditional on cargo
> > features and cargo features need to be chained to the cargo features of
> > gkrust, where they are easily visible in review.
> 
> Ok. I remembered you saying you didn't want encoding.rs to have a non-simd
> variant. If you're willing to support conditional compilation, I think this
> will work fine as long as we have some alt-build or lint task on integration
> to ensure we compile without nightly features.

encoding_rs supports conditional compilation and will continue to do so, since I'm not going to write SIMD code for all architectures even though I made the ALU code work even in the big-endian 32-bit and 64-bit cases. I think I've previously said that I'd like it to be the case in the future that if someone is compiling encoding_rs for x86_64, which always support SSE2, they'd get the SSE2 code compiled in automatically instead of having to think about asking for it (--features simd-accel).
You need to log in before you can comment on or make changes to this bug.