1444141 - add a Rust dependency to the JS engine

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

This change ensures that the JS engine can start hooking into Rust
things via moz.build, instead of the current "manually invoke cargo"
recommendtations that it has today.  This change *does not* make the JS
engine require Rust/cargo in any way.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: move Rust configury bits to a more central location

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: move Rust configury bits to a more central location

Here, let's review a patch that is much closer to compiling.

Comment 3

[Nathan Froyd [:froydnj]]

OK, so of course things start failing on automation.  First it was the hazard builds, which were an easy fix:

https://hg.mozilla.org/try/rev/37b6d10be0537822e7adc9f1b47bc528ac9285e6

(I don't exactly understand how the browser builds were working before without that bit, but OK.)

Next is the spidermonkey Windows tests, and this is what I don't understand.  The configure process says that it can't find Rust, which is fine: the jobs don't explicitly depend on the Rust toolchain.  But then adding the Rust toolchain--which should make autospider.py able to find it--leads to the same result:

https://hg.mozilla.org/try/rev/9ade68f601dc088ea7d9ca25cd2577523733fd51
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ade68f601dc088ea7d9ca25cd2577523733fd51

sfink, do you have ideas on what's going wrong here?

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8957246 [details] [diff] [review]
move Rust configury bits to a more central location

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

::: moz.configure
@@ +141,5 @@
>          when='--enable-compile-environment')
>  include('build/moz.configure/flags.configure',
>          when='--enable-compile-environment')
> +include('build/moz.configure/rust.configure',
> +        when='--enable-compile-environment')

Please move this to js/moz.configure and change the options to js_options

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #4)
> ::: moz.configure
> @@ +141,5 @@
> >          when='--enable-compile-environment')
> >  include('build/moz.configure/flags.configure',
> >          when='--enable-compile-environment')
> > +include('build/moz.configure/rust.configure',
> > +        when='--enable-compile-environment')
> 
> Please move this to js/moz.configure and change the options to js_options

What is the gain from having it in js/moz.configure vs. the toplevel moz.configure?  Rust seems rather toolchain-y, and toolchain.configure is included from the toplevel moz.configure, so...

Comment 6

[Mike Hommey [:glandium]]

I don't want e.g. --enable-project=memory to start requiring rust before I actually land rust code under memory/

Comment 7

[Steve Fink [:sfink] [:s:]]

I didn't look closely at the log or your patch, but my first guess would be https://searchfox.org/mozilla-central/source/js/src/devtools/automation/autospider.py#241 ?

    rust_dir = os.path.join(DIR.tooltool, 'rustc')
    if os.path.exists(os.path.join(rust_dir, 'bin', 'rustc')):
        env.setdefault('RUSTC', os.path.join(rust_dir, 'bin', 'rustc'))
        env.setdefault('CARGO', os.path.join(rust_dir, 'bin', 'cargo'))
    else:
        env.setdefault('RUSTC', 'rustc')
        env.setdefault('CARGO', 'cargo')

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #6)
> I don't want e.g. --enable-project=memory to start requiring rust before I
> actually land rust code under memory/

That's fair, I guess.  But putting an additional include in js/moz.configure doesn't work:

  File "/home/froydnj/src/gecko-dev.git/python/mozbuild/mozbuild/configure/__init__.py", line 694, in include_impl
    self.include_file(what)
  File "/home/froydnj/src/gecko-dev.git/python/mozbuild/mozbuild/configure/__init__.py", line 382, in include_file
    'Cannot include `%s` because it was included already.' % path)

Comment 9

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > I don't want e.g. --enable-project=memory to start requiring rust before I
> > actually land rust code under memory/
> 
> That's fair, I guess.  But putting an additional include in js/moz.configure
> doesn't work:

Why additional?

Comment 10

[Nathan Froyd [:froydnj]]

Changing this bug to reflect a new plan; jorendorff approved over IRC, and I'll r? him on the necessary patches.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 1 - export RUSTC and CARGO variables for the hazard analysis build

configure needs these environment variables to find rustc and cargo.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 2 - make some spidermonkey tasks depend on rust

Spidermonkey tasks have not had to depend on Rust before, but since
we're about to add a dependency on Rust to the JS engine, we need Rust
binaries.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 3 - fix exports of PATH in sm-tooltool-config.sh

TOOLTOOL_CHECKOUT is typically `.`, which doesn't work so great for
adding things to $PATH.  We need to turn everything we're adding to
$PATH into absolute paths, so $PATH actually works properly.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 4 - make the JS engine depend on Rust

The Rust dependency in Firefox has been limited to Firefox builds by
virtue of having the Rust check in a Firefox-specific location,
toolkit/moz.configure.  For JS to start depending on Rust, we need to
move that check to a location where a standalone JS engine build will
pick up the Rust check.  The toplevel moz.configure seems like the best
place for that.

Note that this is an *unconditional* dependency: we're not compiling Rust code
in the JS engine yet, and even if we add some Rust code, it may only be
compiled on certain channels or certain architectures.  Given that Firefox
already depends on Rust, I don't see this as a large burden.

I couldn't see how to make rust.configure only fire for Firefox and JS, as per
comment 4.  If we want to insert project checks somehow for Rust, I suppose we
could do that?

Comment 15

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8994596 [details] [diff] [review]
part 3 - fix exports of PATH in sm-tooltool-config.sh

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

::: taskcluster/scripts/builder/sm-tooltool-config.sh
@@ +65,5 @@
>  for bin in $TOOLTOOL_CHECKOUT/*/bin $TOOLTOOL_CHECKOUT/VC/bin/Hostx64/x86; do
> +    if [ ! -d "$bin" ]; then
> +        continue
> +    fi
> +    absbin=$(cd "$bin" && pwd)

This is something of "cross your fingers and hope it works everywhere we care about", but it's fine with me.

Though as long as we're pretending to care about paths with spaces, this probably ought to be

  absbin="$(cd "$bin" && pwd)"

horrific as that looks.

(Adding */bin to $PATH is... not exactly principled in the first place. We probably ought to have an activate.sh script with each tooltool package. Or... something. But that's some tougher yak hair than you'd want to tackle here.)

Comment 16

[Steve Fink [:sfink] [:s:]]

I don't know anything about it, which is why I hacked it, but could

https://searchfox.org/mozilla-central/source/js/src/devtools/automation/autospider.py#426

be improved / go away as part of this?

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Steve Fink [:sfink] [:s:] from comment #16)
> I don't know anything about it, which is why I hacked it, but could
> 
> https://searchfox.org/mozilla-central/source/js/src/devtools/automation/
> autospider.py#426
> 
> be improved / go away as part of this?

Given that those variables are fake, but provided to make symbol machinery run smoothly, and given that we now produce real values for those when building the JS engine, I think we could get rid of those environment variables entirely?  (Though AFAICS, the accesses to them from symbol code are in Python through buildconfig.substs, and not through environment variables, so I'm not entirely sure that code does what you think it might be doing...)

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8994597 [details] [diff] [review]
part 4 - make the JS engine depend on Rust

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

::: js/moz.configure
@@ +460,5 @@
>  
>  set_config('ENABLE_PIPELINE_OPERATOR', enable_pipeline_operator)
>  set_define('ENABLE_PIPELINE_OPERATOR', enable_pipeline_operator)
>  
>  

Stray whitespace change?

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8994597 [details] [diff] [review]
part 4 - make the JS engine depend on Rust

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

See comment 4.

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #19)
> See comment 4.

Like I said in comment 14, I don't see how to make this Firefox/JS only, since the js_option route mentioned in comment 4 doesn't seem applicable.  I don't think we should hold up this patch just so a project that's not built in automation can continue to work; follow-up patches seem more appropriate.

Comment 21

[Mike Hommey [:glandium]]

> Like I said in comment 14, I don't see how to make this Firefox/JS only

By doing literally what's written in comment 4.

Comment 22

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #21)
> > Like I said in comment 14, I don't see how to make this Firefox/JS only
> 
> By doing literally what's written in comment 4.

What options are supposed to be js_options?  There are no `option` calls being touched in that patch, and the only `option` calls in rust.configure are for WIN64_{LINK,LIB}.  Are you saying that RUSTC/CARO/RUSTDOC/RUSTFMT should be turned into js_option?  It's not obvious to me that will DTRT with the checks in `rust_compiler`...

Comment 23

[Mike Hommey [:glandium]]

Yes, all of them should be js_options. Otherwise, something like `configure CARGO=/foo/bar/cargo` will only set CARGO for top-level and not js.

Comment 24

[Nathan Froyd [:froydnj]]

The js_optionizing of rust.configure is being handled in bug 1478655.

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8994597 [details] [diff] [review]
part 4 - make the JS engine depend on Rust

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

Given that we're js_option-izing things in bug 1478986, is this now OK to land?

Comment 26

[Mike Hommey [:glandium]]

comment 4 still applies.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: part 4 - make the JS engine depend on Rust

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb730dc5b375
part 1 - export RUSTC and CARGO variables for the hazard analysis build; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d0193f2bd3
part 2 - make some spidermonkey tasks depend on rust; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0285701a15
part 3 - fix exports of PATH in sm-tooltool-config.sh; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f22d19eb69
part 4 - make the JS engine depend on Rust; r=chmanchester,glandium,jorendorff

Comment 29

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bb730dc5b375
https://hg.mozilla.org/mozilla-central/rev/74d0193f2bd3
https://hg.mozilla.org/mozilla-central/rev/6c0285701a15
https://hg.mozilla.org/mozilla-central/rev/e1f22d19eb69