Cache: introduce prefixes to manifests #13596
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewrk
force-pushed
the
cache-path-prefixes
branch
from
532b0c8 to
3da0ab5
Compare
andrewrk
force-pushed
the
cache-path-prefixes
branch
2 times, most recently
from
cabd955 to
fb511e3
Compare
Member
Author
|
The code looks good to me and it's passing all the tests, however, I was not able to confirm that this solves the use case from #13050 yet. I want to investigate that before merging this. Update: I've identified the issue: Problem is when the prefix is relative but the added file path is absolute, the prefix detection goes wrong. I'm working on a fix. |
Member
Author
|
After these two commits, we're looking good: This is ready to merge once it is passing the CI tests. I did implement a breaking change so it may take a few rounds until it is green. |
andrewrk
force-pushed
the
cache-path-prefixes
branch
2 times, most recently
from
23cc3e2 to
a8c06b2
Compare
Contributor
|
First pass, just tying it to Note the missing first letter of each file/dir after the I will dig in and try to reproduce this outside of Bazel. |
Contributor
Looks like bazel-zig-cc was adding a trailing slash to |
Contributor
|
I have tested this change with https://git.sr.ht/~motiejus/test-zigcc. Results:
|
andrewrk
force-pushed
the
cache-path-prefixes
branch
2 times, most recently
from
2e51020 to
a2e9dd2
Compare
Before, cache manifest files would have absolute file paths. This is problematic for two reasons: * Absolute file paths are not portable. Some operating systems such as WASI have trouble with them. The files themselves are less portable; they cannot be migrated from one user's home directory to another's. And finally they can break due to file paths exceeding maximum path component size. * They would prevent some advanced use cases of Zig, where the lib dir has a different path in a different invocation but is ultimately the same Zig version and lib directory as before. This commit adds a new column that specifies the prefix directory for each file. 0 is an escape hatch and has the previous behavior. The other two prefixes introduced are zig lib directory, and the cache directory. This means files in zig-cache manifests can reference files local to these directories. In practice, this means it is possible to use a different file path for the zig lib directory in a subsequent run of zig and have it still take advantage of the global cache, provided that the files inside remain unchanged. closes #13050
This is a breaking change to the API. Instead of the first path
implicitly being the current working directory, it now asserts that the
number of paths passed is greater than zero.
Importantly, it never calls getcwd(); instead, it can possibly return
".", or a series of "../". This changes the error set to only be
`error{OutOfMemory}`.
closes #13613
* Update for the breaking changes to std.fs.path.resolve. This had a happy side effect of deleting some error handling code which is no longer needed. * Introduce cache_exempt_flags field to CSourceFile. This is used only for include directories when building libc++ and libc++abi which depend only on the zig lib path. * libc_include_dir_list is only added to the cache hash when it contains directories which have been obtained from system probing. It is exempt when the directories depend only on the zig lib path.
Fixes issues with trailing slashes and things like this.
Standard library tests require the root source file to be the corresponding file inside the Zig lib directory. In other words, there may not be two copies of the standard library. After the changes in this branch, Zig no longer notices that `../lib/std.zig` and `$(pwd)/../lib/std.zig` are the same file because one is relative and one is absolute.
andrewrk
force-pushed
the
cache-path-prefixes
branch
from
62ddd6c to
984acae
Compare
motiejus
reviewed
Nov 23, 2022
Comment on lines
-3249
to
-3255
| error.CurrentWorkingDirectoryUnlinked, | ||
| error.Unexpected, | ||
| => comp.lockAndSetMiscFailure( | ||
| .analyze_pkg, | ||
| "unexpected problem analyzing package '{s}'", | ||
| .{pkg.root_src_path}, | ||
| ), |
Contributor
There was a problem hiding this comment.
I am getting this error when compiling the PR:
module.semaPkg(pkg) catch |err| switch (err) {
^~~~~~
/home/user/zig/src/Compilation.zig:3255:45: note: unhandled error value: 'error.CurrentWorkingDirectoryUnlinked'
/home/user/zig/src/Compilation.zig:3255:45: note: unhandled error value: 'error.Unexpected'
referenced by:
performAllTheWork: /home/user/zig/src/Compilation.zig:3087:17
update: /home/user/zig/src/Compilation.zig:2396:13
remaining reference traces hidden; use '-freference-trace' to see all reference traces
I am quite puzzled how the CI passes this.
Member
Author
There was a problem hiding this comment.
I suspect you have mixed the new binary with the old standard library. It would explain this error as well as the regression noted below regarding absolute ZIG_LIB_DIR.
Contributor
There was a problem hiding this comment.
You are right, not sure how this happened. I built zig like I always do. Hmm. Indeed this was a false alarm (and the reported issue below), re-checked with master.
motiejus
suggested changes
Nov 23, 2022
Contributor
There was a problem hiding this comment.
This PR regresses relative paths returned by -M.
If ZIG_LIB_DIR is relative, it is desired to return relative paths in -M flags.
setup
lrwxrwxrwx 1 motiejus engineering 33 Nov 23 12:42 zig_lib_dir_0.10 -> /code/zig-linux-x86_64-0.10.0/lib
lrwxrwxrwx 1 motiejus engineering 24 Nov 23 12:42 zig_lib_dir_13596 -> /code/zig/stage3/lib/zig
/code/zig/stage3/ is zig with this PR with the Compilation.zig change reverted for zig to compile.
zig 0.10 (also current master)
$ ZIG_LIB_DIR=zig_lib_dir_0.10 /code/zig-linux-x86_64-0.10.0/zig cc -target x86_64-linux-gnu.2.28 -M main.c
main.o: main.c zig_lib_dir_0.10/libc/include/generic-glibc/stdio.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/libc-header-start.h \
zig_lib_dir_0.10/libc/include/generic-glibc/features.h \
zig_lib_dir_0.10/libc/include/generic-glibc/features-time64.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/bits/wordsize.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/bits/timesize.h \
zig_lib_dir_0.10/libc/include/generic-glibc/stdc-predef.h \
zig_lib_dir_0.10/libc/include/generic-glibc/sys/cdefs.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/bits/long-double.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/gnu/stubs.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/gnu/stubs-64.h \
zig_lib_dir_0.10/include/stddef.h zig_lib_dir_0.10/include/stdarg.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/bits/typesizes.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/time64.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types/__fpos_t.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types/__mbstate_t.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types/__fpos64_t.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types/__FILE.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types/FILE.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/types/struct_FILE.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/stdio_lim.h \
zig_lib_dir_0.10/libc/include/x86_64-linux-gnu/bits/floatn.h \
zig_lib_dir_0.10/libc/include/generic-glibc/bits/floatn-common.h
This PR
$ ZIG_LIB_DIR=zig_lib_dir_13596 /code/zig/stage3/bin/zig cc -target x86_64-linux-gnu.2.28 -M main.c
main.o: main.c \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/stdio.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/libc-header-start.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/features.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/features-time64.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/bits/wordsize.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/bits/timesize.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/stdc-predef.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/sys/cdefs.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/bits/long-double.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/gnu/stubs.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/gnu/stubs-64.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/include/stddef.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/include/stdarg.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/bits/typesizes.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/time64.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types/__fpos_t.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types/__mbstate_t.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types/__fpos64_t.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types/__FILE.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types/FILE.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/types/struct_FILE.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/stdio_lim.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/x86_64-linux-gnu/bits/floatn.h \
/code/bazel-zig-cc/test/c/zig_lib_dir_13596/libc/include/generic-glibc/bits/floatn-common.h
I'd expect the paths to ZIG_LIB_DIR as returned by -M to be relative.
FabianHahn
pushed a commit
to FabianHahn/bazel-zig-cc
that referenced
this pull request
Dec 11, 2022
- do not pass where it's unnecessary - remove trailing slash. That conflicts with ziglang/zig/pull/13596
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Jan 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before, cache manifest files would have absolute file paths. This is problematic for two reasons:
This commit adds a new column that specifies the prefix directory for each file. 0 is an escape hatch and has the previous behavior. The other two prefixes introduced are zig lib directory, and the cache directory. This means files in zig-cache manifests can reference files local to these directories.
In practice, this means it is possible to use a different file path for the zig lib directory in a subsequent run of zig and have it still take advantage of the global cache, provided that the files inside remain unchanged.
closes #13050