centralize build stamp logic #135281

onur-ozkan · 2025-01-09T08:56:05Z

This PR brings all the stamp file handling into one place inside build_stamp module, which takes care of everything related to build stamps. By doing this, we cut down on duplicated code and types and keep the codebase easier to maintain and more consistent.

Main goals are:

Make stamp handling stricter so we don't have to pass Paths around and manually join on arbitrary directories
Keep all stamp-related logic in one place
Make it easier to test and debug
Avoid duplication
Keep things simple and well-documented

Resolves #134962

Signed-off-by: onur-ozkan <[email protected]>

rustbot · 2025-01-09T08:56:14Z

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
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 · 2025-01-09T08:56:17Z

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Signed-off-by: onur-ozkan <[email protected]>

rustbot · 2025-01-09T08:58:09Z

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Signed-off-by: onur-ozkan <[email protected]>

jieyouxu · 2025-01-09T09:14:12Z

(This would close #134962)

onur-ozkan · 2025-01-09T09:26:33Z

(This would close #134962)

Funny that I haven't even seen it...

Signed-off-by: onur-ozkan <[email protected]>

rustbot · 2025-01-09T09:41:22Z

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

jieyouxu · 2025-01-09T10:42:41Z

I'll review this tmrw, I wanted to do some more manual local testing just in case. I had a brief glance at the changes, and I must say this is much easier to follow. So thank you for cleaning this up!

klensy · 2025-01-09T11:22:14Z

src/bootstrap/src/utils/build_stamp.rs

+            "prefix can not start or end with '.'"
+        );
+
+        let stamp_filename = self.path.components().last().unwrap().as_os_str().to_str().unwrap();


Maybe https://doc.rust-lang.org/std/path/struct.Path.html#method.file_name?

klensy · 2025-01-09T11:24:54Z

src/bootstrap/src/utils/build_stamp.rs

+    /// Creates a new `BuildStamp` for a given directory.
+    ///
+    /// By default, stamp will be an empty file named `.stamp` within the specified directory.
+    pub fn new(dir: &Path) -> Self {
+        Self { path: dir.join(".stamp"), stamp: String::new() }
+    }


Expecting directory, so maybe assert that it is directory?

klensy · 2025-01-09T11:42:49Z

Maybe add single constructor like with_stamp_and_prefix instead of that 2 methods, or builder instead? Because currently it's possible to call with_stamp and with_prefix more than once, and i don't see why this should be needed at all.

Hmm, ok, multiple calls to with_prefix is used.

klensy · 2025-01-09T12:02:08Z

src/bootstrap/src/core/build_steps/llvm.rs

-    let stamp = out_dir.join("llvm-finished-building");
-    let stamp = HashStamp::new(stamp, Some(smart_stamp_hash));
+    let stamp = BuildStamp::new(&out_dir).with_prefix("llvm").with_stamp(smart_stamp_hash);


Stamp names mentioned in docs https://github.com/search?q=repo%3Arust-lang%2Frust+%22finished-building%22&type=code so need to do something with that.

klensy · 2025-01-09T12:20:36Z

src/bootstrap/src/core/build_steps/check.rs

-        run_cargo(
-            builder,
-            cargo,
-            builder.config.free_args.clone(),
-            &libstd_stamp(builder, compiler, target),
-            vec![],
-            true,
-            false,
-        );
+
+        let stamp = build_stamp::libstd_stamp(builder, compiler, target).with_prefix("check");
+        run_cargo(builder, cargo, builder.config.free_args.clone(), &stamp, vec![], true, false);


Changes in 7b72325 for check.rs will somehow mangle prefix names?
Before: .libstd-check-stamp
After: .check-libstd-stamp

I did this intentionally to keep it simple by avoiding the need to duplicate similar functions. I assume this rename shouldn't cause any issues. Maybe, at worst, it might require running x clean after pulling these changes to remove old build artifacts.

klensy · 2025-01-09T12:30:56Z

Missing stamp refactor for lld:

rust/src/bootstrap/src/core/build_steps/llvm.rs

Lines 1019 to 1022 in b6b8361

    
           let done_stamp = out_dir.join("lld-finished-building"); 
        
           if done_stamp.exists() { 
        
               return out_dir; 
        
           }

onur-ozkan added 9 commits January 9, 2025 08:38

implement BuildStamp that is stricter impl for build stamps

9aed5bb

Signed-off-by: onur-ozkan <[email protected]>

use BuildStamp instead of std paths and strings

dc8a8d9

Signed-off-by: onur-ozkan <[email protected]>

migrate HashStamp to BuildStamp

e9694a1

Signed-off-by: onur-ozkan <[email protected]>

migrate helper stamp functions

7b72325

Signed-off-by: onur-ozkan <[email protected]>

migrate Builder::clear_if_dirty

47a4f0f

Signed-off-by: onur-ozkan <[email protected]>

document build_stamp implementation

a697d15

Signed-off-by: onur-ozkan <[email protected]>

add test coverage for build_stamp implementation

a23e7db

Signed-off-by: onur-ozkan <[email protected]>

migrate generate_smart_stamp_hash

e359f3c

Signed-off-by: onur-ozkan <[email protected]>

fix compiler errors

a685c6b

Signed-off-by: onur-ozkan <[email protected]>

rustbot assigned albertlarsan68 Jan 9, 2025

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 9, 2025

fix an invalid prefix usage on enzyme

2cee8c0

Signed-off-by: onur-ozkan <[email protected]>

add coverage for BuildStamp::with_prefix

e9d3ff7

Signed-off-by: onur-ozkan <[email protected]>

jieyouxu self-assigned this Jan 9, 2025

jieyouxu added the A-bootstrap-stamp Area: bootstrap stamp logic label Jan 9, 2025

onur-ozkan added 2 commits January 9, 2025 12:39

migrate program_out_of_date to BuildStamp::is_up_to_date

b21d860

Signed-off-by: onur-ozkan <[email protected]>

auto label A-bootstrap-stamp

51fd08e

Signed-off-by: onur-ozkan <[email protected]>

onur-ozkan removed the A-bootstrap-stamp Area: bootstrap stamp logic label Jan 9, 2025

onur-ozkan force-pushed the build-stamps branch from f6d3c02 to 51fd08e Compare January 9, 2025 09:41

rustbot added the A-meta Area: Issues & PRs about the rust-lang/rust repository itself label Jan 9, 2025

onur-ozkan added the A-bootstrap-stamp Area: bootstrap stamp logic label Jan 9, 2025

klensy reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

centralize build stamp logic #135281

centralize build stamp logic #135281

onur-ozkan commented Jan 9, 2025 •

edited

Loading

rustbot commented Jan 9, 2025

rustbot commented Jan 9, 2025

rustbot commented Jan 9, 2025

jieyouxu commented Jan 9, 2025

onur-ozkan commented Jan 9, 2025

rustbot commented Jan 9, 2025

jieyouxu commented Jan 9, 2025 •

edited

Loading

klensy Jan 9, 2025

klensy Jan 9, 2025

klensy commented Jan 9, 2025 •

edited

Loading

klensy Jan 9, 2025

klensy Jan 9, 2025

onur-ozkan Jan 9, 2025

klensy commented Jan 9, 2025

centralize build stamp logic #135281

Are you sure you want to change the base?

centralize build stamp logic #135281

Conversation

onur-ozkan commented Jan 9, 2025 • edited Loading

rustbot commented Jan 9, 2025

rustbot commented Jan 9, 2025

rustbot commented Jan 9, 2025

jieyouxu commented Jan 9, 2025

onur-ozkan commented Jan 9, 2025

rustbot commented Jan 9, 2025

jieyouxu commented Jan 9, 2025 • edited Loading

klensy Jan 9, 2025

Choose a reason for hiding this comment

klensy Jan 9, 2025

Choose a reason for hiding this comment

klensy commented Jan 9, 2025 • edited Loading

klensy Jan 9, 2025

Choose a reason for hiding this comment

klensy Jan 9, 2025

Choose a reason for hiding this comment

onur-ozkan Jan 9, 2025

Choose a reason for hiding this comment

klensy commented Jan 9, 2025

onur-ozkan commented Jan 9, 2025 •

edited

Loading

jieyouxu commented Jan 9, 2025 •

edited

Loading

klensy commented Jan 9, 2025 •

edited

Loading