Skip to content
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

Tweak symlinks in create_input #125

Merged
merged 4 commits into from
Sep 15, 2024
Merged

Tweak symlinks in create_input #125

merged 4 commits into from
Sep 15, 2024

Conversation

kylewlacy
Copy link
Member

This PR fixes some issues around symlink handling in the create_input function. I found this after finding a bug introduced by #115, but also found some other issues too.

First, when calling create_input on a directory, if two files refer to the same resource path and that path is a symlink, then that resource would only get added to one of the files. This was due to some unsound memoization, which has now been removed.

Second, the way that symlink resources were canonicalized meant that, in some situations, create_input could end up creating a directory structure that didn't match the input. I worked out two such cases:

  1. When a symlink's target points to another symlink (link1 -> link2, link2 -> target)
  2. When a symlink's target has another symlink as a path component (link1 -> link2/file, link2 -> dir)

For now, I made the call to just return an error if either of these two situations occur. I don't believe either case was used in practice (although, if it was, hopefully brioche-packages will make that pretty clear). I believe a proper implementation to support both cases would be very complicated and error-prone, so just throwing an error now will make it possible to revisit in the future

@kylewlacy kylewlacy merged commit e0b05cc into main Sep 15, 2024
5 checks passed
@kylewlacy kylewlacy deleted the symlink-fixes branch September 15, 2024 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant