Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for named profiles (RFC 2678) #6989

Merged
merged 67 commits into from
Sep 30, 2019
Merged

Conversation

da-x
Copy link
Member

@da-x da-x commented May 27, 2019

Tracking issue: #6988

Implementation according to the RFC.

This allows creating custom profiles that inherit from other profiles.

For example, one can have a release-lto profile that looks like this:

    [profile.release-lto]
    inherits = "release"
    lto = true

The profile name will also carry itself into the output directory name
so that the different build outputs can be cached independently from
one another.

So in effect, at the `target` directory, a name will be created for
the new profile, in addition to the 'debug' and 'release' builds:

```
    $ cargo build --profile release-lto
    $ ls -l target
    debug release release-lto
```
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2019
@ehuss
Copy link
Contributor

ehuss commented May 31, 2019

Nice! I'm looking forward to landing this.

Did you have any questions or have anything you want me to look at now? I've only skimmed this so far, and the only major thing that jumped out is that all new behavior will need to be behind a feature gate (documented in features.rs).

@da-x
Copy link
Member Author

da-x commented Jun 1, 2019

Should the feature gate only affect the use of the [profile.<custom-name>] clauses, or should it also make other behavior changes conditional? (There are other changes, such as the added consistency for smoothing out of the behavior of specifying --profile= combined with --debug and --release across command).

da-x added 4 commits June 1, 2019 13:44
When resolving profile parameters for custom named profiles, we can skip
'dev' and 'release' because they are the root profiles from which all
others inherit. Otherwise, for example, we fail the tests where a simple
`[profile.dev]` clause is specified.
@da-x
Copy link
Member Author

da-x commented Jun 1, 2019

Pushed changes: fixed a bug ; fixed the build of Cargo's tests ; fixed two tests, there are 25 tests still failing which need fixing.

da-x added 4 commits June 1, 2019 15:06
The predefined 'test' and 'bench' profiles should exist even if
the user did not define anything in the Cargo.toml. This also
rewrites how the merging is done.
The profile returned for custom build should have the proper 'root'
field value.

This fixes test build_script::profile_and_opt_level_set_correctly.
Since we also for 'cargo rustc' to received '--profile check', let
it be consistent with the other commands and use a predefined 'check'
profile for it that inherits from "dev".

Fixes tests check::rustc_check and check::rustc_check_err.
@da-x
Copy link
Member Author

da-x commented Jun 1, 2019

More fixes made, 12 failures remaining.

@da-x
Copy link
Member Author

da-x commented Jun 1, 2019

@ehuss The profile_targets::profile_selection* test failures are mainly the ones that may benefit from your observation. The implementation changed which profile is being picked for which target. This is some of the confusion we wanted to clear up regarding how Cargo picks from the various profiles under various commands.

The results from the current PR changes are that at least for profile_selection_bench(), only codegen-units=4 is being seen in verbose, which means only [profile.bench] gets picked up, which I assume is what we wanted. The effects mainly stem from the removal of the reliance over CompileMode:: in src/cargo/core/profiles.rs.

@ehuss
Copy link
Contributor

ehuss commented Jun 3, 2019

I think the feature gate should cover the new [profile] tables and the --profile flag. Generally anything that changes the UI. The --profile flag can just be gated on -Zunstable-options (just check config.cli_unstable().unstable_options before reading it).

As for the change in consistency for test/bench, I'm on the fence. I think it would be OK to change it now as long as it inherits the settings from dev/release. Assuming that, then I think you should go ahead and update the profile_selection tests.

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2019
da-x added 6 commits June 7, 2019 21:47
Before the RFC, the 'clean' command would have cleaned entirety of
target/ unless `--release` is specified - in that case it would have
cleaned only `target/release`.

Initial commit broke that.

Now, passing `--profile=NAME` will clean the directory related to
profile `NAME` via the dir-name setting. `--release` is supported for
backward compatibility.
New code relies on a BTreeMap, rather than handed coded calls.
@ehuss
Copy link
Contributor

