-
Notifications
You must be signed in to change notification settings - Fork 200
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
Preserve timestamps for installed headers to improve incremental builds #530
base: main
Are you sure you want to change the base?
Preserve timestamps for installed headers to improve incremental builds #530
Conversation
8899620
to
5fe4094
Compare
This change makes it so that the timestamps of installed header files are only updated when the contents of the header files change. This change is beneficial for incremental builds of projects depending on wasi-libc. For example, the Swift project builds wasi-libc as part of its build process, and this change reduces the number of build products in the Swift build that are unnecessarily rebuilt. The following commands can be used to verify the changes: ```bash make clean && make -j16 && ( cd sysroot && find . -name "*.h" -type f -exec stat -c '%n %y' {} \; ) > sysroot.v1.timestamp && \ make -j16 && ( cd sysroot && find . -name "*.h" -type f -exec stat -c '%n %y' {} \; ) > sysroot.v2.timestamp diff sysroot.v1.timestamp sysroot.v2.timestamp ``` The diff should show no changes in timestamps.
5fe4094
to
e5e2e9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was conditionally overwritten by Makefile, but it makes it difficult to generate the file by a regular make rule, so I changed to generate it in Makefile consistently.
@@ -981,7 +988,7 @@ check-symbols: startup_files libc | |||
|
|||
install: finish | |||
mkdir -p "$(INSTALL_DIR)" | |||
cp -r "$(SYSROOT)/lib" "$(SYSROOT)/share" "$(SYSROOT)/include" "$(INSTALL_DIR)" | |||
cp -p -r "$(SYSROOT)/lib" "$(SYSROOT)/share" "$(SYSROOT)/include" "$(INSTALL_DIR)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using cp -ar
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-a
has subtle behavior differences across implementations (e.g. darwin cp does not preserve hard link but GNU coreutils does.) and it's not a part of POSIX spec unlike -p
https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/cp.html
So I prefer -p
unless we have clear needs for -a
$(SYSROOT_INC)/__wasi_snapshot.h: | ||
mkdir -p "$(SYSROOT_INC)" | ||
ifeq ($(WASI_SNAPSHOT), p2) | ||
printf '#ifndef __wasilibc_use_wasip2\n#define __wasilibc_use_wasip2\n#endif\n' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use echo
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just kept the existing way but I don't find any reason not to use echo. Let me do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we need to use printf for Windows... https://github.com/WebAssembly/wasi-libc/actions/runs/10618495813/job/29433860327?pr=530#step:9:99
86b0fdb
to
e5e2e9d
Compare
This change makes it so that the timestamps of installed header files are only updated when the contents of the header files change. This change is beneficial for incremental builds of projects depending on wasi-libc. For example, the Swift project builds wasi-libc as part of its build process, and this change reduces the number of build products in the Swift build that are unnecessarily rebuilt.
The following commands can be used to verify the changes:
The diff should show no changes in timestamps.
Eventually, unnecessary copyings themselves should be skipped but it requires making the wasi-libc build system to support complete incremental build.