Cargo profile dependencies

[Manishearth]

Tracking issue: rust-lang/rust#48683

Rendered

[Manishearth]

pre-rfc at https://internals.rust-lang.org/t/strawman-pre-rfc-custom-profiles/6412/9

[matklad]

Custom profiles can be listed in a .cargo/config, however the user is responsible for clearing up build directories if the profile changes. That is, it is undefined behavior to run cargo build --profile foo if foo has been defined in .cargo/config and the profile has been edited since the last time you ran cargo build --profile foo.

Hm, I would think Cargo should just rebuild everything in this case? And I even think this will happen naturally with the current implementation: IIRC, compiler flags are hashed into the fingerprint for determining if the crate should be rebuild.

What is the purpose of profiles .cargo/config? Is it something for the "big build system" use case? One interesting idea I have is that we can allow to specify profile as json on the command line. That would allow the build system to control Cargo without writing any files.

[Manishearth]

Hm, I would think Cargo should just rebuild everything in this case?

I think tracking changes to config files might be out of scope for cargo

And I even think this will happen naturally with the current implementation: IIRC, compiler flags are hashed into the fingerprint for determining if the crate should be rebuild.

If this is true then that works great!

What is the purpose of profiles .cargo/config? Is it something for the "big build system" use case?

Well, two purposes. Lets people specify global useful profiles like cargo build --debugopt or something.

Also, it fixes the big build system case -- Having a custom profile for each combination of flags quickly leads to blowup. Firefox lets you individually flip debug symbols, debug-assertions, and optimization levels. There are more involved, though. (There are use cases; folks hacking on Rust code may want different settings from folks hacking on C++ code)

Profiles aren't part of the package anyway, they're build info, and that really should go in config anyway.

[est31]

Is this compatible with the profiles 2.0 proposal by @matklad ? IIRC there was an embargo for new features for profiles until they get revamped? Generally I like the idea of this RFC though.

[ehuss]

Is this an unfinished statement?

[Manishearth]

My bad, that line should be removed.

[Manishearth]

@est31 I think this addresses the "some kind of DSL to map crates to flags" from profiles 2.0

This RFC does not attempt to handle the issue with test/bench/doc but I think that can be addressed independently. One solution for that would be closer to the "includeable profiles" proposal in the pre-RFC, and have something like [profile.dev] testing = otherprofilename

[matklad]

@est31 I think of this RFC as basically profiles 2.0, and I really love it :) It doesn't solve the "which build in profile is run when" problem, but I think it can be addressed separately. Although I would prefer that instead of [profile.dev] testing = otherprofilename we'd just killed all build-in profiles other then dev or release.

[est31]

Naming folders after profiles is no good idea because of profiles named del or similar. Better is to name them as profile.foo or sth.

[matklad]

Profiles affect only the leaf crate, so I think it's ok not to guard against bad names here. That is, even if one uploads crate with a forbidden profile name to crates.io, no bad things should happen.

Although, it might be a good idea to give a warning/error like "Hey, you named your profile NUL, and this will probably break your crate on windows".

[Manishearth]

yeah, I agree with @matklad here. We can add warnings for badly-named profiles (doesn't need to be in the rfc)

[ben0x539]

Hash the profile data into the output path and you don't need to worry about undefined behavior on profile changes, I guess.

[jonhoo]

One question I have with this is how this applies transitively. For example, consider crate c which depends on b which depends on a. Say b knows that a should always be compiled with optimizations. If c is compiled in debug mode, is it necessary for c to also declare that b.a should be compiled with optimizations? Or can c somehow be informed by b that a should be compiled with optimizations?

[matklad]

@jonhoo currently, only the leaf crate's profiles are accounted for (it can even be argued on this basis that profiles should not live in Cargo.toml and should go into .cargo/config instead), and, for workspaces, you can only specify profiles in the root of the workspace. I believe that this RFC does not propose to change this mechanics.

I can imagine an orthogonal RFC though, which allows upstream crate to advice some profile settings to the downstream crates. For example, the image crate could advise "compile me with -O 3 in the dev profile", and this setting would be used for crates which depend on image, unless they explicitly override it.

The possible interaction between this RFC and hypothetical advise RFC is that it's not clear how upstream crates can advice on custom profiles (they don't even know their names!). I think it's reasonable to assume though that this is not a super important problem which we need to solve now: either of "advises do not work with custom profiles" or "custom profile can opt into advises from a particular build-in profile" seems like an ok solution.

[matklad]

Also, cc @wycats for the whole discussion :)

[jonhoo]

@matklad you're right that this could totally be addressed in a separate RFC. In fact, I wonder whether, if we had that RFC, that would alleviate many of the pains that this RFC tries to counter. The reason I bring this up as a concern here is that I worry that we'll see these extra profiles added all over the place as a result of this particular solution to the problem. Basically every crate that depends on image would add this extra profile (probably with different names too!), and crates that depend on those crates would also add them, and so on. Going forward with this RFC (without the other one first) will likely lead to lots of people sticking extra duplicated overrides in their Cargo.toml, which bothers me. An "upstream-down" approach strikes me as much nicer for the ecosystem as a whole.

On a somewhat separate tangent: how often do we expect users to actually add separate profiles, rather than just adding overrides for the debug profile? I personally think (though I may be wrong) that the primary use-case for this is compiling specific dependencies with opts in debug mode, in which case it's unclear we even need custom profiles, and not just "being able to specify overrides for existing profiles". Do we have a compelling use-case for where an entirely new profile is warranted?

[Manishearth]

@jonhoo FWIW this RFC does propose allowing profiles in .cargo/config, but it also allows them in Cargo.toml for consistency

[matklad]

@jonhoo note that the RFC supports overrides for build-in profiles, so I don't believe that "every crate that depends on image would add this extra profile". Rather, every crate which depends on image would add [profiles.dev.overrides.image] opt-level = 3. This indeed will be duplicated, but the amount of duplication is small, and gives you extra flexibility as each crate can decide precisely on the settings for image.

In general, I think the need to customize profiles should be rare, so I'd go for the increased flexibility rather than for increased convenience (but the syntax proposed in this RFC looks pretty and convenient).

I personally think (though I may be wrong) that the primary use-case for this is compiling specific dependencies with opts in debug mode, in which case it's unclear we even need custom profiles, and not just "being able to specify overrides for existing profiles".

Hm, interesting! I believe that it is indeed possible to split this RFC further into "overrides" and "custom profiles" parts, and I agree that "overrides" part has potentially many more users, because fully custom profiles a required mainly by large users like Servo. My personal preference would be to nevertheless pursue the custom profiles part. I really like how it creates a very simple mental model for profiles: "if you pass --profile foo, then everything uses the foo profile". This model is pretty different for the one for build-in profiles, where different parts of the packages use different profiles. And having such simple model implemented in Cargo would be very useful, because it would make "fixing" build-in profiles to use this model simpler.

[matklad]

@rfcbot fcp merge

[rfcbot]

Team member @matklad has proposed to merge this. The next step is review by the rest of the tagged teams:

• @alexcrichton
• @aturon
• @carols10cents
• @joshtriplett
• @matklad
• @withoutboats
• @wycats

Concerns:

• build_override resolved by Cargo profile dependencies #2282 (comment)
• Document how to set panic resolved by Cargo profile dependencies #2282 (comment)
• workspaces resolved by Cargo profile dependencies #2282 (comment)

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[matklad]

@rust-lang/cargo discussed this RFC at today's meeting, and it looks really great!

There are couple of implementation-level concerns/questions though.

[matklad]

@rfcbot concern workspaces

Cargo supports building several packages together as a part of the workspace. In workspaces, only the profile section from the root of the workspace (virtual or not) is taking an effect. Currently this works as expected, because profiles apply to all crates in the DAG of dependencies equally.

However with this RFC there's an observable difference between the "current" crate and all other crates. How this interacts with multi-package workspaces.

For example, if workspace consists of foo and bar, and foo depends on bar, and profiles section contains

[profile.dev]
opt-level = 0

[profile.dev.overrides."*"]
opt-level = 2

what rule applies to bar?

[matklad]

@rfcbot concern build_override

build_override definitely feels like a special case. Would it be feasible/worthwhile to make a general feature which allows to tweak each target? The staraman syntax would be [profile.dev.overrides.foo.build], [profile.dev.overrides.foo.bin."*"], [profile.dev.overrides.foo.test.test_bar]?

I personally also think that maybe we should just always compile build.rs files with optimizations? You compile build.rs only once, so it seems like a win, and is somewhat similar to cargo install in spirit. There's also a question of what to do with crates like LALRPOP: if you use it, you definitely want to compile build.rs in release mode, and, with current proposal, every reverse dependecy of LALRPOP would need to specify this explicitly.

It would be great if someone could measure the impact of always-release build.rs. How frequent are the cases when it significantly worsens compile times? How frequent are the cases where compile times are improved?

[Manishearth]

uh oh it's the robot rebellion

[matklad]

This sentence is :)

[rfcbot]

The final comment period is now complete.

[matklad]

We've discussed the broader issue of workflows at today's meeting, here's the summary.

Today's profiles represent two different things. The first one is the workflow, or operation, which Cargo currently is doing, like testing, documenting or benchmarking. The other, more narrow, is the set of "optimization flags" applied when building the code.

It makes sense to split the current profile concept into two separate ones, by introducing workflow as a first-class thing for describing a particular set of operations cargo does, and scaling down profile concept to just mean the set of optimization flags.

One can imagine that there will be roughly one workflow for each major Cargo command (testing workflow, documenting workflow, benchmarking workflow). Ideally, it should be possible to add custom workflows like a fuzzing workflow.

In contrast, there should be relatively fewer profiles: dev and release seem to cover most use-case, but obvious possible extensions include release-with-debuginfo and a "slightly optimized" dev.

Each workflow should be linked to some default profile, but the profile could be overridden via command line flag. For example, testing workflow would use dev profile by default, and benchmarking workflow would use the release profile, however, test --profile=release would also work.

Now, the interesting question is, which flags belong to profiles, and which flags belong to workflow? The interesting dimension for comparison here is "can I apply this flag to only part of the packages?". It is interesting because workflows are global for the whole project, while profiles, with this RFC, can apply only to the specific packages. However, certain optimization flags, like lto apply to the artifact as a whole: i.e, their granularity is larger than a crate, but smaller than project. So, this gives us three bins to sort flags into:

• workflow flags (per project)
• profile global (per artifact)
• profile local (per package == per dependency == per library crate)

Sorting the current flags gives this picture:

• workflow:

  • rpath
  • panic

• profile global:

  • lto
  • codegen-units

• profile local:

  • opt-level
  • debug
  • debug-assertions

[Manishearth]

Why is codegen-units profile global?

I think LTO, rpath, panic are all global, of which rpath/LTO are workflow-related ones, and panic is weird.

We can amend this RFC so that LTO, rpath, panic can only be specified at the top level

(we already do for panics)

[Manishearth]

@matklad I'm confused as to what the status of this RFC is. Was your comment an indicator of the overall plans for profiles, or a request for amendments to this one (seems like they would be minor). The FCP has finished already.

[matklad]

@Manishearth this was a comment about the general future direction of profiles, the RFC is great as it stands! I think it should be merged now, but I am not sure about the precise process to do this :)

[Manishearth]

Cool. I think we're just supposed to rename the file and fill in details and then merge it. I'll let @nrc merge

[Manishearth]

Updated

[Manishearth]

https://botbot.me/mozilla/rust-internals/2018-03-02/?msg=97436474&page=5