ehuss commented Sep 21, 2019

Not sure if you saw my question about check, fix, and rustc above. Can those support named profiles?

I'll try to get back to you on the mode issue, I'd like to talk to the rest of the team about it.

@da-x
Copy link
Member Author

da-x commented Sep 22, 2019

  • fix and check - both make use of --profile to determine if it's "test" or not. If we remove that code, it will make use of the custom profile, but then we would still need to determine the boolean given to CompileMode::Check in some other way. One way to solve it is check if we indirectly or directly inherit from test profile. Another is to add a new attribute to all profiles that would be used to determine CompileMode only for those special commands, and then lift the restriction regarding "test".
  • rustc already makes use of --profile with the changes, and also uses it to determined CompileMode, and likely needs a similar solution for it.

@bors
Copy link
Contributor

bors commented Sep 25, 2019

☔ The latest upstream changes (presumably #7421) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Sep 26, 2019

☔ The latest upstream changes (presumably #7425) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Sep 27, 2019

We discussed the issues with how targets relate to profiles (such as in --all-targets), and…we didn't really have a good answer. There was some desire to keep the existing behavior where targets can drive which profile is chosen (maybe even making that explicit or configurable). But we decided this is something we can iterate on later, and we'd definitely like to get this PR landed.

The main issue left is that I don't think there should be any changes to existing behavior without the use of unstable flags. Can you go through and make sure the original behavior is retained unless cargo-features = ["named-profiles"] is specified? The ones I see are:

  • All the changes in testsuite/profile_targets.rs should keep the original behavior.
  • It looks like profile inheritance is happening for stable profiles. That is, a profile like test is inheriting dev. That should only happen with the unstable feature flag. It should retain the original behavior of using the hard-coded defaults for each profile unless the cargo-feature is specified.

The effects over the profile used by targets are made conditional
in this commit, using the old scheme if the `named-profiles` feature
is disabled. This also affects the `profile_targets` tests, which
now have two modes - stable, and nightly with the feature enabled.
…e=check`

And a small cleanup in `base_profile` to make the logic parallel to the
one in `get_profile`.
@da-x
Copy link
Member Author

da-x commented Sep 28, 2019

Done changes to keep original behavior, except regarding the finished line print which would take a bit more effort (and some wide-reaching test result parameterization - I am not sure if it's wanted).

@ehuss
Copy link
Contributor

ehuss commented Sep 30, 2019

Thanks so much! This looks great!

I'll try to follow up on the tracking issue with some more things to discuss.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2019

📌 Commit fad192d has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Sep 30, 2019
@bors
Copy link
Contributor

bors commented Sep 30, 2019

⌛ Testing commit fad192d with merge 8b0561d...

bors added a commit that referenced this pull request Sep 30, 2019
Support for named profiles (RFC 2678)

Tracking issue: #6988

Implementation according to the [RFC](https://github.com/rust-lang/rfcs/blob/master/text/2678-named-custom-cargo-profiles.md).
@bors
Copy link
Contributor

bors commented Sep 30, 2019

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 8b0561d to master...

@bors bors merged commit fad192d into rust-lang:master Sep 30, 2019
@bors bors mentioned this pull request Sep 30, 2019
@da-x
Copy link
Member Author

da-x commented Oct 10, 2019

@ehuss, previously misguided attempts to affect debug builds via [profile.debug], which resulted in an unused key warning in Cargo, now fail via features.require(Feature::named_profiles()). Are we keeping this behavior or should we support backward compatibility until the feature is ungated?

@ehuss
Copy link
Contributor

ehuss commented Oct 10, 2019

unused key warning in Cargo

Hm, so to be clear, it now issues an error if you don't have the feature enabled and when you do enable the feature, it will require an inherits field, but still issues a warning?

I think I'm fine with that for now. It was an invalid configuration before (it was ignored other than the warning). I don't think we need to be backwards compatible with all invalid configurations.

We may want to reconsider the warning in the future, but doesn't seem too important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants