1300835 - Distinguish between --enable-debug and --disable-optimize when building rust code in tree

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently rust code is only built with one of two configurations:

[profile.dev]
opt-level = 1
debug = true
rpath = false
lto = false
debug-assertions = true
codegen-units = 1
panic = "abort"

[profile.release]
opt-level = 2
debug = true
rpath = false
lto = true
debug-assertions = false
panic = "abort"

It would be better if we could distinguish between debug and disable optimize, such that we could build with `debug = true` and `opt-level = 0` when both are enabled, etc. This requires either more profiles than are currently supported by cargo (to my knowledge), or for us to generate the Cargo.toml file in our objdir for each toplevel crate based on the build options.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Alex, do you know if there is a good way to handle this within cargo, or will we have to work around it with mozbuild?

Comment 2

[Alex Crichton [:acrichto]]

Yeah unfortunately there's not a great way to do this in Cargo right now. We'd eventually like to support custom profiles, but that's yet to be designed/unimplemented.

I'd recommend living with two profiles for now, and otherwise the only solution I know of is to generate Cargo.toml on the fly.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Generate Cargo.toml files to seperate MOZ_OPTIMIZE from MOZ_DEBUG for rust code

This is my first take at unifying the rust build options for both Cargo.toml generation and rust prelinking into a single source of truth, and generating Cargo.toml files based on Cargo.toml.in files and that single source of truth.

Comment 4

[(not currently active) Ted Mielczarek]

This is going to conflict with my patch in bug 1298422 FYI. Can we put this in .cargo/config instead? That bug is generating a .cargo/config in the objdir.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> This is going to conflict with my patch in bug 1298422 FYI. Can we put this
> in .cargo/config instead? That bug is generating a .cargo/config in the
> objdir.

I'm not sure I'm completely clear as to how these conflict, other than that this patch will have to be modified to work with that patch (due to changes in how Cargo.lock works etc.). The [profile] keys which we specify here have to be specified in a Cargo.toml, as they cannot be specified in a .cargo/config file.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcb32c6aebdd719c1b00292647903bb42f61be2f

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8789020 [details] [diff] [review]
Generate Cargo.toml files to seperate MOZ_OPTIMIZE from MOZ_DEBUG for rust code

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

This is a reasonable start; I think we want to change some aspects of what's being done here.

::: config/expandlibs_exec.py
@@ +320,4 @@
>          ]
> +
> +        if RUST_BUILD_PROFILE['debug']:
> +            rustc_args += ["-g"]

We don't want this to be conditional; we want debug information turned on at all times.

::: config/gen_cargo_toml.py
@@ +10,5 @@
> +    if 'workspace' in package:
> +        package['workspace'] = os.path.join(options.srcdir, package['workspace'])
> +
> +def rewrite_lib(options, lib):
> +    lib['path'] = os.path.join(options.srcdir, lib.get('path', 'src/lib.rs'))

It seems prudent to check for |'path'| here, since 'path' comes with a reasonable default if it's not specified, right?

@@ +28,5 @@
> +    for key in ['dependencies', 'dev-dependencies', 'build-dependencies', 'replace']:
> +        rewrite_deps(options, parsed.get(key, {}))
> +    for target in parsed.get('target', {}).itervalues():
> +        rewrite_deps(options, target.get('dependencies', {}))
> +    rewrite_lib(options, parsed['lib'])

We need to rewrite a build script, if it exists, correct?

Tests for some of this stuff would be good.

::: config/rules.mk
@@ +928,5 @@
>  #
>  # We need to run cargo unconditionally, because cargo is the only thing that
>  # has full visibility into how changes in Rust sources might affect the final
>  # build.
> +force-cargo-build: $(CARGO_FILE)

Before this patch, make would look at this target, and always re-run it.  Cargo would take care of recompiling the necessary files based on what had changed, possibly an empty set.

