Closed Bug 1444141 Opened 2 years ago Closed 2 years ago

add a Rust dependency to the JS engine

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

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.
Attachment #8957230 - Flags: review?(core-build-config-reviews)
Here, let's review a patch that is much closer to compiling.
Attachment #8957246 - Flags: review?(core-build-config-reviews)
Attachment #8957230 - Attachment is obsolete: true
Attachment #8957230 - Flags: review?(core-build-config-reviews)
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?
Flags: needinfo?(sphink)
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
Attachment #8957246 - Flags: review?(core-build-config-reviews)
(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...
Flags: needinfo?(mh+mozilla)
I don't want e.g. --enable-project=memory to start requiring rust before I actually land rust code under memory/
Flags: needinfo?(mh+mozilla)
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')
Flags: needinfo?(sphink)
(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)
Flags: needinfo?(mh+mozilla)
(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?
Flags: needinfo?(mh+mozilla)
Changing this bug to reflect a new plan; jorendorff approved over IRC, and I'll r? him on the necessary patches.
Blocks: 1469027
Summary: move Rust configury bits to a more central location → add a Rust dependency to the JS engine
configure needs these environment variables to find rustc and cargo.
Attachment #8994594 - Flags: review?(sphink)
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.
Attachment #8994595 - Flags: review?(sphink)
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.
Attachment #8994596 - Flags: review?(sphink)
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?
Attachment #8994597 - Flags: review?(jorendorff)
Attachment #8994597 - Flags: review?(core-build-config-reviews)
Attachment #8957246 - Attachment is obsolete: true
Attachment #8994594 - Flags: review?(sphink) → review+
Attachment #8994595 - Flags: review?(sphink) → review+
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.)
Attachment #8994596 - Flags: review?(sphink) → review+
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?
(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 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?
Attachment #8994597 - Flags: review?(core-build-config-reviews) → review+
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.
Attachment #8994597 - Flags: review+
(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.
Flags: needinfo?(mh+mozilla)
> 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.
Flags: needinfo?(mh+mozilla)
(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`...
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
The js_optionizing of rust.configure is being handled in bug 1478655.
Depends on: 1478655
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?
Attachment #8994597 - Flags: review?(mh+mozilla)
Attachment #8994597 - Flags: review?(jorendorff) → review+
comment 4 still applies.
Attachment #8994597 - Attachment is obsolete: true
Attachment #8996915 - Flags: review?(mh+mozilla)
Attachment #8996915 - Flags: review+
Attachment #8994597 - Flags: review?(mh+mozilla)
Attachment #8996915 - Flags: review?(mh+mozilla) → review+
Pushed by nfroyd@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.