fix: symlinks handling

[2ZeroSix]

fixes: #3143

• reverts to old behavior: symlinks to directories are handled as copy-paste #3143
• fix of pubignore/gitignore:
  • allow non-resolving symlinks if they are ignored
  • don't warn on ignored directory symlinks

[jonasfj]

I'm a bit unsure if we want to support this, how does it handle cycles? What is the need for symlink directories?

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 dart pub publish command with a bash script living in tool/ -- it's ugly, but how many people actually need this?

[2ZeroSix]

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

[2ZeroSix]

• added test for cycles
• fixed wrongly appended / to path consisted of single delimiter

The only failing test should be one which ensures lish is failing on package with symlink to directory. This seems to be a regression and was introduced somewhere between flutter 2.5+ and 2.8- (dart 2.12 and 2.15)

Previous behavior (pub bundled in dart 2.12 and behavior in current PR):

• pub archives all filtered files in tar.gz

• consumer receives them as separate files after unpacking of tar.gz

• so, there is no OS-dependant problems for consumer and no headache for package developer

Current behavior:

• pub throws on any (even ignored) directory symlink beneath package directory
• package developers should apply workarounds to their pipelines
• consumers doesn't see any difference

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

[2ZeroSix]

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

02:34 +32 -1: test/package_list_files_test.dart: throws on symlink cycles
02:34 +32 -2: test/package_list_files_test.dart: throws on symlink cycles [E]
  Expected: throws <Instance of 'DataException'> with `message`: contains 'Pub does not support publishing packages with non-resolving symlink:'
    Actual: <Closure: () => List<String>>
     Which: threw FileSystemException:<FileSystemException: Directory listing failed, path = 'C:\Users\runneradmin\AppData\Local\Temp\dart_test_5e4ddcd9\myapp\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\subdir\symlink\*' (OS Error: The system cannot find the path specified.
                  , errno = 3)>
            stack dart:io                                   _Directory.listSync
                  package:pub/src/package.dart 248:48       Package.listFiles.<fn>
                  package:pub/src/ignore.dart 291:28        Ignore.listFiles
                  package:pub/src/package.dart 245:19       Package.listFiles
                  test\package_list_files_test.dart 115:30  main.<fn>.<fn>
                  package:test_api                          expect
                  test\package_list_files_test.dart 114:5   main.<fn>
                  
            which is not an instance of 'DataException'
  
  package:test_api                         expect
  test\package_list_files_test.dart 114:5  main.<fn>

[sigurdm]

Besides the cycle-handling this is looking quite good!

[2ZeroSix]

jfyi: I had no time to reslove windows-related bugs lately, will continue in a few days

[2ZeroSix]

Hi, today I finally found time to fix all problems on windows

I'm going to take a look at the overall code style tomorrow, but otherwise it seems to be ready for review

UPD: nope, as I've started to add comments with reasoning of this solution I found mistake, writing an additional test and fixing the problem now

UPD2: now it should be definitely ready for review 😅

[jonasfj]

@2ZeroSix, can you refresh my memory, how does this handle symlinks?

With this symlink folders will be considered the same as if the folder had been copied. Thus, cycles won't be allowed. And symlinks will not be retained in the tarballs, instead the files will simply be duplicated. Is that correct?

---

My concerns are:

• It might be much harder / impossible to verify package contents vs git revision? (cc, @simolus3)
• Wouldn't it be better to fix the Flutter plugin templates to not require duplication?

[jonasfj]

@2ZeroSix is this mostly for ios/macos dart:ffi based plugins? Or are the other use-cases?

[2ZeroSix]

@jonasfj

There is a few use cases:

• any shared code between plugins (macos+ios, ffi, etc)
• some local tooling (like build scripts or something) among packages in multi-package repositories via symlinks
• any other project specific directory symlinks that are not meant to be published
• etc

In most use cases besides "code" directory symlinks are not meant to be published, but existing code throws an exception even for symlink that points to empty directory or ignored (works as expected only if you ignore whole parent directory of symlink)

This pull request solves:

• allows directory symlinks to be published as directory
• allows directory symlinks if they are ignored:
  • gitignore validator seen directory symlinks as directories (but for git they are files), and listed them as gitignored but checked out
• allows non-resolving symlinks if they are ignored

It might be much harder / impossible to verify package contents vs git revision? (cc, @simolus3)

• existing solution: either duplicate code or add some pre-publish scripts to modify package structure
• after this: with minor adjustments it's possible to create archive from these files by running pub archive (for example) and compare them against published archive (there is still room for custom pre-publish scripts, but the space in which it is necessary will decrease)

[2ZeroSix]

If that doesn't seem convincing enough, here is another way:

here is spinoff of this pull request which allows anything (if it is ignored), but still forbids directory symlinks to be published https://github.com/dart-lang/pub/pull/3300 (it's a bit outdated and i've closed it as there was no activity)

After addition of proper loop detection it's pretty easy to adjust solution so it would throw only if symlinked directory or it's content isn't ignored

[jonasfj]

any other project specific directory symlinks that are not meant to be published

This I'm sold we should allow.. I thought we did fix that, or did we never land the PR? But yes, .pubignore/.gitignore should be able to ignore symlinks.

[2ZeroSix]

Yeah, it was never landed.

So lets consider what is the best behavior for pub:

• allow any symlinks and throw only on cycles and non-resolving ones (if they are not ignored)
• disallow directory symlinks and only land a fix for ignored symlinks

It seems to me a bit illogical to allow file-symlinks but disallow directory-symlinks at the same time

Anyway, I'm ok with any decision as on daily basis I mostly use symlinks for internal tooling that are not meant to be published

[jonasfj]

@2ZeroSix

disallow directory symlinks and only land a fix for ignored symlinks

This seems like a no-brainer. We're just correctly ignoring symlinks.

I see how allowing non-cyclic directory symlinks could be nice, but the cost in terms of complexity (and ongoing maintenance) is non-trivial. So maybe this something we should reconsider, if there is a lot requests for it in the future. But right now it's not a highly requested feature, and in general most people are probably better off not using symlinks :D

[Gustl22]

I would simply allow it for the sake of expectation. One should be able to get work everything which is working right now through git.

Of course it's independent, but it's the most common workflow. I see that it could be misused by some devs, but honestly, so you can with code, your git history and everything else. Linking provides much more flexibility and saves space and time for maintenance. Linking in general is fundamental for so many fields in informatics: packaging, memory, compiling, networks, file system.

The work every developer has to spend to circumvent this lack of feature takes more time and maintenance for all together and certainly brings more bugs, than having a collective and supervised mechanism for directory symlinks.

But I'm also no messias, so this may brings the linking hell. But did it in git?

[spydon]

Could we maybe re-open this one first: https://github.com/dart-lang/pub/pull/3300 So that symlinks can be properly ignored at least, since that is a bug instead of a change in behavior?

[tgrushka]

This PR has been open nearly 3 years. Frustrating that @jonasfj keeps knocking this down -- why? Maybe he doesn't know of a use case for it, but others clearly have them.

Here's why it's illogical to shoot down this idea/PR: If someone has already done the work to re-enable this feature, and also done the work to check for cycles, etc., it doesn't make sense for someone else to say that it's too much work or too complicated. That discounts both the many users who want this feature back, as well as the developer(s) who put their time and energy into developing, testing, and reviewing the code.

There are two use cases for symlinks in pub I can think of:

1. Multi-platform Flutter packages that have a darwin folder that is shared, and the ios and macos folders are symlinked to it. Maybe this has been updated to no longer require it, but I see lots of Flutter repos on GitHub that use this pattern. @jonasfj says this should be changed in Flutter. Well, that's kind of waiting for another project to do something it may or may not do, to solve a problem that is not even specific to that project, but specific to pub.dev!
2. Including header files in an FFI package (not Dart files!) that have a current version. (Even if they are Dart files, having copies of the files shouldn't break Dart code any more than a symlink would).

An example package I'm trying to publish: https://github.com/dra11y/ffipeg-dart

This package aides in generating FFI bindings for FFmpeg, and includes a default set of FFmpeg headers. The latest FFmpeg version is 7.1, so I created a 7.1 folder with those headers, and symlinked a current link to the 7.1 folder. The current will always point to the current headers version.

So now I have to take yet more time and go rework my package (which @jonasfj also suggests I should write a wrapper around dart pub publish -- crazy IMHO), when I'm not earning any income from this package, and already have way too much work to just get the basics done. It creates more work for open source developers because someone at Google says it's too complicated. Too complicated ... for Google? No, it's not. This kind of response is not helpful to us.

If the decision is so bad to allow symlinks, please explain. If it's bad practice, that wide-reaching decision should be backed up by fact, championed, and supported. There are a plethora of design decisions in Dart/Flutter that are, IMHO, far more complicated than they need to be, but I concede that allows flexibility. As a user of Dart/Flutter, I've had to learn these complexities and work with them. So why should a feature that users have been asking for since #3143 way back almost 4 years ago... be taken away with no other argument than "it's too complicated" and might be hard to maintain (speculative)?

[sigurdm]

Thanks for pinging us!

First of all we are sorry for neglecting this PR. If I remember correctly we were at the time unsure about the correct approach, and kind of punted it.

Symlinks are problematic for several reasons

• their semantics on windows are not very clear (as far as I know - the situation might look better today than last I checked)
• they risk causing circularities, that tools might fail to handle.
• if we allow publication of packages with symlinks, we most likely want to just "inline" the files (as this PR does). Not having the same file layout in the repository as in the package might cause confusion.

I would not say that symlinks don't have merit - and it is probably not warranted to completely disallow them. Just that they complicate things a lot, and that incurs a cost everywhere we handle files in pub.

Please don't appeal to the age of an issue. We always try to prioritize issues we work on best we can, and cannot fix everything. Please don't call suggestions as "crazy" even if you don't find them helpful (read https://dart.dev/community/code-of-conduct) Even at google we are limited :) .

This issue (mostly) has a work-around, so that is one reason we have not been as eager to resolve it as one could wish.

That said, we hope to find time to pick up this PR again soon. It should really not have had to wait so long.

[sigurdm]

I have done a bit of research, and it seems that both on windows, mac and linux resolveSymbolicLinksSync will fail with an exception if the nesting of a symlink to resolve is too deep.

By that logic we probably don't need to do anything explicit to prevent circularities. We just handle that exception.

I'll try to make a merge, and an update without the check for circularities.

[sigurdm]

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...

[spydon]

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?

[sigurdm]

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...

[sigurdm]

@jonasfj I reinstated some symlink detection. I think™ this works.

• For every visited directory x, it resolves canonicalize(x) to x'.
• Then for every parent of canonicalize(x), p:
  • it resolves p to p'. If x' equals p' it reports a cycle.

Could you do a sanity check?