Now, make checks the $(CARGO_FILE) dependency, which always need to be re-run, and gen_cargo_toml.py always writes a new Cargo.toml.  So the question here is whether cargo is smart enough to notice that Cargo.toml hasn't changed (even though the mtime on the file is different), and therefore recompile only the Rust sources that have changed, or whether "changing" Cargo.toml necessitates a rebuild of the entire Rust world.

Using FileAvoidWrite in gen_cargo_toml.py might help mitigate this problem; I think make will run the commands below even if $(CARGO_FILE) isn't updated by its commands, but I'm not totally sure...

All of this is moot with the comment below, but it is at least worth thinking about.

@@ +936,5 @@
> +	cp Cargo.lock $(srcdir)/Cargo.lock
> +
> +$(CARGO_FILE):
> +	$(REPORT_BUILD)
> +	$(PYTHON) $(topsrcdir)/config/gen_cargo_toml.py --srcdir "$(srcdir)" --output $@

We don't want to build this on every invocation of |make|; we only want to build it when configure settings change, i.e. we want it built during build-backend in moz.build.  When moving this into moz.build, you want to build this file in the common backend, if at all possible, not the recursivemake backend.

::: config/rust_build_profile.py
@@ +4,5 @@
> +_MOZ_DEBUG = bool(substs.get('MOZ_DEBUG', False))
> +
> +RUST_BUILD_PROFILE = {
> +    'opt-level': 2 if _MOZ_OPTIMIZE else 0,
> +    'debug': _MOZ_DEBUG,

Note that this is always True in our current profiles; it shouldn't be dependent on MOZ_DEBUG.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Generate Cargo.toml files to seperate MOZ_OPTIMIZE from MOZ_DEBUG for rust code

This is a major rewrite which integrates it better with the build backend, uses a wrapper crate instead of rewriting Cargo.toml.in files, which was error-prone, and adds a cargo command for regenerating lockfiles across the tree.

Comment 9

[(not currently active) Ted Mielczarek]

My patch in bug 1294565 for `mach vendor rust` does generate-lockfile as part of it. Is the problem here that since you're using lockfiles in the objdir `cargo build` isn't sufficient to wind up updating them?

I am not wild about this, using Cargo.toml files from the objdir makes our build different from the typical cargo workflow and seems likely to expose us to weird bugs that regular rust development won't hit.

I also have a patch that was split off from the patch in bug 1298422 to use cargo workspaces for all our crates, I don't know how that will interact with this.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> My patch in bug 1294565 for `mach vendor rust` does generate-lockfile as
> part of it. Is the problem here that since you're using lockfiles in the
> objdir `cargo build` isn't sufficient to wind up updating them?

When we build with `cargo build` we pass `--frozen` to the cargo build command, so the lockfiles won't update. My command is mostly as a utility because we need to generate lockfiles for crates which don't exist in tree, and copy them into the tree to commit them such that we can build with `--frozen`.

> I am not wild about this, using Cargo.toml files from the objdir makes our
> build different from the typical cargo workflow and seems likely to expose
> us to weird bugs that regular rust development won't hit.

All I'm doing is creating a crate at build time which wraps an arbitrary crate with a path dependency in order to control the build options and the crate type. I doubt I'll hit too many problems with that, it's just slightly tedious because we pass --frozen, and thus I have to juggle around the lockfile. This would be a lot easier if we just didn't have the lockfiles, (after all, we are vendoring the versions of the crates we are using in tree, so it's not like the version we are pinning to can change), but I felt like that would be even less likely to be merged. 

> I also have a patch that was split off from the patch in bug 1298422 to use
> cargo workspaces for all our crates, I don't know how that will interact
> with this.

We would simply generate a workspace in the objdir which holds each of the generated crates, and manage a single lockfile for this workspace. I imagine it would actually simplify the system.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: ALTERNATIVE - Remove Cargo.locks from tree, and don't pass --frozen to Cargo

This is a quick example of how much simpler the system would be if we didn't pass --frozen to cargo, and instead just allowed it to generate its lockfiles itself.

It would make generating code easier (as we don't have to juggle around lockfiles). I believe that it has the disadvantage of us being less confident that we aren't hitting the network? (maybe?) but I think with the cargo-vendor system we shouldn't be hitting the network anyways as we are exclusively using a local repository.

As we would only have a single version of each crate vendored, it's not like we need to worry about pinning down exact versions. That constraint is already recorded in our vendoring setup.

Comment 12

[(not currently active) Ted Mielczarek]

The `cargo vendor` stuff actually does require lockfiles to be checked in, because it puts a hash of each crate into the lockfile.

(In reply to Michael Layzell [:mystor] from comment #10)
> All I'm doing is creating a crate at build time which wraps an arbitrary
> crate with a path dependency in order to control the build options and the
> crate type. I doubt I'll hit too many problems with that

This is pretty different from what normal cargo usage looks like. I'd like to try to keep our weirdness to a minimum so that we aren't causing ourselves extra pain.

More generally, I think if this is a pain point we should get an issue open on cargo (if there isn't already one, per comment 2) and figure out what fixing this in cargo looks like.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8789857 [details] [diff] [review]
Generate Cargo.toml files to seperate MOZ_OPTIMIZE from MOZ_DEBUG for rust code

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

I think Ted has the right of it; if we want to solve this problem, what we should really be doing is writing custom profile support for Cargo, which would let us do this in about 10 lines, rather than hacking up all manner of things.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use the RUSTFLAGS environment variable to control the flags Cargo passes to Rustc

This is a new implementation strategy which I realized we could use after the previous design failed. It uses the RUSTFLAGS environment variable to manually specify the rustc command line arguments. As rustc respects the last-added option for most compiler options, this has the desired effect of allowing us to control the build options without modifying Cargo.toml.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8791439 [details] [diff] [review]
Use the RUSTFLAGS environment variable to control the flags Cargo passes to Rustc

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

Wow, this is a little more...thorough than I thought this patch was going to be.  I thought we were just going to have, in rules.mk:

# Cargo currently supports only two interesting profiles for building: development and release.
# Those map (roughly) to --enable-debug and --disable-debug in Gecko, respectively, but there's
# another axis that we'd like to support: --{disable,enable}-optimize.  Since that would be
# four choices, and Cargo only supports two, we choose to enable various optimization levels in
# our Cargo.toml files all the time, and override the optimization level here, if necessary.
# (The Cargo.toml files already specify debug-assertions appropriately for --{disable,enable}-debug.)
ifndef MOZ_OPTIMIZE # or whatever the incantation is
rustflags_override = -C opt-level=0
endif

...

force-cargo-build:
        env CARGO_TARGET_DIR=. RUSTFLAGS="$rustflags_override" RUSTC=$(RUSTC) $(CARGO) build ...

Since we're not agreed on the expandlibs_exec.py approach from bug 1300208, can we just go with something like the above, if it works, and then modify that in whatever other bugs we need?

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: enable --disable-optimize --enable-debug to DTRT for Rust code

Man, that was a little too long to let this fix sit around. :(

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 8808805 [details] [diff] [review]
enable --disable-optimize --enable-debug to DTRT for Rust code

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

::: config/rules.mk
@@ +939,5 @@
>  # has full visibility into how changes in Rust sources might affect the final
>  # build.
>  force-cargo-build:
>  	$(REPORT_BUILD)
> +	env $(rustflags_envvar) CARGO_TARGET_DIR=. RUSTC=$(RUSTC) $(CARGO) build $(cargo_build_flags) --

You don't actually set `rustflags_envvar` anywhere... Did you leave a bit out of this patch?

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: enable --disable-optimize --enable-debug to DTRT for Rust code

Doh, you're right.  That's what I get for last-minute refactoring.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc554c8e9e6a
enable --disable-optimize --enable-debug to DTRT for Rust code; r=ted.mielczarek

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc554c8e9e6a