Skip to content

Follow file symlinks too #2827

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

Closed

Conversation

lukaslihotzki
Copy link

Currently, pub publish follows directory symlinks, but not file symlinks. With this PR, file symlinks are followed too. This fixes broken symlinks if they point outside the package root and should not break anything, although it could increase package size.

The dry-run shows the directory tree with directory symlinks resolved, and without indicating symlinks for file symlinks, so it is very confusing currently.

Ideally, symlinks would only be resolved if they point outside the package root. However, that would need two or three tar runs (one with, one without --dereference) and using tar --concatenate (in the second or the final, third run). It would make the code significantly more complex.

Related issues:

@google-cla google-cla bot added the cla: yes label Jan 13, 2021
@kevmoo kevmoo requested a review from jonasfj January 13, 2021 16:49
@sigurdm
Copy link
Contributor

sigurdm commented Jan 18, 2021

I think I agree this is the right behavior.

One issue here is that we don't have the same tar process for windows and linux/macos. On windows we use 7zip. On the other hand symlinks are probably much less used on windows.

Could you write a test that shows this as being fixed?

We are working on migrating to a tar implementation in dart (#2817). But it is not clear how long it will take to land. It would be great to have the test for ensuring that the behavior stays consistent when we do the switch.

@lukaslihotzki
Copy link
Author

On Windows, 7-Zip has no option to follow file symlinks. There are just these options about links:

  -snh : store hard links as links
  -snl : store symbolic links as links

Windows 10 seems to have a system tar.exe included since v1803 (bsdtar 3.3.2). This one could support -L to dereference symlinks on create. However, I would prefer to disable the test on Windows until #2817.

@jonasfj
Copy link
Member

jonasfj commented Jan 20, 2021

Currently, pub publish follows directory symlinks, but not file symlinks.

It does follow directory symlinks? doesn't it just package the actual symlink?

@lukaslihotzki
Copy link
Author

lukaslihotzki commented Jan 20, 2021

It does follow directory symlinks? doesn't it just package the actual symlink?

It does follow directory symlinks, as shown by the 'publish follows directory symlinks' test in this PR. At least for symlinks pointing to excluded or out-of-package-root targets, this is the only sensible behavior.

@sigurdm
Copy link
Contributor

sigurdm commented Feb 9, 2021

We have merged #2817 . If you want we can land the test. I believe our implementation follows links.

@sigurdm
Copy link
Contributor

sigurdm commented Mar 8, 2021

Closing for lack of activity.

Feel welcome to open a new PR with the test if you would like ensuring the desired behavior does not regress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants