-
Notifications
You must be signed in to change notification settings - Fork 230
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
fix: symlinks handling #3298
fix: symlinks handling #3298
Conversation
I'm a bit unsure if we want to support this, how does it handle cycles? Might it not be better to just ask advanced users to copy files if they need to include them in multiple locations. They can wrap their |
It’s common use case for macOS/iOS plug-ins, third party libraries in ffi plugins etc In our case it forces us to remove directory with symlink to common scripts before publish pubignore also does not solve it since this check is done prior to application of ignore rules |
The only failing test should be one which ensures Previous behavior (pub bundled in dart 2.12 and behavior in current PR):
Current behavior:
If you insist this is expected behavior: I could try to move the check after resolving of ignored files and allow only ignored directory symlinks |
Hmm, it seems that in windows it fails to list dirrectory with loops in symlinks It differs from what is done on posix-like systems
|
Besides the cycle-handling this is looking quite good! |
jfyi: I had no time to reslove windows-related bugs lately, will continue in a few days |
Hmm - seems the CI doesn't agree quite with my research. On mac it seems contents of sufficiently nested directories is not listed. And on windows sufficiently nested links exists as Link but not as a Directory... |
Does that matter if it is documented? It would still work for all use cases where everything is set up correctly right? |
Yeah - perhaps we don't need to care that much about symlink cycles. If you have them, you kind of set yourself up for trouble... |
@jonasfj I reinstated some symlink detection. I think™ this works.
Could you do a sanity check? |
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 think this will work.
fixes: #3143