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

Avoid replacing the definition of CURRENT_RUSTC_VERSION #135173

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Avoid replacing the definition of CURRENT_RUSTC_VERSION #135173

merged 1 commit into from
Jan 7, 2025

Conversation

pietroalbini
Copy link
Member

Before this PR, replace-version-placeholder hardcoded the path defining CURRENT_RUSTC_VERSION (to avoid replacing it). After a refactor moved the file defining it without changing the hardcoded path, the tool started replacing the constant itself with the version number.

To avoid this from happening in the future, this changes the definition of the constant to avoid the tool from ever matching it.

r? @workingjubilee

@pietroalbini
avoid replacing the definition of CURRENT_RUSTC_VERSION
80cdaea 
Before this commit, replace-version-placeholder hardcoded the path
defining CURRENT_RUSTC_VERSION (to avoid replacing it). After a refactor
moved the file defining it without changing the hardcoded path, the tool
started replacing the constant itself with the version number.

To avoid this from happening in the future, this changes the definition
of the constant to avoid the tool from ever matching it.
@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2025

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 6, 2025
workingjubilee
workingjubilee approved these changes Jan 6, 2025
View reviewed changes
compiler/rustc_attr_data_structures/src/stability.rs
@@ -9,7 +9,12 @@ use crate::RustcVersion;
/// `since` field of the `#[stable]` attribute.
///
/// For more, see [this pull request](https://github.com/rust-lang/rust/pull/100591).
pub const VERSION_PLACEHOLDER: &str = "CURRENT_RUSTC_VERSION";
pub const VERSION_PLACEHOLDER: &str = concat!("CURRENT_RUSTC_VERSIO", "N");
Copy link
Member

Choose a reason for hiding this comment

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

lmao

compiler/rustc_attr_data_structures/src/stability.rs
Comment on lines +13 to +15
// Note that the `concat!` macro above prevents `src/tools/replace-version-placeholder` from
// replacing the constant with the current version. Hardcoding the tool to skip this file doesn't
// work as the file can (and at some point will) be moved around.
Copy link
Member

Choose a reason for hiding this comment

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

lmaoooo

compiler/rustc_attr_data_structures/src/stability.rs
// replacing the constant with the current version. Hardcoding the tool to skip this file doesn't
// work as the file can (and at some point will) be moved around.
//
// Turning the `concat!` macro into a string literal will make Pietro cry. That'd be sad :(
Copy link
Member

Choose a reason for hiding this comment

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

don't cry Pietro :(

src/tools/replace-version-placeholder/src/main.rs
// for their operation
|| path.ends_with("compiler/rustc_attr/src/builtin.rs")
},
|path, _is_dir| walk::filter_dirs(path),
Copy link
Member

Choose a reason for hiding this comment

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

oh nice, it even makes the actual code simpler.

@workingjubilee
Copy link
Member

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Jan 6, 2025

📌 Commit 80cdaea has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2025
@lqd
Copy link
Member

lqd commented Jan 6, 2025

Why not ignore the lines that contain the "VERSION_PLACEHOLDER" definition itself 🤔

@workingjubilee
Copy link
Member

that seems way more context-sensitive therefore fragile than just having a slightly cursed string constant

@pietroalbini
Copy link
Member Author

Why not ignore the lines that contain the "VERSION_PLACEHOLDER" definition itself 🤔

I kinda don't want someone refactoring that variable name from accidentally regressing this, which could happen without reading the comment if someone uses the rename assist in rust-analyzer. This was a fairly painful failure to track down tbh, I don't want it to happen again.

@pietroalbini
Copy link
Member Author

I was thinking of implementing something like // skip-replacing-placeholder to add to the same line, but it would've made the replacing code way more complex than it is right now. If we really want to keep the constant clean I can do that, but IMO it adds a lot of complexity for little benefit.

@workingjubilee
Copy link
Member

I like the gross concat!, it's an obvious "what the fuck, why?"

@bors
Copy link
Contributor

bors commented Jan 6, 2025

⌛ Testing commit 80cdaea with merge a8f7913...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2025
@bors
Auto merge of rust-lang#135173 - pietroalbini:pa-fix-rvp, r=workingju…
a8f7913 
…bilee

Avoid replacing the definition of `CURRENT_RUSTC_VERSION`

Before this PR, replace-version-placeholder hardcoded the path defining CURRENT_RUSTC_VERSION (to avoid replacing it). After a refactor moved the file defining it without changing the hardcoded path, the tool started replacing the constant itself with the version number.

To avoid this from happening in the future, this changes the definition of the constant to avoid the tool from ever matching it.

r? `@workingjubilee`
@pietroalbini
Copy link
Member Author

@bors retry

Yield priority to the stable release.

@bors
Copy link
Contributor

bors commented Jan 7, 2025

⌛ Testing commit 80cdaea with merge fb546ee...

@bors
Copy link
Contributor

bors commented Jan 7, 2025

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing fb546ee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 7, 2025
@bors bors merged commit fb546ee into rust-lang:master Jan 7, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 7, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fb546ee): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 3.5% [3.5%, 3.5%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.5% [3.5%, 3.5%] 1

Cycles

Results (primary -3.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -3.3% [-3.3%, -3.3%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -3.3% [-3.3%, -3.3%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 764.114s -> 764.15s (0.00%)
Artifact size: 325.66 MiB -> 325.71 MiB (0.02%)

@pietroalbini pietroalbini deleted the pa-fix-rvp branch January 7, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees

@workingjubilee workingjubilee

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
7 participants
@pietroalbini @rustbot @workingjubilee @bors @lqd @rust-timer @estebank