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

Named custom cargo profiles #2678

Merged
merged 20 commits into from
Jun 1, 2019
Merged

Conversation

da-x
Copy link
Member

@da-x da-x commented Apr 4, 2019

Summary

The proposed change to Cargo is to add the ability to specify user-defined profiles in addition to the four predefined profiles, dev, release, test, and bench. It is also desired in this scope to reduce confusion regarding where final outputs reside, and increase the flexbility to specify the user-defined profile attributes.

Related links

text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
kennytm and others added 4 commits April 4, 2019 22:34
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
@Centril Centril added A-profile Proposals relating to cargo profiles. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. labels Apr 4, 2019
@kornelski
Copy link
Contributor

Is it worthwhile keeping test and bench outputs in target/debug and target/release?

It's very helpful to be able to run test binary under gdb, or the bench binary under a profiler. These have unpredictable paths already, so a different location would be fine, but they should be persisted in an accessible location.

@kornelski
Copy link
Contributor

The RFC doesn't mention cross-compilation. Cargo changes target/release to target/<platform triple>/release when cross-compiling. Presumably dir-name would be too?

@crlf0710
Copy link
Member

Might be orthogonal, but many times i really wished that there's a profile that build the current crate in dev mode but all its dependencies in release mode.

@kornelski
Copy link
Contributor

Yes, a different profile for the workspace and dependencies outside of the workspace would be fantastic.

@ssokolow
Copy link

ssokolow commented Apr 10, 2019

I definitely want something iike this. The "Direct cargo build flags alternative" option wouldn't really change the status quo for me.

The justfile in my project boilerplate already works around the lack of this functionality by using custom CARGO_TARGET_DIR values to simulate it in tasks like kcov.

The downside is that, due to that and my justfile forcing CARGO_BUILD_TARGET to always have a value for the sake of consistency, I treat invoking cargo subcommands directly as unsupported, purely because the odds are high that they'll act as if I'd just run cargo clean.

Another former downside that wasn't specifically due to the lack of custom profiles (rather, it was about opt-level = "z" only being valid in nightly builds, which could have been remedied by defining a release-nightly profile), but which shows how far I'm willing to go, was how hacky it was for my justfile to use sed to rewrite and then restore the [profile.release] section of my Cargo.toml as part of the build process under certain conditions.

