Only try to modify file times of a writable file on Windows #128977

jieyouxu · 2024-08-11T16:07:46Z

Introduces a set_file_times helper which opens a given path as a file in r+w mode on Windows and then sets file times. Previously the file was open as read-only for Windows which caused permission errors locally.

This should hopefully make setting file times less error prone, since trying to set file times on read-only file on Windows also happened in #127850.

try-job: dist-loongarch64-musl
try-job: x86_64-msvc

rustbot · 2024-08-11T16:07:53Z

r? @Kobzol

rustbot has assigned @Kobzol.
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

src/bootstrap/src/lib.rs

Kobzol · 2024-08-11T16:11:00Z

Documented here in the Windows API.

LGTM, feel free to r=me once CI is green.

jieyouxu · 2024-08-11T17:24:07Z

@bors r=@Kobzol rollup

bors · 2024-08-11T17:24:10Z

📌 Commit 13c36f1 has been approved by Kobzol

It is now in the queue for this repository.

Only try to modify file times of a writable file - First commit fixes a failure that I was running into locally trying to build a stage 1 `./x build library --stage 1` on windows due to trying to modify file times of a read-only file. - Second commit introduces a `set_file_times` helper which opens a given path as a file in r+w mode and then sets file times. This should hopefully make setting file times less error prone, since trying to set file times on read-only file also happened in rust-lang#127850. (And apparently it only fails locally on Windows or something weird like that.)

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#128886 (Get rid of some `#[allow(rustc::untranslatable_diagnostic)]`) - rust-lang#128936 (Support reading thin archives in ArArchiveBuilder) - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`) - rust-lang#128977 (Only try to modify file times of a writable file) - rust-lang#128978 (Use `assert_matches` around the compiler more) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-08-12T08:11:04Z

@bors r-

