Skip to content

Correctly set filetime for copied LLVM #57871

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

Merged
merged 1 commit into from
Jan 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ use std::cell::{RefCell, Cell};
use std::collections::{HashSet, HashMap};
use std::env;
use std::fs::{self, OpenOptions, File};
use std::io::{self, Seek, SeekFrom, Write, Read};
use std::io::{Seek, SeekFrom, Write, Read};
use std::path::{PathBuf, Path};
use std::process::{self, Command};
use std::slice;
Expand Down Expand Up @@ -1263,9 +1263,15 @@ impl Build {
if !src.exists() {
panic!("Error: File \"{}\" not found!", src.display());
}
let mut s = t!(fs::File::open(&src));
let mut d = t!(fs::File::create(&dst));
io::copy(&mut s, &mut d).expect("failed to copy");
let metadata = t!(src.symlink_metadata());
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 atime = FileTime::from_last_access_time(&metadata);
let mtime = FileTime::from_last_modification_time(&metadata);
t!(filetime::set_file_times(&dst, atime, mtime));
Copy link
Contributor

Choose a reason for hiding this comment

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

So... if I'm seeing this right, the only actual change of this PR is this file.

Let me see if I get this right:

The old code copied e.g. rustc over from where it was built in stage 1 to where it is used to build the tests. This copy operation nuked all metadata and timestamps, thus causing compiletest to think rustc changed and causing a full rebuild.

The new code instead reads the metadata, copies the file and writes back the relevant old metadata. So even if there's a new file, its timestamp didn't change.

If I got everything right, please add a few comments explaining why we don't just do fs::copy and be done with it. r=me with that

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, fs::copy doesn't copy the timestamp (just the permission bits); this code is copied from the copy method on Build (in this file) anyway, so I'd prefer to not modify it in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify, the reason we don't call that copy method instead is that it prefers to create a hardlink instead of copying if it can, and we don't want that here.

}
chmod(&dst, perms);
}
Expand Down
54 changes: 35 additions & 19 deletions src/tools/compiletest/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,15 +669,6 @@ fn stamp(config: &Config, testpaths: &TestPaths, revision: Option<&str>) -> Path
output_base_dir(config, testpaths, revision).join("stamp")
}

/// Return an iterator over timestamps of files in the directory at `path`.
fn collect_timestamps(path: &PathBuf) -> impl Iterator<Item=FileTime> {
WalkDir::new(path)
.into_iter()
.map(|entry| entry.unwrap())
.filter(|entry| entry.file_type().is_file())
.map(|entry| mtime(entry.path()))
}

fn up_to_date(
config: &Config,
testpaths: &TestPaths,
Expand All @@ -700,13 +691,15 @@ fn up_to_date(
let rust_src_dir = config
.find_rust_src_root()
.expect("Could not find Rust source root");
let stamp = mtime(&stamp_name);
let mut inputs = vec![mtime(&testpaths.file), mtime(&config.rustc_path)];
let stamp = Stamp::from_path(&stamp_name);
let mut inputs = vec![Stamp::from_path(&testpaths.file), Stamp::from_path(&config.rustc_path)];
inputs.extend(
props
.aux
.iter()
.map(|aux| mtime(&testpaths.file.parent().unwrap().join("auxiliary").join(aux))),
.map(|aux| {
Stamp::from_path(&testpaths.file.parent().unwrap().join("auxiliary").join(aux))
}),
);
// Relevant pretty printer files
let pretty_printer_files = [
Expand All @@ -717,24 +710,47 @@ fn up_to_date(
"src/etc/lldb_rust_formatters.py",
];
inputs.extend(pretty_printer_files.iter().map(|pretty_printer_file| {
mtime(&rust_src_dir.join(pretty_printer_file))
Stamp::from_path(&rust_src_dir.join(pretty_printer_file))
}));
inputs.extend(collect_timestamps(&config.run_lib_path));
inputs.extend(Stamp::from_dir(&config.run_lib_path));
if let Some(ref rustdoc_path) = config.rustdoc_path {
inputs.push(mtime(&rustdoc_path));
inputs.push(mtime(&rust_src_dir.join("src/etc/htmldocck.py")));
inputs.push(Stamp::from_path(&rustdoc_path));
inputs.push(Stamp::from_path(&rust_src_dir.join("src/etc/htmldocck.py")));
}

// UI test files.
inputs.extend(UI_EXTENSIONS.iter().map(|extension| {
let path = &expected_output_path(testpaths, revision, &config.compare_mode, extension);
mtime(path)
Stamp::from_path(path)
}));

// Compiletest itself.
inputs.extend(collect_timestamps(&rust_src_dir.join("src/tools/compiletest/")));
inputs.extend(Stamp::from_dir(&rust_src_dir.join("src/tools/compiletest/")));

inputs.iter().any(|input| *input > stamp)
inputs.iter().any(|input| input > &stamp)
}

#[derive(Debug, PartialEq, PartialOrd, Ord, Eq)]
struct Stamp {
time: FileTime,
file: PathBuf,
}

impl Stamp {
fn from_path(p: &Path) -> Self {
Stamp {
time: mtime(&p),
file: p.into(),
}
}

fn from_dir(path: &Path) -> impl Iterator<Item=Stamp> {
WalkDir::new(path)
.into_iter()
.map(|entry| entry.unwrap())
.filter(|entry| entry.file_type().is_file())
.map(|entry| Stamp::from_path(entry.path()))
}
}

fn mtime(path: &Path) -> FileTime {
Expand Down
3 changes: 2 additions & 1 deletion src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1998,7 +1998,8 @@ impl<'test> TestCx<'test> {

fn fatal(&self, err: &str) -> ! {
self.error(err);
panic!();
error!("fatal error, panic: {:?}", err);
panic!("fatal error");
}

fn fatal_proc_rec(&self, err: &str, proc_res: &ProcRes) -> ! {
Expand Down