(On the plus side, cargo-make's @rust script runner should make it more robust to pull that trick, since I could use toml_edit for my crazy hacks.)

The dev profile receives the dir-name = "debug" attribute, so that its final outputs are emitted to target/debug, as existing Rust developers will expect this behavior. This should be added in the official documentation for the Cargo manifest, to make this fact clearer for users.

A debug profile name is not allowed, with a warning saying to use the already established dev name.

Why is it so important for the profile to be named dev that it justifies this special-casing, this hard-coded exception to letting projects define their own profile names, and this potential point of confusion?

I couldn't find a justification for that.

Do we really need pre-defined profiles for test, bench and/or doc, or can we make them obsolete?

Assuming we take it as a given that we're not going to accept the convenience regression of requiring users to do some setup before they can run cargo test and/or cargo doc, can you elaborate on what the alternative would look like?

@da-x
Copy link
Member Author

da-x commented Apr 10, 2019

@ssokolow:

Why is it so important for the profile to be named dev that it justifies this special-casing, this hard-coded exception to letting projects define their own profile names, and this potential point of confusion?

I couldn't find a justification for that

The main issue is backward compatibility. If the alternative is to abolish the dev profile name in favor of the debug name, it would break existing projects' assumptions about the affects of the [profile.dev] clauses to the cargo build command (without any --profile parameter). Another alternative, is to emit to a target/dev directory instead of target/debug directory, but that would break current assumptions about the default cargo build output directories. Now, if we take neither of the aforementioned alternatives and we allow for a debug profile to be defined, then unwary people may expect the default cargo build command (i.e. without any --profile parameter) to be affected by it, and be surprised when it isn't. This is the problem I've had in mind. Perhaps someone can think of a better solution.

Assuming we take it as a given that we're not going to accept the convenience regression of requiring users to do some setup before they can run cargo test and/or cargo doc, can you elaborate on what the alternative would look like?

The alternative would be for bench to use the release profile (unless --profile is specified), and for doc and test to use dev (unless --profile is specified), and to remove these predefined profiles from code and documentation.

@da-x
Copy link
Member Author

da-x commented Apr 10, 2019

The RFC doesn't mention cross-compilation. Cargo changes target/release to target/<platform triple>/release when cross-compiling. Presumably dir-name would be too?

Yes, that would be the expected behavior. Should update the RFC.

@da-x
Copy link
Member Author

da-x commented Apr 10, 2019

Might be orthogonal, but many times i really wished that there's a profile that build the current crate in dev mode but all its dependencies in release mode.

@crlf0710 @kornelski I think this is achievable with profiles overrides (currently in nightly cargo).

Combined with this RFC's changes, it would be even better, because you could have these overrides under a custom profile, retaining the standard functionality with the dev profile.

For example:

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

(See Unstable Features - The Cargo Book)

@ssokolow
Copy link

The main issue is backward compatibility. If the alternative is to abolish the dev profile name in favor of the debug name, it would break existing projects' assumptions about the affects of the [profile.dev] clauses to the cargo build command (without any --profile parameter). Another alternative, is to emit to a target/dev directory instead of target/debug directory, but that would break current assumptions about the default cargo build output directories.

Ahh, yes. I'd forgotten about that existing mismatch. Hazard of putting together a project template once and then not needing to modify it much, I guess.

@kornelski
Copy link
Contributor

kornelski commented Apr 11, 2019

Maybe dev profile name could be used only as a fallback? If there's profiles.debug in Cargo.toml, then use that. Otherwise try profiles.dev. That would keep backwards compatibility, and allow eventually deprecating and removing the dev name.

It's for development only, so it doesn't affect crate users, and the developer will see any deprecation notices.

@da-x
Copy link
Member Author

da-x commented Apr 11, 2019

Maybe dev profile name could be used only as a fallback? If there's profiles.debug in Cargo.toml, then use that. Otherwise try profiles.dev. That would keep backwards compatibility, and allow eventually deprecating and removing the dev name.

It's for development only, so it doesn't affect crate users, and the developer will see any deprecation notices.

So the suggestion as I understand - having a debug profile defined will affect the default cargo build, cargo test, and cargo doc profile to be debug instead of dev. We can limit the emission of the warning to the case where there are customization on the dev profile.

However I am not sure it puts the dev profile on a full deprecation path, at least not during 1.x Rust, because the Cargo.toml of many old crates will probably still remain without a debug profile defined, and with dev customized.

This may be a good, but I'd feel better with more votes on the matter. Pinging @ehuss @matklad @Manishearth.

@Manishearth
Copy link
Member

(I'm not really a cargo person, and while I've written a profile RFC before I don't have strong opinions on this. I recall the cargo team having plans about profiles, though)

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this together!

Some questions/comments:

  • What happens if the user specifies --release --profile=foo?
  • Is --release an alias for --profile=release?
  • Currently the profile is driven by the command, cargo target (--test uses test, --bench uses bench, --all-targets uses a mixture, etc.), and the --release flag. Does Cargo continue to behave that way if no profile is chosen? I'm concerned that is a little confusing, but the alternative (only using one profile) would also be problematic.
  • doc is currently unused, and I think has some issues that would prevent it from being reintroduced. I would prefer to leave out doc in the discussion here and leave it for future consideration.
  • There should be restrictions on valid profile names. I'm thinking the same as a package name (validate_package_name) would be a good start, though maybe it should be even more restrictive?
  • Can you add a link to the PR description to https://internals.rust-lang.org/t/strawman-pre-rfc-custom-profiles/6412 which had a very similar proposal?
  • Does cargo test --profile=foo build all crates with the foo profile? Currently cargo test is a little strange in that dependencies are built with dev/release. I'm thinking this might be a good opportunity to change that, so that test/bench are less special. The drawback is that it takes longer to compile if you change the test profile (since it can't share dependencies with dev), but it may be worthwhile.

text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved

Finished release-lto [optimized + lto] target(s) in 3.83s

* As before, optimization mode is always printed, with `optimized` if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the Finished line is that multiple profiles may be used during a single command. For example, cargo build --all-targets can use both dev and test. I think this will need more consideration to figure out the best way to present a final summary.

FWIW, I don't mind that it is slightly incorrect now, so I don't think it is critical to fix this. But it should be fixed at some point, so it would be good to hear if anyone has any ideas on how to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on more feedback from more users, we may leave it out from the RFC or move it to the 'future possibilities' section (makes sense?). Maybe a good solution would come out of trying to implement a better Finished line while working to implement this RFC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do think it would be realistic to print a Finished line per target, with the profiles that used to build it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with deferring the details here. I'm not sure if printing multiple lines would be desirable. Perhaps after it's implemented we could experiment with a few different examples and come up with something people can look at.

* Similarly, `TomlProfiles` will be changed to hold profiles in a `BTreeMap`.
* We would need to compute the actual `build_override` for a profile based on
resolution through the `inherited` key.
* Custom build scripts: For compatibility, the `PROFILE` environment currently
Copy link
Contributor

@ehuss ehuss Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change. Why the output directory and not the profile name? Some build scripts use it for accessing the output directory, but in general they shouldn't be.

Also, build scripts today only expect two values ("release" and "debug") and typically use that to determine how to compile other code. They will have no knowledge of any other profile name (especially within dependencies which are from other projects). I'm not sure we can change this behavior. No matter what is done here, I think it will break some projects.

I'm wondering if it might be best to deprecate "PROFILE", and build scripts should use "DEBUG" and "OPT_LEVEL" for determining build settings. I don't think they should be writing to the target directory. It could keep some sort of "best guess" to set it to "debug" or "release".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it would be ok, to have it debug or release depending on the inherited-from profile being used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so?

text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
text/0000-named-custom-cargo-profiles.md Outdated Show resolved Hide resolved
ehuss and others added 6 commits April 14, 2019 11:21
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
@da-x
Copy link
Member Author

da-x commented Apr 14, 2019

@ehuss, thanks for investing the time to review!

  • What happens if the user specifies --release --profile=foo?
  • Is --release an alias for --profile=release?

I'd suggest the following:

  • Using --profile and --release together will emit an error unless --profile=release.
  • Using --release on its own will be equivalent to specifying --profile=release
  • Currently the profile is driven by the command, cargo target (--test uses test, --bench uses bench, --all-targets uses a mixture, etc.), and the --release flag. Does Cargo continue to behave that way if no profile is chosen? I'm concerned that is a little confusing, but the alternative (only using one profile) would also be problematic.

Yes. I think it would be too late to deprecate this functionality.

  • doc is currently unused, and I think has some issues that would prevent it from being reintroduced. I would prefer to leave out doc in the discussion here and leave it for future consideration.

Agreed.

  • There should be restrictions on valid profile names. I'm thinking the same as a package name (validate_package_name) would be a good start, though maybe it should be even more restrictive?

Yes, let's take the restriction from package names: Name must not be empty, use only alphanumeric characters or - or _.

Was not aware of it. Sure.

  • Does cargo test --profile=foo build all crates with the foo profile? Currently cargo test is a little strange in that dependencies are built with dev/release. I'm thinking this might be a good opportunity to change that, so that test/bench are less special. The drawback is that it takes longer to compile if you change the test profile (since it can't share dependencies with dev), but it may be worthwhile.

It's very good that you brought this up. If I understood correctly, only the special test crate is built with the test profile all the dependencies including the current crate are built with either dev or release, depending on whether --release is provided (this includes both dependencies and dev-dependencies). This is what the Cargo docs say:

# The release profile, used for `cargo build --release` (and the dependencies
# for `cargo test --release`, including the local library or binary).

The existing functionality has the advantage that in cargo test --release the a test crate code which makes heavy use of dependencies for testing runs fast due to the optimized dependencies, while the test crate itself is still in debug mode, having more checks and emitting full backtraces for itself. Do we want to regress on this behavior? It may depends on how much logic people put inside their test crates, and we may need more data or user feedback for deciding.

Co-Authored-By: da-x <alonid@gmail.com>
ehuss and others added 4 commits April 14, 2019 11:37
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
Co-Authored-By: da-x <alonid@gmail.com>
@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2019

Do we want to regress on this behavior? It may depends on how much logic people put inside their test crates, and we may need more data or user feedback for deciding.

I'm not sure. I'll ask the team if there's any opinion on this.

Spurious b indeed. Thanks.

Co-Authored-By: da-x <alonid@gmail.com>
@ehuss
Copy link
Contributor

ehuss commented May 13, 2019

Does cargo test --profile=foo build all crates with the foo profile? Currently cargo test is a little strange in that dependencies are built with dev/release.

Discussed this with the team. I think we all agree that we should remove the special casing for test/bench, and make them behave like a normal profile (that is, they don't treat dependencies differently). If someone really wants to only apply profile settings to the local crate, they can use "*" profile override to achieve something close to the old behavior. If you want to add that as a note to this RFC, that would be fine.

I'd like to move this forward, as I think we're all 👍 on the basic concept of adding named profiles.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 13, 2019

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels May 13, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented May 13, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label May 13, 2019
@da-x
Copy link
Member Author

da-x commented May 15, 2019

Thanks! Regarding this discussion:

Maybe dev profile name could be used only as a fallback? If there's profiles.debug in Cargo.toml, then use that. Otherwise try profiles.dev. That would keep backwards compatibility, and allow eventually deprecating and removing the dev name.
It's for development only, so it doesn't affect crate users, and the developer will see any deprecation notices.

So the suggestion as I understand - having a debug profile defined will affect the default cargo build, cargo test, and cargo doc profile to be debug instead of dev. We can limit the emission of the warning to the case where there are customization on the dev profile.

However I am not sure it puts the dev profile on a full deprecation path, at least not during 1.x Rust, because the Cargo.toml of many old crates will probably still remain without a debug profile defined, and with dev customized.

This may be a good, but I'd feel better with more votes on the matter. Pinging @ehuss @matklad @Manishearth.

It's the main point still standing, i.e whether to have this fallback or keep the RFC as it is. I'm assuming this can be a version 2.0 material?

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label May 23, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented May 23, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 23, 2019
@Centril Centril merged commit de0917d into rust-lang:master Jun 1, 2019
@Centril
Copy link
Contributor

Centril commented Jun 1, 2019

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/cargo#6988

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-profile Proposals relating to cargo profiles. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants