Conversation
nyh
left a comment
There was a problem hiding this comment.
Looks good, but I had a beef with the comments so maybe you can fix them before I merge? Thanks.
| # | ||
| # Do NOT narrow this to a filesystem or to a conf_zfs value. Both forms leave | ||
| # the default configuration broken: fs=zfs misses every other root filesystem, | ||
| # and conf_zfs=openzfs misses conf_zfs=bsd, which also builds libsolaris.so. |
There was a problem hiding this comment.
Nitpick: The first three lines of this comment are good - the rest are just extra fluff that shows a lot of irrelevant details. I don't know why AI insists on writing such comments. The problem is not just that they are two long - it's that they list super-specific details which 1. Are not relevant for understanding and, 2. May not be accurate tomorrow.
| # For fs=ramfs, scripts/build passes the generated $(out)/usr.manifest as the | ||
| # bootfs manifest. That manifest comes from usr_ramfs.manifest.skel, which | ||
| # lists tools/mount/mount-fs.so and tools/mount/umount.so, so a fresh tree | ||
| # races on those two the same way. |
There was a problem hiding this comment.
Again, the last comment line is superfluous... The dependency is needed because it is needed. We don't need a comment for each individual dependency in the makefile explaining in detail that "if we forget this dependency, the result will be a failure or race on a fresh build"!
bootfs.manifest.skel names /usr/lib/fs/libsolaris.so unconditionally, so
libsolaris.so is part of bootfs.bin for every configuration. bootfs_dep
listed it only under fs=zfs, so with any other value of fs, or with fs unset,
mkbootfs.py runs before the library has been built.
Serially that is a hard failure. On an unmodified tree, with no options:
$ make -j1 build/release.x64/bootfs.bin
MKBOOTFS build/release.x64/bootfs.bin
strip: 'libsolaris.so': No such file
Failed stripping libsolaris.so. Using original.
Traceback (most recent call last):
File "scripts/mkbootfs.py", line 90, in <module>
main()
File "scripts/mkbootfs.py", line 63, in main
size = os.stat(hostname).st_size
FileNotFoundError: [Errno 2] No such file or directory: 'libsolaris.so'
make: *** [Makefile:2239: build/release.x64/bootfs.bin] Error 1
Under -j it is a race, because whether it fails depends only on whether some
other rule happened to produce libsolaris.so first:
$ make -j8 build/release.x64/bootfs.bin build/release.x64/libsolaris.so
... same FileNotFoundError ...
This is worth fixing beyond the direct failure, because a missing prerequisite
is exactly the shape that produces INTERMITTENT parallel-build failures: at
high -j the two rules overlap and bootfs.bin wins the race often enough to
look flaky rather than broken. Anyone who has papered over an occasional
".so races bootfs" failure by pre-building the libraries first was working
around this line.
The reason it is not noticed in normal use is that scripts/build defaults
fs=zfs (scripts/build: fs_type=${vars[fs]-zfs}), so the one branch that
happens to set the dependency is the only branch anyone routinely exercises.
Make the prerequisite unconditional. Two narrower forms do not work, and both
leave the default configuration broken: keying on fs=zfs misses every other
root filesystem, and keying on conf_zfs=openzfs misses conf_zfs=bsd, which
also builds libsolaris.so.
Also order the two mount tools before bootfs for fs=ramfs. There
scripts/build passes the generated usr.manifest, which is produced from
usr_ramfs.manifest.skel and lists tools/mount/mount-fs.so and
tools/mount/umount.so, so a fresh tree races on those two in the same way.
Verified: bootfs.bin now builds with fs unset, both serially and under -j8,
and the full loader.elf still links.
b5708e9 to
0e7ddbf
Compare
|
Fixed in The first one is now the three lines you kept: and the You are right about the failure mode, and it is worth naming because it is a habit I should not have: the details I cut were specific enough to go stale, and a comment that has quietly gone stale is worse than no comment, because the next reader trusts it. The "if we forget this dependency there will be a race" line was the clearest case - a Makefile prerequisite does not need a comment explaining what prerequisites are for. The reproducer and the reasoning about why narrower forms do not work are still in the commit message and the PR description, which is where someone looking for them will be. |
PR cloudius-systems#1514 was respun to shorten two Makefile comments the reviewer objected to. The change is comment-only, so this branch's build behaviour was already correct, but carrying the superseded text means the branch misrepresents the PR. Sync it so "this branch carries PR cloudius-systems#1514" is true of its content and not just its number. Signed-off-by: Greg Burd <greg@burd.me>
Problem
bootfs.manifest.skelnames/usr/lib/fs/libsolaris.sounconditionally, solibsolaris.sois part ofbootfs.binin every configuration. Butbootfs_deplisted it only underfs=zfs, so with any other value offs, or withfsunset,mkbootfs.pyruns before the library has been built.Serially that is a hard failure. On an unmodified tree at current master, with no options:
Under
-jit becomes a race, because whether it fails depends only on whether some other rule happened to producelibsolaris.sofirst.It is not noticed in normal use because
scripts/builddefaultsfs=zfs(fs_type=${vars[fs]-zfs}), so the one branch that happens to set the dependency is the only branch routinely exercised.Why it is worth fixing beyond the direct failure
A missing prerequisite is exactly the shape that produces intermittent parallel-build failures: at high
-jthe two rules overlap andbootfs.binwins the race often enough that it looks flaky rather than broken. If you have ever worked around an occasional ".so races bootfs" failure by pre-building the libraries first, this line is why. I had been carrying that workaround for weeks before looking at the actual rule.Fix
Make the prerequisite unconditional. Two narrower forms both leave the default configuration broken, so I want to be explicit about why neither is used: keying on
fs=zfsmisses every other root filesystem, and keying onconf_zfs=openzfsmissesconf_zfs=bsd, which also buildslibsolaris.so(the rule atMakefile:2566is not conditional on the ZFS provider).Also order the two mount tools before bootfs for
fs=ramfs. Therescripts/buildpasses the generatedusr.manifest, produced fromusr_ramfs.manifest.skel, which liststools/mount/mount-fs.soandtools/mount/umount.so, so a fresh tree races on those two the same way.Testing
bootfs.binbuilds withfsunset, both serially and under-j8, and the fullloader.elfstill links. Makefile-only change, no effect on thefs=zfspath that was already correct.