Closed Bug 1300835 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
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?
Flags: needinfo?(acrichton)
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.
Flags: needinfo?(acrichton)
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.
Attachment #8789020 - Flags: review?(nfroyd)
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.
Assignee: nobody → michael
(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 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.
Attachment #8789020 - Flags: review?(nfroyd) → feedback+
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.
Attachment #8789857 - Flags: review?(nfroyd)
Attachment #8789020 - Attachment is obsolete: true
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.
(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.
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.
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 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.
Attachment #8789857 - Flags: review?(nfroyd)
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.
Attachment #8791439 - Flags: review?(nfroyd)
Attachment #8789857 - Attachment is obsolete: true
Attachment #8789935 - Attachment is obsolete: true
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?
Attachment #8791439 - Flags: review?(nfroyd)
Man, that was a little too long to let this fix sit around. :(
Attachment #8808805 - Flags: review?(ted)
Attachment #8791439 - Attachment is obsolete: true
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?
Attachment #8808805 - Flags: review?(ted)
Doh, you're right.  That's what I get for last-minute refactoring.
Attachment #8812273 - Flags: review?(ted)
Attachment #8808805 - Attachment is obsolete: true
Attachment #8812273 - Flags: review?(ted) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc554c8e9e6a
enable --disable-optimize --enable-debug to DTRT for Rust code; r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/dc554c8e9e6a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1322803
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.