2024-08-12T08:09:07.8523294Z �[1m�[32m Compiling�[0m clap_derive v4.5.5
2024-08-12T08:09:08.7043836Z [RUSTC-TIMING] tracing_attributes test:false 1.637
2024-08-12T08:09:08.7050616Z �[1m�[32m Compiling�[0m thiserror-impl v1.0.61
2024-08-12T08:09:10.1303074Z [RUSTC-TIMING] thiserror_impl test:false 1.419
2024-08-12T08:09:10.1792105Z [RUSTC-TIMING] thiserror test:false 0.043
2024-08-12T08:09:10.1798434Z �[1m�[32m Compiling�[0m tracing v0.1.37
2024-08-12T08:09:10.2561947Z [RUSTC-TIMING] clap_derive test:false 2.397
2024-08-12T08:09:10.2568760Z �[1m�[32m Compiling�[0m clap v4.5.7
2024-08-12T08:09:10.2983590Z [RUSTC-TIMING] clap test:false 0.034
2024-08-12T08:09:10.5155182Z [RUSTC-TIMING] tracing test:false 0.330
2024-08-12T08:09:10.5173792Z �[1m�[32m Compiling�[0m llvm-bitcode-linker v0.0.1 (/checkout/src/tools/llvm-bitcode-linker)
2024-08-12T08:09:11.0202785Z [RUSTC-TIMING] llvm_bitcode_linker test:false 0.498
2024-08-12T08:09:11.1082040Z [RUSTC-TIMING] clap_builder test:false 8.264
2024-08-12T08:09:12.3069458Z [RUSTC-TIMING] llvm_bitcode_linker test:false 1.194
2024-08-12T08:09:12.3083923Z �[1m�[32m Finished�[0m release profile [optimized] target(s) in 11.09s
2024-08-12T08:09:12.3229000Z ##[endgroup]
2024-08-12T08:09:12.3230751Z [TIMING] core::build_steps::tool::LlvmBitcodeLinker { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu, extra_features: [] } -- 11.115
2024-08-12T08:09:12.3233162Z [TIMING] core::build_steps::compile::Assemble { target_compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu } } -- 0.003
2024-08-12T08:09:12.3234606Z [TIMING] core::build_steps::compile::StartupObjects { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu }, target: loongarch64-unknown-linux-musl } -- 0.000
2024-08-12T08:09:12.3236187Z [TIMING] core::builder::Builder::sysroot_libdir::Libdir { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu }, target: loongarch64-unknown-linux-musl } -- 0.000
2024-08-12T08:09:12.3278104Z thread 'main' panicked at src/lib.rs:1794:13:
2024-08-12T08:09:12.3279076Z set_file_times(dst, file_times) failed with Permission denied (os error 13)
2024-08-12T08:09:12.3279792Z note: run with RUST_BACKTRACE=1 environment variable to display a backtrace
2024-08-12T08:09:12.3301323Z Build completed unsuccessfully in 0:16:13
2024-08-12T08:09:12.3381725Z local time: Mon Aug 12 08:09:12 UTC 2024
2024-08-12T08:09:12.4135264Z network time: Mon, 12 Aug 2024 08:09:12 GMT
2024-08-12T08:09:12.6295762Z ##[error]Process completed with exit code 1.
2024-08-12T08:09:12.6402805Z Post job cleanup.

jieyouxu · 2024-08-12T08:49:00Z

... wat

Kobzol · 2024-08-12T09:04:52Z

It's possible that some files are not writable on Linux (not sure if it's because of Docker or something else), but it was possible to set their modification times (?). Would be useful to see the path for which the mtime modification failed.

ChrisDenton · 2024-08-12T09:14:14Z

The code that failed is this section:

rust/src/bootstrap/src/lib.rs

Lines 1785 to 1794 in 13c36f1

 if let Err(e) = fs::copy(&src, dst) { 

 panic!("failed to copy `{}` to `{}`: {}", src.display(), dst.display(), e) 

 } 

 t!(fs::set_permissions(dst, metadata.permissions())); 

 let file_times = fs::FileTimes::new() 

 .set_accessed(t!(metadata.accessed())) 

 .set_modified(t!(metadata.modified())); 

 t!(set_file_times(dst, file_times));

It should be writeable because the file was just copied. But setting the permissions may make it unable to be reopen with write access.

jieyouxu · 2024-08-12T09:18:59Z

Hm, my money is currently on read-only things (on CI) that I am trying to open as writable. I'll run a try-job to see which path causes the failure.

jieyouxu · 2024-08-12T09:19:35Z

@bors try

Only try to modify file times of a writable file - First commit fixes a failure that I was running into locally trying to build a stage 1 `./x build library --stage 1` on windows due to trying to modify file times of a read-only file. - Second commit introduces a `set_file_times` helper which opens a given path as a file in r+w mode and then sets file times. This should hopefully make setting file times less error prone, since trying to set file times on read-only file also happened in rust-lang#127850. (And apparently it only fails locally on Windows or something weird like that.) try-job: dist-loongarch64-musl

bors · 2024-08-12T09:20:45Z

⌛ Trying commit 73132a9 with merge edbc734...

Kobzol · 2024-08-12T09:29:33Z

The code that failed is this section:

rust/src/bootstrap/src/lib.rs

Lines 1785 to 1794 in 13c36f1

if let Err(e) = fs::copy(&src, dst) {

panic!("failed to copy `{}` to `{}`: {}", src.display(), dst.display(), e)

}

t!(fs::set_permissions(dst, metadata.permissions()));

let file_times = fs::FileTimes::new()

.set_accessed(t!(metadata.accessed()))

.set_modified(t!(metadata.modified()));

t!(set_file_times(dst, file_times));

It should be writeable because the file was just copied. But setting the permissions may make it unable to be reopen with write access.

That's a good point. Maybe just reordering the file time modification and the permissions setting could fix it?

jieyouxu · 2024-08-12T09:32:22Z

That's a good point. Maybe just reordering the file time modification and the permissions setting could fix it?

(We can't do that AFAICT because as I understand it the set file times is there because chmod changes file times, not sure about windows)

Only try to modify file times of a writable file on Windows Introduces a `set_file_times` helper which opens a given path as a file in r+w mode on Windows and then sets file times. Previously the file was open as read-only for Windows which caused permission errors locally. This should hopefully make setting file times less error prone, since trying to set file times on read-only file on Windows also happened in rust-lang#127850. try-job: dist-loongarch64-musl try-job: x86_64-msvc

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#128064 (Improve docs for Waker::noop and LocalWaker::noop) - rust-lang#128922 (rust-analyzer: use in-tree `pattern_analysis` crate) - rust-lang#128965 (Remove `print::Pat` from the printing of `WitnessPat`) - rust-lang#128977 (Only try to modify file times of a writable file on Windows) - rust-lang#129018 (Migrate `rlib-format-packed-bundled-libs` and `native-link-modifier-bundle` `run-make` tests to rmake) - rust-lang#129037 (Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake) - rust-lang#129078 (`ParamEnvAnd::fully_perform`: we have an `ocx`, use it) - rust-lang#129110 (Add a comment explaining the return type of `Ty::kind`.) - rust-lang#129111 (Port the `sysroot-crates-are-unstable` Python script to rmake) - rust-lang#129135 (crashes: more tests) r? `@ghost` `@rustbot` modify labels: rollup

workingjubilee · 2024-08-16T01:43:14Z

almost certainly failed in #129142

workingjubilee · 2024-08-16T01:58:10Z

@bors r-

jieyouxu · 2024-08-16T06:56:26Z

Ah there's a new Flags wrapper struct

jieyouxu · 2024-08-16T06:56:41Z

@rustbot ready

Kobzol · 2024-08-16T07:24:03Z

It's not new, it's just now required to pass it to Config, instead of the raw args. The Config struct will receive further refactorings in the near future =D

Thanks!

@bors r+

bors · 2024-08-16T07:24:06Z

📌 Commit 26fae1e has been approved by Kobzol

It is now in the queue for this repository.

jieyouxu · 2024-08-16T07:26:04Z

The Config struct will receive further refactorings in the near future =D

Looking forward to it 👀

bors · 2024-08-16T09:24:20Z

⌛ Testing commit 26fae1e with merge 8fbdc04...

bors · 2024-08-16T11:51:30Z

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 8fbdc04 to master...

rust-timer · 2024-08-16T13:09:01Z

Finished benchmarking commit (8fbdc04): 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 -2.6%, secondary 1.0%)

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)	4.7%	[4.7%, 4.7%]	1
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Cycles

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

Binary size

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

Bootstrap: 749.902s -> 751.027s (0.15%)
Artifact size: 339.17 MiB -> 339.18 MiB (0.00%)

rustbot assigned Kobzol Aug 11, 2024

rustbot added 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 Aug 11, 2024

jieyouxu commented Aug 11, 2024

View reviewed changes

src/bootstrap/src/lib.rs Show resolved Hide resolved

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 Aug 11, 2024

matthiaskrgr mentioned this pull request Aug 12, 2024

Rollup of 6 pull requests #128996

Closed

matthiaskrgr mentioned this pull request Aug 12, 2024

Rollup of 5 pull requests #128997

Closed

Kobzol mentioned this pull request Aug 12, 2024

Slightly refactor Flags in bootstrap #128878

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2024

rust-cloud-vms bot force-pushed the writable-file branch from 13c36f1 to 73132a9 Compare August 12, 2024 09:18

This comment has been minimized.

Sign in to view

workingjubilee mentioned this pull request Aug 16, 2024

Rollup of 10 pull requests #129141

Closed

workingjubilee mentioned this pull request Aug 16, 2024

Rollup of 10 pull requests #129142

Closed

workingjubilee closed this Aug 16, 2024

workingjubilee reopened this Aug 16, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 16, 2024

This comment has been minimized.

Sign in to view

bootstrap: fix trying to modify file times on read-only file on Windows

26fae1e

rust-cloud-vms bot force-pushed the writable-file branch from 2e7e619 to 26fae1e Compare August 16, 2024 06:55

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2024

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 Aug 16, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 16, 2024

bors merged commit 8fbdc04 into rust-lang:master Aug 16, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 16, 2024

jieyouxu deleted the writable-file branch August 16, 2024 11:55

mati865 mentioned this pull request Sep 4, 2024

rust: update to 1.81.0 msys2/MINGW-packages#21847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only try to modify file times of a writable file on Windows #128977

Only try to modify file times of a writable file on Windows #128977

jieyouxu commented Aug 11, 2024 •

edited

Loading

rustbot commented Aug 11, 2024

Kobzol commented Aug 11, 2024

jieyouxu commented Aug 11, 2024

bors commented Aug 11, 2024

matthiaskrgr commented Aug 12, 2024

jieyouxu commented Aug 12, 2024

Kobzol commented Aug 12, 2024

ChrisDenton commented Aug 12, 2024

jieyouxu commented Aug 12, 2024 •

edited

Loading

jieyouxu commented Aug 12, 2024

bors commented Aug 12, 2024

Kobzol commented Aug 12, 2024 •

edited

Loading

This comment has been minimized.

jieyouxu commented Aug 12, 2024

workingjubilee commented Aug 16, 2024

workingjubilee commented Aug 16, 2024

This comment has been minimized.

jieyouxu commented Aug 16, 2024

jieyouxu commented Aug 16, 2024

Kobzol commented Aug 16, 2024

bors commented Aug 16, 2024

jieyouxu commented Aug 16, 2024

bors commented Aug 16, 2024

bors commented Aug 16, 2024

rust-timer commented Aug 16, 2024

Only try to modify file times of a writable file on Windows #128977

Only try to modify file times of a writable file on Windows #128977

Conversation

jieyouxu commented Aug 11, 2024 • edited Loading

rustbot commented Aug 11, 2024

Kobzol commented Aug 11, 2024

jieyouxu commented Aug 11, 2024

bors commented Aug 11, 2024

matthiaskrgr commented Aug 12, 2024

jieyouxu commented Aug 12, 2024

Kobzol commented Aug 12, 2024

ChrisDenton commented Aug 12, 2024

jieyouxu commented Aug 12, 2024 • edited Loading

jieyouxu commented Aug 12, 2024

bors commented Aug 12, 2024

Kobzol commented Aug 12, 2024 • edited Loading

This comment has been minimized.

jieyouxu commented Aug 12, 2024

workingjubilee commented Aug 16, 2024

workingjubilee commented Aug 16, 2024

This comment has been minimized.

jieyouxu commented Aug 16, 2024

jieyouxu commented Aug 16, 2024

Kobzol commented Aug 16, 2024

bors commented Aug 16, 2024

jieyouxu commented Aug 16, 2024

bors commented Aug 16, 2024

bors commented Aug 16, 2024

rust-timer commented Aug 16, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

jieyouxu commented Aug 11, 2024 •

edited

Loading

jieyouxu commented Aug 12, 2024 •

edited

Loading

Kobzol commented Aug 12, 2024 •

edited

Loading