esp-rs / embuild

Build support for embedded Rust: Cargo integration with other embedded build ecosystems & tools, like PlatformIO, CMake and kconfig.
Apache License 2.0
139 stars 40 forks source link

Use custom copy function to also copy metadata #51

Closed svenstaro closed 2 years ago

svenstaro commented 2 years ago

This function is required because Cargo's fingerprinting uses mtime which is not perpetuated by fs::copy. If Cargo detects a fingerprint mismatch, it will rebuild the crate with the mismatch. This could lead Cargo to keep rebuilding the same crate over and over because the comparison will always yield a new fingerprint since fs::copy doesn't take mtime into account.

A practical problem I ran into when doing this is copying partitions.csv for esp-idf-sys like so:

ESP_IDF_GLOB_PARTITION_BASE = { value = ".", relative = true }
ESP_IDF_GLOB_PARTITION_CSV = "partitions.csv"

This resulted in esp-idf-sys always getting rebuilt in my chain of dependencies and I couldn't figure out why. Cargo's log was a little help though since it told me of fingerprinting problems:

CARGO_LOG=cargo::core::compiler::fingerprint=trace cargo build

I eventually discovered this crate's copy function and saw that it didn't copy files into the target dir in such a way that it would preserve the original timestamps. I went ahead fixed it in the way that I think is doing it properly. Sadly we need a new dependency to ensure it works cross-platform but I think it's worth it.

N3xed commented 2 years ago

Another fix would have been to use embuild::fs::copy_file_if_different, this changes write to a potential binary compare (also wouldn't need an additional dependency).

N3xed commented 2 years ago

Nvm, what I said. It seems like I didn't consider that fs::copy wouldn't copy the exact file type (i.e. so that Metadata::file_type() is the same).

I eventually discovered this crate's copy function and saw that it didn't copy files into the target dir in such a way that it would preserve the original timestamps.

I don't think the timestamp is the issue here, though.

Anyway, it seems like the new dependency is needed for setting the metadata, and comparing the modified times should also reduce binary comparisons. So thanks for the fix!