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

Cargo check does not respect the --release flag #5218

Closed
bepzi opened this issue Mar 21, 2018 · 4 comments · Fixed by #5384
Closed

Cargo check does not respect the --release flag #5218

bepzi opened this issue Mar 21, 2018 · 4 comments · Fixed by #5384
Labels

Comments

@bepzi
Copy link

bepzi commented Mar 21, 2018

Running cargo check --release is functionally equivalent to cargo check.

Minimal test case:

// src/main.rs

// Warn about unused stuff when in debug mode, fail compilation in release
#![cfg_attr(debug_assertions, warn(unused))]
#![cfg_attr(not(debug_assertions), deny(unused))]

fn main() {
    let unused = 0;
}

Results:

Command Expected Result Actual Result
cargo build warnings warnings
cargo build --release failure failure
cargo check warnings warnings
cargo check --release failure warnings

Reproduced on cargo 0.25.0 (96d8071da 2018-02-26) and cargo 1.26.0-nightly (d6c3983fe 2018-03-16).


I did a bit of digging, and I think I know why this is happening:

When running cargo check --release, the debug_assertions variable is still set to true instead of false, but when running cargo build --release, that flag is properly left disabled.

debug_assertions is defined within the Profile struct (manifest.rs), and an instance of Profile is retrieved during compilation:

let build = if release {
&profiles.release
} else {
&profiles.dev
};
let test = if release {
&profiles.bench
} else {
&profiles.test
};
let profile = match mode {
CompileMode::Test => test,
CompileMode::Bench => &profiles.bench,
CompileMode::Build => build,
CompileMode::Check { test: false } => &profiles.check,
CompileMode::Check { test: true } => &profiles.check_test,
CompileMode::Doc { .. } => &profiles.doc,
CompileMode::Doctest => &profiles.doctest,
};

