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.

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

Rebuild on mid build file modification #6484

Merged
merged 7 commits into from Jan 7, 2019

Conversation

Projects
None yet
6 participants
@Eh2406
Copy link
Contributor

Eh2406 commented Dec 25, 2018

This is an attempt to Fixes #2426, it is based on the test @dwijnand made for #5417.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Dec 25, 2018

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from ca3efdc to 6e47199 Dec 26, 2018

@Eh2406 Eh2406 changed the title [wip] Rebuild on mid build file modification Rebuild on mid build file modification Dec 26, 2018

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from 1679901 to 6e47199 Dec 27, 2018

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Dec 27, 2018

That clean up mess things up. I forced pushed to the previous one. CI is green.

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Dec 27, 2018

LGTM, but I thought I saw @ehuss exhibit concern or at the very least ask about whether a dedicated file is necessary, on Discord, but I can't find it now and I might've got my patches mixed up. Looking for a second opinion 😄

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Dec 27, 2018

The concern was raised on my other PR adding a timestamp file.

For this PR I do not see a way to do it without a new file. (Assuming we want to gen the mtime from a file, as described hear, ruffly that the file system may be on a different clock then Systomtime::new().) At the start of the compile we do not yet have the info we need to write any of the existing files, so we need to put filler content into whichever file we make. After the build, if we override the the file then we will lose the mtime we care about.

Now that I am thinking about it, this may now have a new problem.

  1. Make input files at t = 0
  2. Start build, making a timestamp at t = 1
  3. Rustc terminated without writing an output file.
  4. Start a new build, the input files all have a mtime less then the timestamp, so no rebuild is needed.

The fix is eazy we use the min(timestamp.mtime, output.mtime), but I don't know how to write a test.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Dec 28, 2018

Now that I am thinking about it, this may now have a new problem.

I don't think I understand your concern. It will still rebuild the target because the new, computed fingerprint's hash does not match the original fingerprint hash. And the fingerprint isn't saved if it fails. I think the main consequences is that the log output won't be correct (the "stale" entries, and maybe the compare() mtime checking).

Show resolved Hide resolved src/cargo/core/compiler/fingerprint.rs Outdated
Show resolved Hide resolved tests/testsuite/build.rs Outdated

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from 7317dbb to ec9dfc0 Dec 28, 2018

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Dec 28, 2018

I moved the test, and removed the min.
Does that look better?

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Dec 28, 2018

I've been kicking this around, and I can't find any problems. However, the fingerprint stuff is pretty subtle so I'd like it if Alex could take a look before merging.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 2, 2019

Thanks for this @Eh2406! I've only got a few concerns about this:

  • I think the foo.with_file_name("invoked.timestamp") pattern may want to be refactored a bit to reduce duplication throughout. I was also initially worried that with the same file name this would cause a lot of targets to collide onto the same file, but I think these files we're making are all in the .fingerprint folder in unique directory names already? I seem to recall though that multiple targets for one package go into the same directory...

  • I'm a little worried about the implications for this on low-resolution timestamp filesystems. I think this means that if your filesystem has a timestamp resolution of N time units then if a build script or rustc executes in less than N time units you'll basically always be rebuilding, right? The previous behavior I believe is that you'd get one spurious recompile but it'd be stable after that, but this may be bad that it keeps rebuilding.

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 2, 2019

refactored a bit to reduce duplication throughout.

Good point. I was trying to limit the state that needed to be passed around. Do you think a helper function would look better? Or, should I add a timestamp_path arg to keep track of it? Or, is there something else you had in mind?

.fingerprint folder in unique directory names already?

I think the build-script one may be in the build folder, but still in a directory with metadata hash in the name.

I seem to recall though that multiple targets for one package go into the same directory...

Interesting! I am new to this part of the code, so I can easily miss this kind of corner case. Do you have a test you can point me to, or some way to reproduce this behavior?

I'm a little worried about the implications for this on low-resolution timestamp filesystems.

Me to. I am hoping that someone with access to one can test whether we get this correct. That being said I don't see how this ends up infinitely looping.

  • 1.1 file modified with a mtime of 1
  • 1.2 build started, making a timestamp with a mtime of 1
  • 1.3 file modified with a mtime of 1 (this may or may not happen, the important part is that the code has to assume it did.)
  • 2.1 a new build is started, it sees that the mtime of the file == the mtime of the timestamp so it has to assume that 1.3 happened and starts compiling, making a timestamp with a mtime of 2
  • 3.1 a new build is started, it sees that the mtime of the file < the mtime of the timestamp so it uses the existing artifacts.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 2, 2019

☔️ The latest upstream changes (presumably #6493) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 2, 2019

I ran some tests on HFS (1 second resolution) on nightly vs this PR. The test is touch src/lib.rs && cargo build && cargo build && ..., and counting how many times cargo build is run before it considers everything fresh. For medium/large projects (like cargo), I didn't see any difference — it always builds just once as expected. Only for small projects (like bitflags or "hello world") does it run fast enough to rebuild multiple times. With an empty lib project, here's the comparison:

Tries 6484 nightly
1 3 0
2 0 1
3 25 9
4 65 44
5 629 748
6 258 191
7 20 7

You can read this chart as on nightly, 748 out of 1000 attempts cargo build ran 5 times before it was considered fresh (~75% of the time). They numbers are roughly similar, although this PR has a wider spread/variance. I'd like to dig a little more to better understand why.

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from ec9dfc0 to 03a6b6e Jan 2, 2019

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 2, 2019

Note that if we decided that the definition of touch being considered in #6477 (comment) is adequate then there is a much simpler impl for this PR. The reason I am suspicious of that straightforward impl, is @matklad's comment at #2426 (comment). Am I just being paranoid, and it safe to mtime = SystemTime::now()?

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from b07ea30 to bde1418 Jan 2, 2019

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 2, 2019

Inspired by my last comment, I just made a commit that modifies the mtime of the output files to match the timestamp file. This means that the "invoked.timestamp" file is now a temp file needed for only a few cycles, to determine the mtime, and questions if it will be overwritten by other invocation no longer matter. It can be removed entirely if we decided SystemTime::now() is good enough.

Back touch the output instead of having a new important file.
This still round trips through the file system just in case it is on a different clock.

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from bde1418 to eba4323 Jan 2, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2019

@ehuss if you change this block to use > instead of >=, does it move 100% of tries on your script to just one invocation? (that is what I suspect is the culprit here)

@Eh2406 the current PR looks pretty good to me, I think I like the idea (if perf is acceptable which given the last time I saw your laptop I assume it is because you're building this) of using the filesystem clock as the source of truth. Perhaps a utility function could be added like get_current_filesystem_time(dir: &Path) -> io::Result<SystemTime>?

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 3, 2019

get_current_filesystem_time added. We can't switch to >, as the file may have been modified after we started the build, but in the same time slot (my 1.3 above).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 4, 2019

I've written up a comment about how I see a way forward solving both this and the original issue with >=

bors added a commit that referenced this pull request Jan 4, 2019

Auto merge of #6484 - Eh2406:modified-during-build, r=alexcrichton
Rebuild on mid build file modification

This is an attempt to Fixes #2426, it is based on the test @dwijnand made for #5417.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 4, 2019

⌛️ Testing commit 746f7e4 with merge 23de59d...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 4, 2019

💔 Test failed - status-travis

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 4, 2019

Some of these tests only reliably work with >=. I'm not so confident it's a good idea to change that without thorough testing on HFS, and adjusting all those tests (and it arguably demonstrates where > fails).

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 4, 2019

I don't have access to an HFS system. I can iteratively fix theas using CI, but it will take a while.
A thought I have been having, could the filetime crate have a environment variable then wen set and in debug, always rounds times to the second, so we can test this on non limited systems?

Meanwhile, how should we proceed:

a. we continue with > ether by me fixing on CI or someone else offering to fix.
b. we reject this until an implementation of a hashing solution.
c. we merge this with >= and priorities a hashing solution.

Thoughts?

@Eh2406 Eh2406 force-pushed the Eh2406:modified-during-build branch from 746f7e4 to 40a0779 Jan 5, 2019

@Eh2406 Eh2406 referenced this pull request Jan 5, 2019

Merged

debug HFS on other systems #37

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 6, 2019

I used alexcrichton/filetime#37 to fix the two that failed this time on CI. CI is green (twice).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 7, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

📌 Commit 40a0779 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

⌛️ Testing commit 40a0779 with merge b84e625...

bors added a commit that referenced this pull request Jan 7, 2019

Auto merge of #6484 - Eh2406:modified-during-build, r=alexcrichton
Rebuild on mid build file modification

This is an attempt to Fixes #2426, it is based on the test @dwijnand made for #5417.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b84e625 to master...

@bors bors merged commit 40a0779 into rust-lang:master Jan 7, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Eh2406 Eh2406 added the relnotes label Jan 7, 2019

@Eh2406 Eh2406 deleted the Eh2406:modified-during-build branch Jan 7, 2019

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 7, 2019

Someone who knows this stuff better than I, did this also fix the corresponding case with cargo check or does it use a different code path?

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 8, 2019

or does it use a different code path?

It should all be the same.

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 8, 2019

I tryed changing 'build' to 'check' in the test, and it failed. So I think something must be different.

@Eh2406

This comment has been minimized.

Copy link
Contributor Author

Eh2406 commented Jan 8, 2019

turned out to be a problem with the test, the test was looking for the word compiling not the word checking.

@ehuss ehuss referenced this pull request Jan 12, 2019

Merged

Update cargo #57559

bors added a commit to rust-lang/rust that referenced this pull request Jan 14, 2019

Auto merge of #57559 - ehuss:update-cargo, r=alexcrichton
Update cargo

13 commits in 34320d212dca8cd27d06ce93c16c6151f46fcf2e..2b4a5f1f0bb6e13759e88ea9512527b0beba154f
2019-01-03 19:12:38 +0000 to 2019-01-12 04:13:12 +0000
- Add test for publish with [patch] + cleanup. (rust-lang/cargo#6544)
- Fix clippy warning (rust-lang/cargo#6546)
- Revert "Workaround by using yesterday's nightly" (rust-lang/cargo#6540)
- Adding feature-flags to `cargo publish` and `cargo package` (rust-lang/cargo#6453)
- Fix the Travis CI badge (rust-lang/cargo#6530)
- Add helpful text for Windows exceptions like Unix (rust-lang/cargo#6532)
- Report fix bugs to Rust instead of Cargo (rust-lang/cargo#6531)
- --{example,bin,bench,test} with no argument now lists all available targets (rust-lang/cargo#6505)
- Rebuild on mid build file modification (rust-lang/cargo#6484)
- Derive Clone for TomlDependency (rust-lang/cargo#6527)
- publish: rework the crates.io detection logic. (rust-lang/cargo#6525)
- avoid duplicates in ignore files (rust-lang/cargo#6521)
- Rustflags in metadata (rust-lang/cargo#6503)

r? @alexcrichton

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 3, 2019

he
Update rust to version 1.33.0.
Pkgsrc changes:
 * Bump required rust version to build to 1.32.0.
 * Adapt patches to changed file locations.
 * Since we now patch some more vendor/ modules, doctor the corresponding
   .cargo-checksum.json files accordingly

Upstream changes:

Version 1.33.0 (2019-02-28)
==========================

Language
--------
- [You can now use the `cfg(target_vendor)` attribute.][57465] E.g.
  `#[cfg(target_vendor="apple")] fn main() { println!("Hello Apple!"); }`
- [Integer patterns such as in a match expression can now be exhaustive.][56362]
  E.g. You can have match statement on a `u8` that covers `0..=255` and
  you would no longer be required to have a `_ => unreachable!()` case.
- [You can now have multiple patterns in `if let` and `while let`
  expressions.][57532] You can do this with the same syntax as a `match`
  expression. E.g.
  ```rust
  enum Creature {
      Crab(String),
      Lobster(String),
      Person(String),
  }

  fn main() {
      let state = Creature::Crab("Ferris");

      if let Creature::Crab(name) | Creature::Person(name) = state {
          println!("This creature's name is: {}", name);
      }
  }
  ```
- [You can now have irrefutable `if let` and `while let` patterns.][57535]
  Using this feature will by default produce a warning as this behaviour
  can be unintuitive. E.g. `if let _ = 5 {}`
- [You can now use `let` bindings, assignments, expression statements,
  and irrefutable pattern destructuring in const functions.][57175]
- [You can now call unsafe const functions.][57067] E.g.
  ```rust
  const unsafe fn foo() -> i32 { 5 }
  const fn bar() -> i32 {
      unsafe { foo() }
  }
  ```
- [You can now specify multiple attributes in a `cfg_attr` attribute.][57332]
  E.g. `#[cfg_attr(all(), must_use, optimize)]`
- [You can now specify a specific alignment with the `#[repr(packed)]`
  attribute.][57049] E.g. `#[repr(packed(2))] struct Foo(i16, i32);` is a
  struct with an alignment of 2 bytes and a size of 6 bytes.
- [You can now import an item from a module as an `_`.][56303] This allows you
  to import a trait's impls, and not have the name in the namespace. E.g.
  ```rust
  use std::io::Read as _;

  // Allowed as there is only one `Read` in the module.
  pub trait Read {}
  ```
- [You may now use `Rc`, `Arc`, and `Pin` as method receivers][56805].

Compiler
--------
- [You can now set a linker flavor for `rustc` with the `-Clinker-flavor`
  command line argument.][56351]
- [The mininum required LLVM version has been bumped to 6.0.][56642]
- [Added support for the PowerPC64 architecture on FreeBSD.][57615]
- [The `x86_64-fortanix-unknown-sgx` target support has been upgraded to
  tier 2 support.][57130] Visit the [platform support][platform-support]
  page for information on Rust's platform support.
- [Added support for the `thumbv7neon-linux-androideabi` and
  `thumbv7neon-unknown-linux-gnueabihf` targets.][56947]
- [Added support for the `x86_64-unknown-uefi` target.][56769]

Libraries
---------
- [The methods `overflowing_{add, sub, mul, shl, shr}` are now `const`
  functions for all numeric types.][57566]
- [The methods `rotate_left`, `rotate_right`, and `wrapping_{add, sub, mul,
  shl, shr}`
  are now `const` functions for all numeric types.][57105]
- [The methods `is_positive` and `is_negative` are now `const` functions for
  all signed numeric types.][57105]
- [The `get` method for all `NonZero` types is now `const`.][57167]
- [The methods `count_ones`, `count_zeros`, `leading_zeros`, `trailing_zeros`,
  `swap_bytes`, `from_be`, `from_le`, `to_be`, `to_le` are now `const` for all
  numeric types.][57234]
- [`Ipv4Addr::new` is now a `const` function][57234]

Stabilized APIs
---------------
- [`unix::FileExt::read_exact_at`]
- [`unix::FileExt::write_all_at`]
- [`Option::transpose`]
- [`Result::transpose`]
- [`convert::identity`]
- [`pin::Pin`]
- [`marker::Unpin`]
- [`marker::PhantomPinned`]
- [`Vec::resize_with`]
- [`VecDeque::resize_with`]
- [`Duration::as_millis`]
- [`Duration::as_micros`]
- [`Duration::as_nanos`]


Cargo
-----
- [Cargo should now rebuild a crate if a file was modified during the initial
  build.][cargo/6484]

Compatibility Notes
-------------------
- The methods `str::{trim_left, trim_right, trim_left_matches,
  trim_right_matches}` are now deprecated in the standard library, and their
  usage will now produce a warning.  Please use the `str::{trim_start,
  trim_end, trim_start_matches, trim_end_matches}` methods instead.
- The `Error::cause` method has been deprecated in favor of `Error::source`
  which supports downcasting.

[55982]: rust-lang/rust#55982
[56303]: rust-lang/rust#56303
[56351]: rust-lang/rust#56351
[56362]: rust-lang/rust#56362
[56642]: rust-lang/rust#56642
[56769]: rust-lang/rust#56769
[56805]: rust-lang/rust#56805
[56947]: rust-lang/rust#56947
[57049]: rust-lang/rust#57049
[57067]: rust-lang/rust#57067
[57105]: rust-lang/rust#57105
[57130]: rust-lang/rust#57130
[57167]: rust-lang/rust#57167
[57175]: rust-lang/rust#57175
[57234]: rust-lang/rust#57234
[57332]: rust-lang/rust#57332
[57465]: rust-lang/rust#57465
[57532]: rust-lang/rust#57532
[57535]: rust-lang/rust#57535
[57566]: rust-lang/rust#57566
[57615]: rust-lang/rust#57615
[cargo/6484]: rust-lang/cargo#6484
[`unix::FileExt::read_exact_at`]: https://doc.rust-lang.org/std/os/unix/fs/trait.FileExt.html#method.read_exact_at
[`unix::FileExt::write_all_at`]: https://doc.rust-lang.org/std/os/unix/fs/trait.FileExt.html#method.write_all_at
[`Option::transpose`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.transpose
[`Result::transpose`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.transpose
[`convert::identity`]: https://doc.rust-lang.org/std/convert/fn.identity.html
[`pin::Pin`]: https://doc.rust-lang.org/std/pin/struct.Pin.html
[`marker::Unpin`]: https://doc.rust-lang.org/stable/std/marker/trait.Unpin.html
[`marker::PhantomPinned`]: https://doc.rust-lang.org/nightly/std/marker/struct.PhantomPinned.html
[`Vec::resize_with`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.resize_with
[`VecDeque::resize_with`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.resize_with
[`Duration::as_millis`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_millis
[`Duration::as_micros`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_micros
[`Duration::as_nanos`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_nanos
[platform-support]: https://forge.rust-lang.org/platform-support.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.