-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Do not warn about missing change ID in tarball builds #145780
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
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put the check as a guard like the other guard above, + some comment.
r=me with that
👍 @bors r=lqd rollup |
Do not warn about missing change ID in tarball builds Fixes: rust-lang#145778 r? `@jieyouxu`
Rollup of 7 pull requests Successful merges: - #144452 (std/sys/fd: Relax `READ_LIMIT` on Darwin) - #145307 (Fix `LazyLock` poison panic message) - #145515 (Optimize `char::encode_utf8`) - #145540 (interpret/allocation: get_range on ProvenanceMap) - #145774 (Remove default opts from config) - #145780 (Do not warn about missing change ID in tarball builds) - #145781 (Remove profile section from Clippy) r? `@ghost` `@rustbot` modify labels: rollup
@@ -198,14 +198,16 @@ fn check_version(config: &Config) -> Option<String> { | |||
Some(ChangeId::Id(id)) if id == latest_change_id => return None, | |||
Some(ChangeId::Ignore) => return None, | |||
Some(ChangeId::Id(id)) => id, | |||
None => { | |||
// The warning is only useful for development, not release tarballs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty strong assertion, which I'm not sure distros agree with, at least Archlinux uses the change-id
in their build, and they do update it for every new version.
Same for debian, config.toml.in
.
Furthermore users can just put ignore
if they don't want it.
Hm, I think I agree with Urgau's assessment re. this is used by distros. Maybe we should just print a warning or document this somewhere if someone wants to disable change-id for dist? @bors r- (unsettled design question) |
As I wrote here, I also think that distros do this, but I don't think it's common enough that we have to warn them if it's unset. So I would keep the implementation, and just update the source code comment. WDYT? |
I don't feel too strongly about this, but I have a weak preference towards being more "upfront". I.e. for users trying to produce distributions, seeing the warning is arguably a good thing -- I think that if they don't want to care about |
As in, my first reaction seeing the report in #145778 is apart from the fatal not a git repo error, seeing the |
I think that a "clean build" from sources should ideally produce as few warnings as possible. That being said, I also see your point. I'll leave it up to you. If you don't find this worthy, then let's close both this PR and the original issue. |
I have a weak preference towards "not-a-bug", though we can probably improve the docs around it. |
I agree in the general sense, but |
Ok, fair enough! |
Fixes: #145778
r? @jieyouxu