Skip to content

Recompile on profile rustflag changes #11121

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Recompile on profile rustflag changes #11121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4ff787
the fix
Hezuikn Sep 21, 2022
1b2ce33
update comment
Hezuikn Sep 21, 2022
d8449e9
remove hazard by using derivative(procmacro crate) instead of handwri…
Hezuikn Sep 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ unicode-width = "0.1.5"
openssl = { version = '0.10.11', optional = true }
im-rc = "15.0.0"
itertools = "0.10.0"
derivative = "2.2.0"

# A noop dependency that changes in the Rust repository, it's a bit of a hack.
# See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust`
Expand Down
45 changes: 8 additions & 37 deletions src/cargo/core/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::util::{closest_msg, config, CargoResult, Config};
use anyhow::{bail, Context as _};
use std::collections::{BTreeMap, HashMap, HashSet};
use std::hash::Hash;
use std::{cmp, env, fmt, hash};
use std::{env, fmt};

/// Collection of all profiles.
#[derive(Clone, Debug)]
Expand Down Expand Up @@ -539,10 +539,16 @@ pub enum ProfileRoot {

/// Profile settings used to determine which compiler flags to use for a
/// target.
#[derive(Clone, Eq, PartialOrd, Ord, serde::Serialize)]
#[derive(Clone, Eq, PartialOrd, Ord, serde::Serialize, derivative::Derivative)]
Copy link
Contributor

Choose a reason for hiding this comment

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

When adding a dependency, please expand on why it is helpful and why it is worth taking on.

In particular, this is adding a dependency in place of hand-implementing one Hash and one PartialEq. That seems like a fairly small scope that I would expect the dependency to be pulling a lot of weight.

Copy link
Author

Choose a reason for hiding this comment

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

i don't feel code like that should be allowed to exist

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't feel code like that should be allowed to exist

Please expand.

Communication is critical in open source, especially when maintains have limited availability. So far the communication has been pretty minimal between this comment, this PR, and #11120.

I can make my own guesses and some of them might be quite valid but that is a one sided conversation with myself. I'm wanting to understand your intentions and reasoning.

/// Don't compare/hash fields which wont affect compilation.
/// This is necessary for `Unit` deduplication for things like "test" and
/// "dev" which are essentially the same.
#[derivative(Hash, PartialEq)]
pub struct Profile {
#[derivative(Hash = "ignore", PartialEq = "ignore")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you re-do the commits so that one commit is the refactor with the other commit having the fix? And describe the problem and the fix in the PR?

This is putting the burden on the reviewer to figure out what is happening with this and the team is already strapped for availability.

pub name: InternedString,
pub opt_level: InternedString,
#[derivative(Hash = "ignore", PartialEq = "ignore")]
#[serde(skip)] // named profiles are unstable
pub root: ProfileRoot,
pub lto: Lto,
Expand Down Expand Up @@ -620,21 +626,6 @@ impl fmt::Display for Profile {
}
}

impl hash::Hash for Profile {
fn hash<H>(&self, state: &mut H)
where
H: hash::Hasher,
{
self.comparable().hash(state);
}
}

impl cmp::PartialEq for Profile {
fn eq(&self, other: &Self) -> bool {
self.comparable() == other.comparable()
}
}

impl Profile {
fn default_dev() -> Profile {
Profile {
Expand All @@ -656,26 +647,6 @@ impl Profile {
..Profile::default()
}
}

/// Compares all fields except `name`, which doesn't affect compilation.
/// This is necessary for `Unit` deduplication for things like "test" and
/// "dev" which are essentially the same.
fn comparable(&self) -> impl Hash + Eq {
(
self.opt_level,
self.lto,
self.codegen_backend,
self.codegen_units,
self.debuginfo,
self.split_debuginfo,
self.debug_assertions,
self.overflow_checks,
self.rpath,
self.incremental,
self.panic,
self.strip,
)
}
}

/// The link-time-optimization setting.
Expand Down