fix: make build.rs idempotent
#278
Conversation
612d497 to
8beade0
Compare
build.rs
Outdated
| archive.into_inner().unwrap().finish().unwrap(); | ||
|
|
||
| // Clean up copied source trees to save space in target directory | ||
| fs::remove_dir_all(&*STOCKFISH_OUT_PATH).unwrap(); |
There was a problem hiding this comment.
Will this force the .nnue files to be redownloaded for each build?
There was a problem hiding this comment.
Good question. Those lines only remove the copied folders in the output, and build outputs are not shared across builds.
If you mean between Stockfish targets, these are only cleaned at the end of build.rs, so only the first target will download the .nnues.
But if you mean between runs of build.rs, since this PR forces the submodules not to change on build (by Cargo's build scripts contract) .nnues will always be downloaded per build.rs run unless they're somehow already there in the submodule folder (e.g if you ran make net in the submodule manually).
Note
They will only be redownloaded when build.rs itself is run (i.e. only if any of the cargo:rerun-if-* triggers), not for each build of fishnet itself!
Unfortunately we cannot prime the submodules with make net in build.rs due to the build.rs contact, since side-effects outside of OUT_DIR can break builds in subtle ways even if they're idempotent. As a real-world scenario: I'm planning to split fishnet into binary and library which would put fishnet code in Cargo's cache folder (when used as a library) which is supposed to be immutable.
🤔 I'm not sure if there's an easy way to fix this without manually managing a cache for the .nnues that's persisted across builds. Is this a deal breaker?
There was a problem hiding this comment.
@niklasf after investigating this a bit further:
Turns out I was wrong and OUT_DIR's hash changes less often than I thought, so deleting the NNUEs was indeed causing a lot of unnecessary downloads.
Added a700a90 which basically backups the eval files before the submodules are overwritten in OUT_DIR.
Note that the backup is still scoped to OUT_DIR, which means it will download the eval files on changes to build profile, settings, compiler version, etc... Seems like the best we can do without a dedicated cache outside of OUT_DIR.
What do you think?
(Also: rebased on master.)
There was a problem hiding this comment.
Sounds good. I'll check it out later this week.
|
(Now skipping the signpath test job that cannot succeed on pull requests: b7e0c1f) |
Cargo build scipts [should not modify any files outside of `OUT_DIR`](https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script). Both Stockfish and Fairy-Stockfish `Makefile`s generate output files in their `src` folders, which was making the build non-idempotent. To fix this, a copy of the repository is created in `OUT_DIR` and the compilation is performed there. This is not only a theoretical concern: this was actually failing when using rust-analyzer with a separate `targetDir`, since the normal and the analyzer builds can run in parallel, modifying files in the source folder concurrently and making each other fail.
Co-authored-by: Niklas Fiekas <niklas.fiekas@backscattering.de>
886469d to
e59b47b
Compare
This avoids hitting the network on subsequent `build.rs` runs. Note that the backup is still scoped to `OUT_DIR`, which means it will download the eval files on changes to build profile, settings, compiler version, etc... Seems like the best we can do without a dedicated cache directory.
e59b47b to
a700a90
Compare
Submodules need to be initialized before building on a fresh repo. There is an error message indicating this, but lichess-org#278 introduced a bug where the warning is not shown anymore. The root cause is that submodules are (empty) directories even if not yet initialized so checking if "submodule" is a directory always passes. Instead it should check "submodule/src", which should only exist if the submodule has been initialized.
Submodules need to be initialized before building on a fresh repo. There is an error message indicating this, but lichess-org#278 introduced a bug where the warning is not shown anymore. The root cause is that submodules are (empty) directories even if not yet initialized so checking if "submodule" is a directory always passes. Instead it should check "submodule/src", which should only exist if the submodule has been initialized.
Cargo build scipts should not modify any files outside of
OUT_DIR. Both Stockfish and Fairy-StockfishMakefiles generate output files in theirsrcfolders, which was making the build non-idempotent. To fix this, a copy of the repository is created inOUT_DIRand the compilation is performed there.This is not only a theoretical concern: this was actually failing when using rust-analyzer with a separate
targetDir, since the normal and the analyzer builds can run in parallel, modifying files in the source folder concurrently and making each other fail.