Closed Bug 1319228 Opened 9 years ago Closed 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: