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.

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

Jump to bottom

Recompile on profile rustflag changes #11121

Closed
wants to merge 3 commits into from
Closed

Recompile on profile rustflag changes #11121

wants to merge 3 commits into from

Conversation

Hezuikn
Copy link

@Hezuikn Hezuikn commented Sep 21, 2022 •
edited
Loading

turns out rustflags effect compilation 4head
Fixes #11120

@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.

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 Sep 21, 2022
@Hezuikn Hezuikn changed the title profile rustflags fix Fixes #11120 profile rustflags fix Sep 21, 2022
@Hezuikn Hezuikn mentioned this pull request Sep 21, 2022
Closed
@epage epage changed the title profile rustflags fix Recompile on profile rustflag changes Sep 21, 2022
epage
epage reviewed Sep 21, 2022
View reviewed changes
src/cargo/core/profiles.rs
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.

epage
epage reviewed Sep 21, 2022
View reviewed changes
src/cargo/core/profiles.rs
@@ -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.

@Hezuikn Hezuikn force-pushed the profile-rustflags-fix branch from 681775b to 3b3f447 Compare September 21, 2022 18:10
@Hezuikn Hezuikn force-pushed the profile-rustflags-fix branch from 3b3f447 to d8449e9 Compare September 21, 2022 18:41
@weihanglo weihanglo 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 Sep 23, 2022
@weihanglo
Copy link
Member

Ping @Hezuikn. Just checking in to see if you are still interested in working on this. As epage said, a good conversation often leads to a better context of understanding to the solution. If you need helps, comment here and we could take a look.

@Hezuikn
Copy link
Author

Hezuikn commented Dec 6, 2022

yes! i am

@bors
Copy link
Contributor

bors commented Feb 15, 2023

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

@weihanglo
Copy link
Member

Thank you for your interest in this. However, I'm going to close due to inactivity. Feel free to open a new one if you have a chance to work on it again.

@weihanglo weihanglo closed this Mar 2, 2023
@Systemcluster Systemcluster mentioned this pull request Apr 16, 2023
Merged
bors added a commit that referenced this pull request Apr 18, 2023
@bors
Auto merge of #11981 - Systemcluster:rustflags-comparable, r=epage
60bf28f 
Recompile on profile rustflags changes

Adding `rustflags` to the comparable profile properties.

Follow-up to #11121 without the additional changes.

Closes #11120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@ehuss ehuss

Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

profile-rustflags doesnt make cargo recompile the thing
6 participants
@Hezuikn @rust-highfive @weihanglo @bors @epage @ehuss