pkg: remove broken symlinks from package sources #11582
Conversation
gridbugs
force-pushed
the
remove-broken-symlinks
branch
from
d82d990 to
52c1383
Compare
Leonidas-from-XIV
left a comment
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
I think the approach is really good because as you write, both Etienne and me tried to fix the underlying issue in the engine and got stuck so this workaround seems like a really nice pragmatic solution to sidestep the issue without breaking anything.
That said, I have some questions on how this works, I've written them in the comments.
otherlibs/stdune/src/fpath.ml
Outdated
| let traverse = | ||
| let rec loop on_file on_dir root stack acc = | ||
| let rec loop on_file on_dir on_broken_symlink root stack acc = |
There was a problem hiding this comment.
| let traverse = | |
| let rec loop on_file on_dir root stack acc = | |
| let rec loop on_file on_dir on_broken_symlink root stack acc = | |
| let traverse ~on_broken_symlink = | |
| let rec loop on_file on_dir root stack acc = |
Given on_broken_symlink isn't really part of the loop (it is never changed and the recursive calls always reuses the same one), I'd put it outside of the recursive function arguments. That way one does not have to think whether these get mutated or not (same probably goes for on_file and on_dir) and one less argument to get wrong.
There was a problem hiding this comment.
Agreed. I was following suit for the other arguments to this function. I'll rearrange it so only the changing arguments are passed to loop (in a separate commit so it's clearly not part of handling broken symlinks).
src/dune_rules/fetch_rules.ml
Outdated
| ~on_file:(fun ~dir:_ _ () -> ()) | ||
| ~on_broken_symlink:(fun ~dir fname () -> | ||
| let path = Filename.concat target_abs (Filename.concat dir fname) in | ||
| Fpath.rm_rf path); |
There was a problem hiding this comment.
Isn't this a blocking operation? We probably need some Fiber version of traverse to avoid blocking the execution while waiting on file IO.
There was a problem hiding this comment.
It is, but I don't think there's an async rm in dune. Further down in this file we do Io.copy_file inside an async context (not that that makes it correct to do so, but at least there's a precedent). The blocking rm_rf is expected to be called very infrequently - only while extracting packages with broken symlinks (which are quite rare).
There was a problem hiding this comment.
For me the issue isn't so much the rm (but I see how the location where I placed my comment makes it confusing), my concern is rather that Fpath.traverse is blocking.
There was a problem hiding this comment.
Ah makes sense. Dune doesn't currently have an async way of traversing a directory structure but we can probably throw together an async traverse based on the Dune_engine.Fs_memo module.
There was a problem hiding this comment.
Do you want to do this in this PR or should we defer it to a future PR? The rest of the PR looks good so unless somebody raises concerns LGTM.
There was a problem hiding this comment.
I think let's defer this work. There are other instances of Fpath.traverse that we should also update, and I don't want this PR to get too drawn out.
test/blackbox-tests/test-cases/pkg/broken-symlink-in-dependency.t
Outdated
Show resolved
Hide resolved
gridbugs
force-pushed
the
remove-broken-symlinks
branch
from
52c1383 to
74d3e57
Compare
|
What about when a broken symlink is purposefully included for say testing reasons? |
test/blackbox-tests/test-cases/pkg/broken-symlink-in-dependency.t
Outdated
Show resolved
Hide resolved
test/blackbox-tests/test-cases/pkg/broken-symlink-in-dependency.t
Outdated
Show resolved
Hide resolved
Dune doesn't run tests on a project's dependencies anyway so this change won't affect that use case. |
gridbugs
force-pushed
the
remove-broken-symlinks
branch
from
74d3e57 to
0cddc97
Compare
|
Here is a related issue that I tried to tackle previously: I agree that we should probably just delete these for pkg. |
|
Yeh there have been a few attempts at fixing the issue in a general way that didn't end up getting merged. |
Dune cannot handle symlinks with missing targets. Package source archives may contain such broken symlinks. This can be due to packaging errors on the part of the package author which are benign to the correct operation of the package when installed with opam, but render packages uninstallable with dune. It's assumed that all opam packages can be installed with opam, so if a package contains a broken symlink then the symlink must not be necessary for the correct operation of the package. Thus the fix is to remove broken symlinks from extracted source archives, and to avoid copying broken symlinks from local package source directories. A less conservative approach than this might attempt to allow dune to handle broken symlinks in general, but this would be a much larger change, and has been attempted and abandoned several times before. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
gridbugs
force-pushed
the
remove-broken-symlinks
branch
from
0cddc97 to
a649216
Compare
4 participants
Dune cannot handle symlinks with missing targets. Package source archives may contain such broken symlinks. This can be due to packaging errors on the part of the package author which are benign to the correct operation of the package when installed with opam, but render packages uninstallable with dune.
It's assumed that all opam packages can be installed with opam, so if a package contains a broken symlink then the symlink must not be necessary for the correct operation of the package. Thus the fix is to remove broken symlinks from extracted source archives, and to avoid copying broken symlinks from local package source directories.
A less conservative approach than this might attempt to allow dune to handle broken symlinks in general, but this would be a much larger change, and has been attempted and abandoned several times before.