As you can see above, when finding the correct Profile we essentially just get the default Profile instance defined for each action (assuming we're using a default Workspace struct, which most people are) - with the notable exception of CompileMode::Build: for that mode, we either return a Profile made for release-mode or the default Profile that has debug_assertions enabled. We don't do this for CompileMode::Check. So, assuming we just retrieve the default Profile...

The default profiles are defined as such:

let default_profiles = Profiles {
release: Profile::default_release(),
dev: Profile::default_dev(),
test: Profile::default_test(),
test_deps: Profile::default_dev(),
bench: Profile::default_bench(),
bench_deps: Profile::default_release(),
doc: Profile::default_doc(),
custom_build: Profile::default_custom_build(),
check: Profile::default_check(),
check_test: Profile::default_check_test(),
doctest: Profile::default_doctest(),
};

Things to note: the release Profile actually just corresponds to cargo build --release; there isn't a definition for a check_release field. The function definitions are here:

impl Profile {
pub fn default_dev() -> Profile {
Profile {
debuginfo: Some(2),
debug_assertions: true,
overflow_checks: true,
incremental: true,
..Profile::default()
}
}
pub fn default_release() -> Profile {
Profile {
opt_level: "3".to_string(),
debuginfo: None,
..Profile::default()
}
}
pub fn default_test() -> Profile {
Profile {
test: true,
..Profile::default_dev()
}
}
pub fn default_bench() -> Profile {
Profile {
test: true,
..Profile::default_release()
}
}
pub fn default_doc() -> Profile {
Profile {
doc: true,
..Profile::default_dev()
}
}
pub fn default_custom_build() -> Profile {
Profile {
run_custom_build: true,
..Profile::default_dev()
}
}
pub fn default_check() -> Profile {
Profile {
check: true,
..Profile::default_dev()
}
}
pub fn default_check_test() -> Profile {
Profile {
check: true,
test: true,
..Profile::default_dev()
}
}
pub fn default_doctest() -> Profile {
Profile {
doc: true,
test: true,
..Profile::default_dev()
}
}
}
impl Default for Profile {
fn default() -> Profile {
Profile {
opt_level: "0".to_string(),
lto: Lto::Bool(false),
codegen_units: None,
rustc_args: None,
rustdoc_args: None,
debuginfo: None,
debug_assertions: false,
overflow_checks: false,
rpath: false,
test: false,
doc: false,
run_custom_build: false,
check: false,
panic: None,
incremental: false,
}
}
}

And as you can see, the default Profile for CompileMode::Check is equivalent to the default development Profile, with debug_assertions enabled. There is no Profile for CompileMode::Check that is also for release mode.

@bepzi
Copy link
Author

bepzi commented Mar 21, 2018

I think there are a couple of ways to fix this:

The easy, but not as clean way, would be to define another Profile for CompileMode::Check that also has the correct flags enabled for release, something like:

pub fn default_check_release() -> Profile {
    Profile {
        check: true,
        ..Profile::default_release(),
    }
}

Another way would be to refactor to use something like the Builder Pattern to construct Profiles, doing something like:

let default_profiles: Profiles {
    // ...
    check_release: Profile::default().with_check().with_release(),
    check_test_release: Profile::default().with_check().with_test().with_release(),
    // ...
};

@alexcrichton
Copy link
Member

Thanks for the report @ben01189998819991197253, and I definitely agree with your analysis and proposed fix! Would you be up for sending a PR that does that?

@bepzi
Copy link
Author

bepzi commented Mar 22, 2018

@alexcrichton Sure thing!

Even though it's a bit more work, I'll try to refactor the Profile struct to work with the Builder Pattern - it'll make it easier to add new default profiles and also make it more clear what options are set for each default profile.

@alexcrichton
Copy link
Member

Sounds great!

bors added a commit that referenced this issue Apr 27, 2018
Profile Overrides (RFC #2282 Part 1)

Profile Overrides (RFC #2282 Part 1)

WIP: Putting this up before I dig into writing tests, but should be mostly complete.  I also have a variety of questions below.

This implements the ability to override profiles for dependencies and build scripts.  This includes a general rework of how profiles work internally. Closes #5298.

Profile overrides are available with `profile-overrides` set in `cargo-features` in the manifest.

Part 2 is to implement profiles in config files (to be in a separate PR).

General overview of changes:

- `Profiles` moved to `core/profiles.rs`. All profile selection is centralized there.
- Removed Profile flags `test`, `doc`, `run_custom_build`, and `check`.
- Removed `Profile` from `Unit` and replaced it with two enums: `CompileMode` and `ProfileFor`.  This is the minimum information needed to compute profiles at a later stage.
- Also removed `rustc_args`/`rustdoc_args` from `Profile` and place them in `Context`.  This is currently not very elegant because it is a special case, but it works. An alternate solution I considered was to leave them in the `Profile` and add a special uber-override layer.  Let me know if you think it should change.
- Did some general cleanup in `generate_targets`.

## Misc Fixes
- `cargo check` now honors the `--release` flag.  Fixes #5218.
- `cargo build --test` will set `panic` correctly for dependences. Fixes #5369.
- `cargo check --tests` will no longer include bins twice (once as a normal check, once as a `--test` check).  It only does `--test` check now.
    - Similarly, `cargo check --test name` no longer implicitly checks bins.
- Examples are no longer considered a "test".  (See #5397). Consequences:
    - `cargo test` will continue to build examples as a regular build (no change).
    - `cargo test --tests` will no longer build examples at all.
    - `cargo test --all-targets` will no longer build examples as tests, but instead build them as a regular build (now matches `cargo test` behavior).
    - `cargo check --all-targets` will no longer check examples twice (once as
      normal, once as `--test`).  It now only checks it once as a normal
      target.

## Questions
- Thumbs up/down on the general approach?
- The method to detect if a package is a member of a workspace should probably be redone.  I'm uncertain of the best approach.  Maybe `Workspace.members` could be a set?
- `Hash` and `PartialEq` are implemented manually for `Profile` only to avoid matching on the `name` field.  The `name` field is only there for debug purposes. Is it worth it to keep `name`?  Maybe useful for future use (like #4140)?
- I'm unhappy with the `Finished` line summary that displays `[unoptimized + debuginfo]`.  It doesn't actually show what was compiled.  Currently it just picks the base "dev" or "release" profile.  I'm not sure what a good solution is (to be accurate it would need to potentially display a list of different options).  Is it ok?  (See also #4140 for the wrong profile name being printed.)
- Build-dependencies use different profiles based on whether or not `--release` flag is given.  This means that if you want build-dependencies to always use a specific set of settings, you have to specify both `[profile.dev.build_override]` and `[profile.release.build_override]`.  Is that reasonable (for now)?  I've noticed some issues (like #1774, #2234, #2424) discussing having more control over how build-dependencies are handled.
- `build --bench xxx` or `--benches` builds dependencies with dev profile, which may be surprising.  `--release` does the correct thing.  Perhaps print a warning when using `cargo build` that builds benchmark deps in dev mode?
- Should it warn/error if you have an override for a package that does not exist?
- Should it warn/error if you attempt to set `panic` on the `test` or `bench` profile?

## TODO
- I have a long list of tests to add.
- Address a few "TODO" comments left behind.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants