CACI-International / ergo-cpp

A C++ build system library for ergo.
MIT License
2 stars 0 forks source link

`cache-policy = configuration` is often insufficient and maybe buggy #4

Open calebzulawski opened 1 year ago

calebzulawski commented 1 year ago

I originally suspected git was resetting modification times (and std:fs:track assumes it monotonically increases), but it looks like git actually always updates the modification time of files it changes when switching branches.

Also, considering it's always link errors (e.g. missing symbols) I'm guessing the relationship with source files and object files is somehow broken when switching branches.

afranchuk commented 1 year ago

std:fs:track doesn't assume increasing mod times, it just checks for differences. But as you've found that is likely not the culprit (not to mention if it were we would have likely found the issue sooner).

calebzulawski commented 1 year ago

This line makes me think the hash is only calculated if the file is newer, but not older:

https://github.com/CACI-International/ergo/blob/7ca0725ee95147bd3000e05626b6e7c90b4e9021/ergo_std/src/fs.rs#L743

afranchuk commented 1 year ago

Ah so it does, that should probably be changed. The modification time is only used as an optimization.

calebzulawski commented 1 year ago

Opened CACI-International/ergo#6 to change that. Also, I realized this is probably due to cache-policy--I'm going to try using cache-policy = content and see how that goes.

calebzulawski commented 1 year ago

@pdurbano reports that incremental builds sometimes (often) don't pick up source files changes and produce outdated binaries. Some source files (cpp files, not headers) always pick up changes, while others don't (depending on the module). Using cache-policy = content fixes the issue.

Unless there's extreme overhead (I didn't measure anything substantial), I think the default should be changed to content.

afranchuk commented 1 year ago

The inherent overhead is disk space. There's no policy to remove old versions. Otherwise, cache-policy = content is fine (that's what it's there for)!

afranchuk commented 1 year ago

I have some unit tests for change tracking, I should expand them.