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

Pub does not support publishing packages with directory symlinks #3143

Open
AlexV525 opened this issue Sep 23, 2021 · 21 comments · May be fixed by #3298
Open

Pub does not support publishing packages with directory symlinks #3143

AlexV525 opened this issue Sep 23, 2021 · 21 comments · May be fixed by #3298
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@AlexV525
Copy link

Environment

  • pub version: 2.14.0
  • OS version: macOS Big Sur 11.6
  • Are you using the Chinese community mirror or a corporate firewall? This issue seems not related but yes, using flutter-io.cn .

Problem

Our package includes a symlink that links ios to macos, before 2.14.0 pub does support symlink, but we cannot publish a new version using the latest pub. We neither got a warning nor a breaking change announcement.

A detailed log can be found at https://github.com/CaiJingLong/flutter_photo_manager/runs/3683581202?check_suite_focus=true .

Expected behavior

Support symlink as usual.

Actual behavior

Does not support symlinks.

@sigurdm
Copy link
Contributor

sigurdm commented Oct 7, 2021

We need to find out what was the previous behavior - did we include the symlink, or copy the files?

I can see this being useful in some cases - we should probably reenable this.

@sigurdm sigurdm added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Oct 7, 2021
@AlexV525
Copy link
Author

AlexV525 commented Oct 7, 2021

Symlinks have included I think. You can check the photo_manager package before v1.3.5 for more detail.

@sigurdm
Copy link
Contributor

sigurdm commented Oct 7, 2021

As far as I can tell the archive for photo_manager 1.3.4 has no symlinks. Which directory should be a symlink?

@AlexV525
Copy link
Author

AlexV525 commented Oct 7, 2021

