Closed Bug 1319228 Opened 7 years ago Closed 5 years ago

Handle rust in the tup backend

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: mshal, Assigned: chmanchester)

References

Details

Attachments

(9 files, 4 obsolete files)

Bug 1319228 - Generate rules for rust in the Tup backend via cargo --build-plan.
59 bytes, text/x-review-board-request
Details
Bug 1319228 - Link libxul in the Tup backend.
59 bytes, text/x-review-board-request
Details
Bug 1319228 - Build dependentlibs.list in the tup backend, create a group for shared libraries to be used as its input.
59 bytes, text/x-review-board-request
Details
Bug 1319228 - Build tup with nightly rust in automation.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1319228 - Disable the js shell in tup automation builds.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1319228 - Use pass_thru=True when running config.status to expose error messages from sub-commands.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1319228 - Generate rules for rust in the Tup backend via cargo --build-plan.
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1319228 - Link libxul in the Tup backend.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1319228 - Build dependentlibs.list in the tup backend, create a group for shared libraries to be used as its input.
59 bytes, text/x-review-board-request
mshal
: review+
Details 
I'm not sure what this will involve, but I'm filing a bug for it now since it's something we'll need to handle. This could have tup invoking rustc / cargo, or perhaps just invoking cargo completely independently.
We've accumulated a good bit of logic related to building a cargo command line in config/rules.mk that I expect we'll need to pull in to moz.build as with other compilation related things.
Product: Core → Firefox Build System
Hi Mike, I'm just going through our bug backlog and trying to clean up.  Is this bug still relevant given your recent work?
Flags: needinfo?(mshal)
Yeah, there is still some work to be done to integrate cargo/rustc into tup. We'll have to land the --build-plan work (https://github.com/rust-lang/cargo/pull/4357) and filesystem probing fix (https://github.com/rust-lang/rust/issues/46816). This bug can track work related to actually integrating 'cargo --build-plan' into moz.build and exposing the rustc commands to the backends in mozbuild.
Flags: needinfo?(mshal)
Assignee: nobody → cmanchester
Attachment #8979785 - Attachment is obsolete: true
Attachment #8979785 - Flags: review?(mshal)
Attachment #8979786 - Attachment is obsolete: true
Attachment #8979786 - Flags: review?(mshal)
Attachment #8979787 - Attachment is obsolete: true
Attachment #8979787 - Flags: review?(mshal)
Attachment #8979791 - Flags: review?(mshal)
Attachment #8979792 - Flags: review?(mshal)
Attachment #8979793 - Flags: review?(mshal)
Attachment #8979794 - Flags: review?(mshal)
Attachment #8979795 - Flags: review?(mshal)
Attachment #8979796 - Flags: review?(mshal)
Sorry about the flag spam, I seem to have triggered some sort of race in mozreview's UI.
Comment on attachment 8979791 [details]
Bug 1319228 - Build tup with nightly rust in automation.

https://reviewboard.mozilla.org/r/245934/#review253036

::: taskcluster/ci/toolchain/linux.yml:415
(Diff revision 1)
> +    description: "rust nightly repack"
> +    treeherder:
> +        kind: build
> +        platform: toolchains/opt
> +        symbol: TL(rust-nightly)
> +        tier: 1

Should this be tier-2 since the tup job that requires it is tier-2?
Attachment #8979791 - Flags: review?(mshal) → review+
Comment on attachment 8979792 [details]
Bug 1319228 - Disable the js shell in tup automation builds.

https://reviewboard.mozilla.org/r/245936/#review253038
Attachment #8979792 - Flags: review?(mshal) → review+
Comment on attachment 8979793 [details]
Bug 1319228 - Use pass_thru=True when running config.status to expose error messages from sub-commands.

https://reviewboard.mozilla.org/r/245938/#review253040
Attachment #8979793 - Flags: review?(mshal) → review+
Comment on attachment 8979791 [details]
Bug 1319228 - Build tup with nightly rust in automation.

https://reviewboard.mozilla.org/r/245934/#review253036

> Should this be tier-2 since the tup job that requires it is tier-2?

I think we can roll the dice with Tier-1 here. If the rust re-pack script breaks in some way I expect we'd need to fix it right away anyway.
Comment on attachment 8979794 [details]
Bug 1319228 - Generate rules for rust in the Tup backend via cargo --build-plan.

https://reviewboard.mozilla.org/r/245940/#review253042

It feels like a bunch of the logic in tup.py should go into the frontend (like as a cargo_reader.py or some such?), but we don't need to block this bug on that. It can be tackled as a followup for when we want to try another build backend.

Also tagging ted for review since much of the JSON processing code came from my initial test script.

::: python/mozbuild/mozbuild/backend/tup.py:699
(Diff revision 1)
> +            output_str = ''
> +            if invocation['outputs']:
> +                output_str = ' -> %s' % ' '.join([os.path.basename(f)
> +                                                  for f in invocation['outputs']])
> +            return '%s %s v%s %s%s' % (
> +                os.path.basename(invocation['program']).upper(),

I guess I didn't realize it when I moved this stuff around, but os.path.basename(invocation['program']).upper() is basically redundant with the header = 'RUSTC' logic below. This makes it print "RUSTC RUSTC ..." for compilation lines and "RUN BUILD-SCRIPT-BUILD" for run scripts. I think "RUN" is a little more descriptive that "BUILD-SCRIPT-BUILD", so we should remove this invocation['program'] part and keep the header part IMO, so that we have "RUSTC ..." and "RUN ..." as the two types of commands.

::: python/mozbuild/mozbuild/backend/tup.py:706
(Diff revision 1)
> +                invocation['package_version'],
> +                invocation['kind'],
> +                output_str,
> +            )
> +
> +        def shell_quote(s):

I think we could re-use shellutil.py's quote now that this is in tree rather than a stand-alone script, and do the dep-info removal separately.

::: python/mozbuild/mozbuild/backend/tup.py:708
(Diff revision 1)
> +                output_str,
> +            )
> +
> +        def shell_quote(s):
> +            x = "'%s'" % s.replace("'", "'\"'\"'") if '"' in s or ' ' in s else s
> +            # TODO HACKS:

I realize this came from my original hacky script, but we should probably have a more sensible comment here about removing dep-info because it's specific to the make backend, and is hard-coded in cargo.

::: python/mozbuild/mozbuild/backend/tup.py:795
(Diff revision 1)
> +            line_handler=accumulate_output,
> +            explicit_env=cargo_env)
> +
> +        cargo_plan = json.loads(''.join(output_lines))
> +        self._gen_cargo_rules(backend_file, cargo_plan, cargo_env)
> +

We'll also want to add cargo_plan['inputs'] to self.backend_input_files, so that we can regenerate the build plan if one of the Cargo.toml files changes.
Attachment #8979794 - Flags: review?(mshal)
Attachment #8979794 - Flags: review?(ted)
Comment on attachment 8979795 [details]
Bug 1319228 - Link libxul in the Tup backend.

https://reviewboard.mozilla.org/r/245942/#review253094

Awesome work! Amazing to see we can get all the way to linking libxul now.
Attachment #8979795 - Flags: review?(mshal) → review+
Comment on attachment 8979796 [details]
Bug 1319228 - Build dependentlibs.list in the tup backend, create a group for shared libraries to be used as its input.

https://reviewboard.mozilla.org/r/245944/#review253098

::: toolkit/library/dependentlibs.py:131
(Diff revision 1)
>          ext = os.path.splitext(lib)[1]
>          assert(ext == '.dll')
>          func = dependentlibs_dumpbin
>  
>      deps = dependentlibs(lib, libpaths, func)
> -    deps[lib] = mozpath.join(libpaths[0], lib)
> +    base_lib = mozpath.basename(lib)

Huh, looks like we need this because we don't pretty_path the inputs like the recursive make backend does? I'm surprised that's the only change we needed (that I've noticed so far, anyway). Would using pretty_path buy us anything else in the tup backend?
Attachment #8979796 - Flags: review?(mshal) → review+
Comment on attachment 8979794 [details]
Bug 1319228 - Generate rules for rust in the Tup backend via cargo --build-plan.

https://reviewboard.mozilla.org/r/245940/#review257092

::: python/mozbuild/mozbuild/backend/tup.py:625
(Diff revision 2)
>              tup = self.environment.substs.get('TUP', 'tup')
>              self._cmd.run_process(cwd=self.environment.topsrcdir, log_name='tup', args=[tup, 'init'])
>  
> +
> +    def _get_cargo_flags(self, obj):
> +        cargo_flags = ['--build-plan', '-Z', 'unstable-options']

As discussed in person, I'd like to see a followup about consolidating the logic here with the logic in rules.mk around invoking cargo.

::: python/mozbuild/mozbuild/backend/tup.py:658
(Diff revision 2)
> +            'PYTHON': self.environment.substs['PYTHON'],
> +            'PYTHONDONTWRITEBYTECODE': '1',
> +        }
> +        cargo_incremental = self.environment.substs.get('CARGO_INCREMENTAL')
> +        if cargo_incremental is not None:
> +            # TODO: CARGO_INCREMENTAL produces outputs that Tup doesn't know

Please file a bug on this and include the bug number here.

::: python/mozbuild/mozbuild/backend/tup.py:694
(Diff revision 2)
> +        def display_name(invocation):
> +            output_str = ''
> +            if invocation['outputs']:
> +                output_str = ' -> %s' % ' '.join([os.path.basename(f)
> +                                                  for f in invocation['outputs']])
> +            return '%s v%s %s%s' % (

This might be easier to read using `.format` and named keys.

::: python/mozbuild/mozbuild/backend/tup.py:715
(Diff revision 2)
> +            for dep in invocation['deps']:
> +                # We'd expect to just handle dependencies transitively (so use
> +                # invocations[dep]['outputs'] here, but because the weird host dependencies
> +                # sometimes get used in the final library and not intermediate
> +                # libraries, tup doesn't work well with them. So build up the full set
> +                # of intermediate dependencies with 'full-deps'

I'd like to hear more about what's going on here, but we can discuss that after the fact.

::: python/mozbuild/mozbuild/backend/tup.py:729
(Diff revision 2)
> +            envvars = invocation.get('env')
> +            for k, v in itertools.chain(cargo_env.iteritems(),
> +                                        envvars.iteritems()):
> +                command.append("%s=%s" % (k, cargo_quote(v)))
> +            command.append(invocation['program'])
> +            command.extend(cargo_quote(a.replace('dep-info,', ''))

You're overriding cargo's dep-info output here because tup is able to get this info by monitoring the process, right? This feels a bit hacky, it would be nicer if we could ask cargo to leave this out in the first place.

::: python/mozbuild/mozbuild/backend/tup.py:732
(Diff revision 2)
> +                command.append("%s=%s" % (k, cargo_quote(v)))
> +            command.append(invocation['program'])
> +            command.extend(cargo_quote(a.replace('dep-info,', ''))
> +                           for a in invocation['args'])
> +            outputs = invocation['outputs']
> +            if os.path.basename(invocation['program']) == 'build-script-build':

Obviously longer-term we need to figure out a more workable solution here.

::: python/mozbuild/mozbuild/backend/tup.py:736
(Diff revision 2)
> +            outputs = invocation['outputs']
> +            if os.path.basename(invocation['program']) == 'build-script-build':
> +                for output in cargo_extra_outputs.get(shortname, []):
> +                    outputs.append(os.path.join(invocation['env']['OUT_DIR'], output))
> +
> +            if (invocation['target_kind'][0] == 'custom-build' and

I ran into this same confusing "building the build script" vs. "executing the build script" thing in my scripts for generating charts. It would be nice if cargo had a way to differentiate the two in the build plan output.

::: python/mozbuild/mozbuild/backend/tup.py:740
(Diff revision 2)
> +
> +            if (invocation['target_kind'][0] == 'custom-build' and
> +                os.path.basename(invocation['program']) == 'rustc'):
> +                flags = cargo_extra_flags.get(shortname, [])
> +                for flag in flags:
> +                    command.append(flag % {'libloading_outdir': get_libloading_outdir()})

Is it not possible for tup to parse the output of the build script like cargo does to influence downstream build rules?

::: python/mozbuild/mozbuild/backend/tup.py:748
(Diff revision 2)
> +                header = 'RUSTC'
> +            else:
> +                inputs.add(invocation['program'])
> +                header = 'RUN'
> +
> +            invocation['full-deps'] = set(inputs)

The `set()` here is just to make a copy, right?

::: python/mozbuild/mozbuild/backend/tup.py:753
(Diff revision 2)
> +            invocation['full-deps'] = set(inputs)
> +            invocation['full-deps'].update(invocation['outputs'])
> +
> +            backend_file.rule(
> +                command,
> +                inputs=list(inputs),

Is this going to output the inputs in an arbitrary order because they're being passed through a `set`?

::: python/mozbuild/mozbuild/backend/tup.py:768
(Diff revision 2)
> +        for val in enumerate(invocations):
> +            _process(*val)
> +
> +
> +    def _gen_rust(self, backend_file):
> +        # TODO: The gtest rust library depends on many of the same libraries

Please file a bug for this as well and put the bug number here.

::: python/mozbuild/mozbuild/backend/tup.py:781
(Diff revision 2)
> +
> +        output_lines = []
> +        def accumulate_output(line):
> +            output_lines.append(line)
> +
> +        cargo_status = self._cmd.run_process(

Seems like it might be simpler to just use `subprocess.check_output` here unless there's something else useful that `run_process` is doing that I'm unaware of.
Attachment #8979794 - Flags: review?(ted) → review+
Comment on attachment 8979794 [details]
Bug 1319228 - Generate rules for rust in the Tup backend via cargo --build-plan.

https://reviewboard.mozilla.org/r/245940/#review257092

> As discussed in person, I'd like to see a followup about consolidating the logic here with the logic in rules.mk around invoking cargo.

Filed bug 1468546.

> Please file a bug on this and include the bug number here.

Filed bug 1468527.

> Is it not possible for tup to parse the output of the build script like cargo does to influence downstream build rules?

mshal might have a better idea here, but I guess we could use a python wrapper script to parse the output and feed that to dependencies to generalize this in the future.

> Is this going to output the inputs in an arbitrary order because they're being passed through a `set`?

I'll sort them for determinism, although it wont matter to tup.

> Please file a bug for this as well and put the bug number here.

Filed bug 1468547
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f3fa38b1a5f
Build tup with nightly rust in automation. r=mshal
https://hg.mozilla.org/integration/autoland/rev/d3c835477d11
Disable the js shell in tup automation builds. r=mshal
https://hg.mozilla.org/integration/autoland/rev/a218f97e1b48
Use pass_thru=True when running config.status to expose error messages from sub-commands. r=mshal
https://hg.mozilla.org/integration/autoland/rev/badf116dde30
Generate rules for rust in the Tup backend via cargo --build-plan. r=ted
https://hg.mozilla.org/integration/autoland/rev/6ba05238789f
Link libxul in the Tup backend. r=mshal
https://hg.mozilla.org/integration/autoland/rev/2eedbab9137b
Build dependentlibs.list in the tup backend, create a group for shared libraries to be used as its input. r=mshal
Backed out 6 changesets (bug 1319228) for Btup bustages on Linux x64 on a CLOSED TREE
Problematic push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2eedbab9137b6b11fa5eba3faef9fb46dcf4984f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=183082484
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f513c7af73e6cfac90e10be46d255a71e45b339c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=183082484&repo=autoland&lineNumber=4443

[task 2018-06-13T21:36:01.006Z] 21:36:01    ERROR - Return code: 1
[task 2018-06-13T21:36:01.008Z] 21:36:01  WARNING - setting return code to 2
[task 2018-06-13T21:36:01.008Z] 21:36:01    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-06-13T21:36:01.008Z] 21:36:01    FATAL - Running post_fatal callback...
[task 2018-06-13T21:36:01.008Z] 21:36:01    FATAL - Exiting -1
[task 2018-06-13T21:36:01.008Z] 21:36:01     INFO - [mozharness: 2018-06-13 21:36:01.008812Z] Finished build step (failed)
[task 2018-06-13T21:36:01.008Z] 21:36:01     INFO - Running post-run listener: _summarize
[task 2018-06-13T21:36:01.009Z] 21:36:01    ERROR - # TBPL FAILURE #
[task 2018-06-13T21:36:01.009Z] 21:36:01     INFO - [mozharness: 2018-06-13 21:36:01.009083Z] FxDesktopBuild summary:
[task 2018-06-13T21:36:01.009Z] 21:36:01    ERROR - # TBPL FAILURE #
Flags: needinfo?(cmanchester)
Bug 1465709 seems to have broken this. I'm looking into it.
Flags: needinfo?(cmanchester)
Comment on attachment 8985275 [details]
Bug 1319228 - Toggle allocator_api feature when building with rustc 1.28

https://reviewboard.mozilla.org/r/250884/#review257174

You're probably using a nightly that is too old.
Attachment #8985275 - Flags: review?(mh+mozilla) → review-
I doubt this change would build anyways, you should just update to last nightly.
I was using a nightly that was too old, but this patch builds and makes the difference on try (although I got the fix from the error message, I don't have context on this code):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78fd8d79901023c3bcf021b2b96823d7f165270d

vs.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=314ea0155ef569b2a80c4d654cee045ea26e4f80
Flags: needinfo?(mh+mozilla)
But the patch will break building with the latest nightly, and, down the line, with 1.28 when it reaches beta and then is released.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #55)
> But the patch will break building with the latest nightly, and, down the
> line, with 1.28 when it reaches beta and then is released.

Both builds from comment 54 are using nightly-2018-06-12.
That's one day too early. (note that strangely, if you rustup install nightly-2018-06-12, the nightly you get says 2018-06-11 in its --version)
Attachment #8985275 - Attachment is obsolete: true
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afe3be13ea43
Build tup with nightly rust in automation. r=mshal
https://hg.mozilla.org/integration/autoland/rev/13546194edc3
Disable the js shell in tup automation builds. r=mshal
https://hg.mozilla.org/integration/autoland/rev/111332735c82
Use pass_thru=True when running config.status to expose error messages from sub-commands. r=mshal
https://hg.mozilla.org/integration/autoland/rev/70d4cf842172
Generate rules for rust in the Tup backend via cargo --build-plan. r=ted
https://hg.mozilla.org/integration/autoland/rev/2835a57d03db
Link libxul in the Tup backend. r=mshal
https://hg.mozilla.org/integration/autoland/rev/f7a62638a91a
Build dependentlibs.list in the tup backend, create a group for shared libraries to be used as its input. r=mshal
Depends on: 1469242
You need to log in before you can comment on or make changes to this bug.