Bugzilla

Tiberius Oros[:tiberius_oros]

Assignee

Comment 2

•

6 years ago

It looks like the jsrust crate wasn't being built before and now is being built, as of bug 1490948. However, for some reason it's doing the build without the RUSTC_BOOTSTRAP=1 flag from the mozconfig [1]. It is picking up the RUSTFLAGS=-Zsave-analysis on the next line, and trying to use -Zsave-analysis without RUSTC_BOOTSTRAP=1 is failing. Benjamin, do you know where this env command is coming from and why it's missing the RUSTC_BOOTSTRAP=1 flag? For comparison see the previous passing build log [2] (grep for -Zsave-analysis and you'll see how the env includes RUSTC_BOOTSTRAP=1)

[1] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/browser/config/mozconfigs/linux64/debug-searchfox-clang#17
[2] https://taskcluster-artifacts.net/dMHYPVdKRhySnajH2Nd_4g/0/public/logs/live_backing.log

Blocks: 1490948

Flags: needinfo?(kats) → needinfo?(bbouvier)

Priority: P5 → P1

Updated

•

6 years ago

Comment 4

•

6 years ago

Never heard of this compilation option before, maybe Ted/Nathan who discussed it in bug 1495012 know a bit more?

Flags: needinfo?(ted)

Flags: needinfo?(nfroyd)

Flags: needinfo?(bbouvier)

Comment 5

•

6 years ago

MOZ_RUST_SIMD is only set for Gecko builds:

https://searchfox.org/mozilla-central/source/toolkit/moz.configure#653

I guess we should move that to someplace JS can see it, like build/moz.configure/rust.configure, so that the JS code being built will unlock Nightly features as well.

Flags: needinfo?(ted)

Flags: needinfo?(nfroyd)

Assignee

Comment 6

•

6 years ago

I think this is separate from MOZ_RUST_SIMD; for the searchfox builds we're setting `export RUSTC_BOOTSTRAP=1` directly in the mozconfig so it should be applied to the environment for everything that gets built, no?

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/browser/config/mozconfigs/linux64/debug-searchfox-clang#17

Comment 7

•

6 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> I think this is separate from MOZ_RUST_SIMD; for the searchfox builds we're
> setting `export RUSTC_BOOTSTRAP=1` directly in the mozconfig so it should be
> applied to the environment for everything that gets built, no?
> 
> https://searchfox.org/mozilla-central/rev/
> 819cd31a93fd50b7167979607371878c4d6f18e8/browser/config/mozconfigs/linux64/
> debug-searchfox-clang#17

Indeed, I missed that part, thanks for pointing it out.

It's not clear to me how the export for RUSTFLAGS is making its way into the construction of the `cargo` command, but the RUSTC_BOOTSTRAP export does not also filter down into the invocation of the `cargo` command.  Does cargo scrub the environment somehow?...but then why would it scrub RUSTC_BOOTSTRAP in this case but not in our "normal" usage?

Assignee

Comment 8

•

6 years ago

Oh, maybe the RUSTC_BOOTSTRAP from the mozconfig *never* propagates into the cargo command invocation, but this was masked by the fact that all other crates use the MOZ_RUST_SIMD thing? Note in particular that the previous passing log [1] has this command:

env   RUSTC_BOOTSTRAP=1  CARGO_TARGET_DIR=/builds/worker/workspace/build/src/obj-firefox/toolkit/library RUSTFLAGS='-C opt-level=2 -C debuginfo=2  -Zsave-analysis' RUSTC= ...

If the RUSTC_BOOTSTRAP=1 was just in the environment from the mozconfig, it wouldn't get printed here as part of the env command. The fact that it's getting printed here means it's coming from the rust_unlock_unstable at [2]. So maybe we should modify that rust_unlock_unstable setter to also propagate it from the environment if it's present?

[1] https://taskcluster-artifacts.net/dMHYPVdKRhySnajH2Nd_4g/0/public/logs/live_backing.log
[2] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/config/rules.mk#957

Assignee

Comment 9

•

6 years ago

Presumably the RUSTFLAGS is getting propagated down via [1], so maybe another option is to do the same for RUSTC_BOOTSTRAP?

[1] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/build/moz.configure/toolchain.configure#1574

Assignee

Comment 10

•

6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&bugfiler=&group_state=expanded&revision=9ec274cbca5a79f56352efee2e35c2ec3e23fef0

(not currently active) Ted Mielczarek

Assignee

Comment 11

•

6 years ago

Attached patch Propagate RUSTC_BOOTSTRAP from the environment (obsolete) — Details — Splinter Review

Assignee: nobody → kats

Attachment #9013314 - Flags: review?(nfroyd)

Comment 12

•

6 years ago

FWIW, this failure is basically the same as bug 1495012. See bug 1495012 comment 6 for an explanation.

Assignee

Comment 13

•

6 years ago

Here's a green try push with regular builds I did to make sure the patch didn't break non-searchfox builds: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=7b4adaad8da22acf1a0f82ecd05c713a2c244e37

Assignee

Comment 14

•

6 years ago

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #12)
> FWIW, this failure is basically the same as bug 1495012. See bug 1495012
> comment 6 for an explanation.

With respect to this, the "use nightly" solution is less desirable for searchfox since we're planning on having the code indexing also index the rust stdlib and it would be good to have that stdlib be the version of rust we're using in production (i.e. stable, not nightly). But if you *really* want we could use nightly... it's what I had originally planned on doing (see bug 1432475 comment 9 onwards for more context).

Comment 15

•

6 years ago

Comment on attachment 9013314 [details] [diff] [review]
Propagate RUSTC_BOOTSTRAP from the environment

Review of attachment 9013314 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +1579,5 @@
>  
> +option(env='RUSTC_BOOTSTRAP',
> +       nargs=1,
> +       help='Rust bootstrap override')
> +set_config('RUSTC_BOOTSTRAP', depends('RUSTC_BOOTSTRAP')(lambda flags: flags))

I think the better way to do this is to make --enable-rust-simd (which is already turned on for automation builds) a js_option, which should turn on RUSTC_BOOTSTRAP for JS crates as well as Gecko ones.  Please try that instead?

Attachment #9013314 - Flags: review?(nfroyd)

Assignee

Comment 16

•

6 years ago

Try push with that change: https://treeherder.mozilla.org/#/jobs?repo=try&bugfiler=&group_state=expanded&revision=eefe2f7e5b7067d8acc5fe5009b9f8dc9b29a3a0

Assignee

Comment 17

•

6 years ago

That ran into a configure error, but I don't know how to resolve it. Any help would be appreciated.

Assignee

Comment 18

•

6 years ago

Oh, maybe I need to move it into the root moz.configure instead of the one in toolkit/. That seems to help locally.

Try with that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a93b6f54054121d6592a908c77e834e242d714b

Assignee

Comment 19

•

6 years ago

Attached patch Make it a js_option (obsolete) — Details — Splinter Review

Attachment #9013314 - Attachment is obsolete: true

Attachment #9013456 - Flags: review?(nfroyd)

Comment 20

•

6 years ago

Comment on attachment 9013456 [details] [diff] [review]
Make it a js_option

Review of attachment 9013456 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this code in build/moz.configure/rust.configure instead of the toplevel moz.configure, please.  Thanks!

Attachment #9013456 - Flags: review?(nfroyd) → review+

Comment 21

•

6 years ago

Sorry for the runaround here. :(

Assignee

Comment 22

•

6 years ago

(In reply to Nathan Froyd [:froydnj] from comment #20)
> r=me with this code in build/moz.configure/rust.configure instead of the
> toplevel moz.configure, please.  Thanks!

Kicked off a try push with that just to make sure: https://treeherder.mozilla.org/#/jobs?repo=try&bugfiler=&group_state=expanded&revision=161c9c6385173e7e64face222a7c6d892fd18c2e

(In reply to Nathan Froyd [:froydnj] from comment #21)
> Sorry for the runaround here. :(

No worries, thanks for the quick reviews!

Pulsebot

Comment 23

•

6 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11df51f97f13
Make enable-rust-simd a JS option so JS crates get built with the RUSTC_BOOTSTRAP=1 flag in automation. r=froydnj

Narcis Beleuzu [:NarcisB]

Comment 24

•

6 years ago

(In reply to Nathan Froyd [:froydnj] from comment #15)
> Comment on attachment 9013314 [details] [diff] [review]
> Propagate RUSTC_BOOTSTRAP from the environment
> 
> Review of attachment 9013314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/moz.configure/toolchain.configure
> @@ +1579,5 @@
> >  
> > +option(env='RUSTC_BOOTSTRAP',
> > +       nargs=1,
> > +       help='Rust bootstrap override')
> > +set_config('RUSTC_BOOTSTRAP', depends('RUSTC_BOOTSTRAP')(lambda flags: flags))
> 
> I think the better way to do this is to make --enable-rust-simd (which is
> already turned on for automation builds) a js_option, which should turn on
> RUSTC_BOOTSTRAP for JS crates as well as Gecko ones.  Please try that
> instead?

Huh, I beg to differ. We should use RUSTC_BOOTSTRAP less, not more. searchfox is a corner case that may want to use everywhere, but it should do so by setting it globally through the environment.

Comment 25

•

6 years ago

Backed out for artifact build bustage

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=202766680&repo=mozilla-inbound&lineNumber=853

Flags: needinfo?(kats)

Pulsebot

Comment 26

•

6 years ago

Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4331c9d829
Backed out changeset 11df51f97f13 for artifact build bustage

Assignee

Updated

•

6 years ago

Flags: needinfo?(kats)

Assignee

Comment 27

•

6 years ago

Do you prefer the first patch I wrote? (attachment 9013314 [details] [diff] [review])

Flags: needinfo?(mh+mozilla)

Comment 28

•

6 years ago

Setting it globally on the task would be better.

Flags: needinfo?(mh+mozilla)

Assignee

Comment 29

•

6 years ago

Also FWIW the long term plan here for searchfox is to actually have a wrapper around rustc based on [1] that lets us get the save-analysis data from the stable compiler without having to use the bootstrap hack. I haven't figured out the build system well enough to attempt that yet.

[1] https://github.com/nrc/rls-rustc

Assignee

Comment 30

•

6 years ago

(In reply to Mike Hommey [:glandium] from comment #28)
> Setting it globally on the task would be better.

I'm not sure what that means exactly. How do you set something globally for a task if not via the mozconfig (which apparently doesn't work anyway)?

Comment 31

•

6 years ago

https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/build/linux.yml#45

Assignee

Comment 32

•

6 years ago

Thanks, I'll try that!

Comment 33

•

6 years ago

(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Nathan Froyd [:froydnj] from comment #15)
> > ::: build/moz.configure/toolchain.configure
> > @@ +1579,5 @@
> > >  
> > > +option(env='RUSTC_BOOTSTRAP',
> > > +       nargs=1,
> > > +       help='Rust bootstrap override')
> > > +set_config('RUSTC_BOOTSTRAP', depends('RUSTC_BOOTSTRAP')(lambda flags: flags))
> > 
> > I think the better way to do this is to make --enable-rust-simd (which is
> > already turned on for automation builds) a js_option, which should turn on
> > RUSTC_BOOTSTRAP for JS crates as well as Gecko ones.  Please try that
> > instead?
> 
> Huh, I beg to differ. We should use RUSTC_BOOTSTRAP less, not more.
> searchfox is a corner case that may want to use everywhere, but it should do
> so by setting it globally through the environment.

I thought the idea behind --enable-rust-simd is that we *don't* have to interact with RUSTC_BOOTSTRAP directly, since we are going behind rustc's back, in a sense.  --enable-rust-simd is there to keep RUSTC_BOOTSTRAP down somewhere where we invoke rustc, so that if we get a different interface for turning on Nightly features, we can use that instead.

--enable-rust-simd's usage has mutated a bit now that coverage (and searchfox) rely on -Z options, but I think the original idea is sound.

Assignee

Comment 34

•

6 years ago

(In reply to Nathan Froyd [:froydnj] from comment #33)
> --enable-rust-simd's usage has mutated a bit now that coverage (and
> searchfox) rely on -Z options, but I think the original idea is sound.

But at the same time it feels hacky to be relying on a SIMD flag for getting access to -Zsave-analysis, which has nothing to do with SIMD. It would be good to explicitly have the searchfox task opt in to RUSTC_BOOTSTRAP so that if, for example, the SIMD code became available without RUSTC_BOOTSTRAP, we wouldn't just remove that rustc_unlock_unstable bit and break everything else.

Try push with glandium's suggestion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=237670bfc51ef98648d21f9b34a981276f79ebeb

Comment 35

•

6 years ago

The original idea for --enable-rust-simd was to unlock usage of simd for encode-rs. It's never meant to be an unlock-all-unstable-features-everwhere thing, nor should it be. And since it's so tempting to abuse it, I'm more and more feeling like I should really finish that patch that I have to actually makes it unlock usage of simd for encode-rs.

Assignee

Comment 36

•

6 years ago

Oh, Python. Let's try that again:

https://treeherder.mozilla.org/#/jobs?repo=try&bugfiler=&group_state=expanded&revision=7c31564fa9c480307e6efd1666297507d83cb098

Assignee

Comment 37

•

6 years ago

Attached patch Move RUSTC_BOOTSTRAP to a place where it actually does something! — Details — Splinter Review

Attachment #9013456 - Attachment is obsolete: true

Attachment #9013583 - Flags: review?(nfroyd)