The macos is a link to the ios in the repo (and it's still a symlink now), maybe a lower version might have one?

@sigurdm
Copy link
Contributor

sigurdm commented Oct 7, 2021

Looks to me like the previous behavior was to copy the symlinked directory.

@AlexV525
Copy link
Author

AlexV525 commented Oct 7, 2021

Looks to me like the previous behavior was to copy the symlinked directory.

That's what we do currently, remove the symlink and make a copy when publishing.

I think the previous behavior is good enoungh, but would be better the symlink can support. In this package we combine classes files for both iOS and macOS and they work together.

@sigurdm
Copy link
Contributor

sigurdm commented Oct 7, 2021

One issue with including symlinks in packages is that they do not work very well on windows. But I think the pub-tool could copy directories (as it already does for files).

@jonasfj I vaguely remember you having some good points here.

@AlexV525
Copy link
Author

AlexV525 commented Oct 7, 2021

Yes symlinks might have some capability issues with various of tools. So copies seem enough, the problem here is the pub tool reject them currently.

@jonasfj
Copy link
Member

jonasfj commented Oct 11, 2021

That's what we do currently, remove the symlink and make a copy when publishing.

I think the previous behavior is good enoungh, but would be better the symlink can support. In this package we combine classes files for both iOS and macOS and they work together.

I read this, as you're requesting support for symlinks? Rather than filing a bug report saying that something that used to work no longer works -- @AlexV525 is that correct?


re: symlinks -> we should avoid these. We are not synchronizing file-systems here, we're publishing packages. And a package is a set of files, maybe including executable bits, but IMO even that could be disputed in the future.

That's just my opinion. Adding all sorts of features like symlinks won't make packages easier to consume.

@AlexV525
Copy link
Author

I read this, as you're requesting support for symlinks? Rather than filing a bug report saying that something that used to work no longer works -- @AlexV525 is that correct?

Not really, it would ge glad to have the original behavior back, for now it's just removed, which looks like symlinks are supported at the first time.

So I think we have three options:

  • Fully support symlinks, but it's impossible for now likely.
  • Support symlinks by making copies when publishing, like it used to be.
  • Not to support symlinks at all, like the pub tool has done currently.

@AlexV525
Copy link
Author

To be clarify, symlinks used to be copies, which makes me think that symlinks worked well with pub. So I don't think this is a feature request, but a bug report for a feature that used to work.

@sigurdm
Copy link
Contributor

sigurdm commented Oct 11, 2021

Support symlinks by making copies when publishing, like it used to be.

I think this is a reasonable behavior, and that's what we do with file-symlinks currently and (I believe) used to do with directories.

@jonasfj
Copy link
Member

jonasfj commented Oct 11, 2021

I think this is a reasonable behavior, and that's what we do with file-symlinks currently and (I believe) used to do with directories.

Yes, we do copies for symlinks when publishing files. I'm not sure we ever did this for directory symlinks.

In either case, I think it's better to make a tool/publish.sh script that wraps your publishing process with whatever tricks you want to do.

There are other problems with symlinks, people might be confused when importing a symlinked file causes code duplication in Dart... etc.

In practice there are ways around symlinks in most cases... either using scripts or by tweaking the build system.


To avoid source duplication between ios/ and macos/ folders, you can technically put all the source in src/ or some other folder and use #include " statements in C, see:
https://github.com/google/webcrypto.dart/blob/9a0af9d3596fd50b70087375827c2242d6f96db2/ios/third_party/boringssl/err_data.c

This is not pretty -- and who knows how it'll break in the future 🙈 but it works for now 🤣

@AlexV525
Copy link
Author

Did we missed something? Cause pub does not support symlinks from 2.14. How do you guys agree woth those three options? If you think blocking symlinks works fine, then this issue can be closed, otherwise we should go for copies at least.

@jonasfj
Copy link
Member

jonasfj commented Oct 11, 2021

Cause pub does not support symlinks from 2.14

I think symlinks pointing to files are supported. Is this incorrect?

I generally dislike adding features like this to dart pub, I think we should keep packages as simple as possible.

@AlexV525
Copy link
Author

AlexV525 commented Oct 11, 2021

I think symlinks pointing to files are supported. Is this incorrect?

Well that's the point, see logs here, as I pointed out in the top comment.

A detailed log can be found at https://github.com/CaiJingLong/flutter_photo_manager/runs/3683581202?check_suite_focus=true .

After we switched to a lower version of pub, it works well with symlinks.

@jonasfj
Copy link
Member

jonasfj commented Oct 12, 2021

After we switched to a lower version of pub, it works well with symlinks.

Symlinks to directories or files?

@AlexV525
Copy link
Author

Symlinks to directories or files?

We use it for directories only.

@2ZeroSix
Copy link
Contributor

2ZeroSix commented Feb 1, 2022

summary of previous discussion:
This seems to be a regression and was introduced somewhere between flutter 2.5+ and 2.8- (dart 2.12 and 2.15), probably when .pubignore was introduced

Previous behavior (pub bundled in dart 2.12/flutter 2.5+ and behavior in current PR):

  • pub archives all filtered files in tar.gz
    • resulting tar.gz would contain file instead of symlink (or content of symlinked directory for directory symlinks)
  • 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 (dart 2.14+, flutter 2.8+):

  • pub throws on any (even ignored) directory symlink beneath package directory
  • consumers doesn't see any difference
  • package developers should apply workarounds to their pipelines
    • it makes common practice of sharing code hard, some examples:
      • common code between ios and macos
      • posix specific code for android, linux, macos, android
      • ffi plugins
    • another use case is to share some project specific scripts, assets or anything else between multiple packages (or respective examples of these packages)
      • we do not forced to publish those files in this case but cannot publish anyway, even .pubignore does not help since this validation is applied prior to filter of ignored files

Summary of solution introduced in #3298

  • developers of packages become happier
    • reverts this regression and would allow to publish packages with valid symlinks or directory symlinks
    • cycles are handled as non-resolving symlink (and prints meaningful description)
  • and all other pros of previous behavior

@jonasfj
Copy link
Member

jonasfj commented Feb 2, 2022

pub throws on any (even ignored) directory symlink beneath package directory

This is definitely something we should fix.


I'm still on the fence about supporting symlink directories, but maybe that's something we should consider. Especially, if cycles can be resolved reasonably on all platforms with all kinds of links (thinking Windows where I don't even recall how many types of symlink-like links there might be).

@2ZeroSix
Copy link
Contributor

2ZeroSix commented Feb 3, 2022

@jonasfj

pub throws on any (even ignored) directory symlink beneath package directory

This is definitely something we should fix.

I extracted these fixes in separate pull request #3300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants