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 publish --dry-run not surfacing all package errors #2828

Closed
ditman opened this issue Jan 13, 2021 · 7 comments
Closed

pub publish --dry-run not surfacing all package errors #2828

ditman opened this issue Jan 13, 2021 · 7 comments

Comments

@ditman
Copy link
Member

ditman commented Jan 13, 2021

Environment

  • pub version or flutter pub version: Pub 2.12.0-214.0.dev
  • OS version: Linux (doesn't matter)
  • Are you using the Chinese community mirror or a corporate firewall? No

Problem

We're attempting to publish this Plugin:

However when I do pub publish on it, I get the following error:

...
Do you want to publish file_selector_web 0.7.0 (y/N)? y
Uploading... (1.1s)

pubspec.yaml allows Flutter SDK version prior to 1.20.0, which does not support having no `ios/` folder.
Please consider increasing the Flutter SDK requirement to ^1.20.0 or higher (environment.sdk.flutter) or create an `ios/` folder.

See https://flutter.dev/docs/development/packages-and-plugins/developing-packages#plugin

That's fine, and the error text is helpful (and I have a PR addressing the issue), however if I run pub publish -n in the same package:

...
Package has 0 warnings.

We use pub publish -n as a CI verification step for incoming pull requests to flutter/plugins. Is there anything else we need to do to get those server-side errors surfaced as well? We allowed a merge that is not "publishable" (and we'd like to avoid that).

Expected behavior

I would expect pub publish --dry-run to surface the same errors that pub publish eventually spits.

Actual behavior

pub publish --dry-run seems to only run a subset of the validations that pub publish does.

@ditman
Copy link
Member Author

ditman commented Jan 13, 2021

This seems like the same issue as: #2743, just with a different validation failing.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 29, 2021

The issue is that these checks happen on the server. Some checks could have a duplicate implementation in the client - but for some checks they might be specific to our server (another server might have less strict license requirements).

Also checks that happen in the client are there in that version of the sdk forever - while those on the server are easy to change moving forward.

Maybe we should have dry-run do a validating upload to the server... not sure - we would probably need another api end-point for that.
@jonasfj @isoos WDYT?

@isoos
Copy link
Collaborator

isoos commented Jan 29, 2021

Maybe having a separate package for the validations would be the best here. It could be a dev or global dependency that CI runs, and while we may tie it to an SDK via the pub tool, it could be kept up-to-date when needed.

@RedBrogdon
Copy link
Contributor

Just ran into this myself while updating https://pub.dev/packages/rebloc.

Looks like you're all over it, but here's my versions just in case:

Flutter 2.0.1 • channel stable • https://github.com/flutter/flutter.git
Framework • revision c5a4b4029c (2 days ago) • 2021-03-04 09:47:48 -0800
Engine • revision 40441def69
Tools • Dart 2.12.0

@jonasfj jonasfj added type-enhancement A request for a change that isn't a bug contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) low priority labels Mar 12, 2021
@jonasfj
Copy link
Member

jonasfj commented Mar 12, 2021

We can totally add a validation for:

pubspec.yaml allows Flutter SDK version prior to 1.20.0, which does not support having no ios/ folder.

We didn't do this client side initially because we wanted to have to ability back it out quickly if it caused problems for anyone.

Implementing it client side is no big deal, it only affects plugin authors who are cleaning up old plugins with ios/ folders that aren't needed -- so I'm not sure it's high priority.

@jonasfj jonasfj removed type-enhancement A request for a change that isn't a bug contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) low priority labels Mar 12, 2021
@jonasfj
Copy link
Member

jonasfj commented Mar 12, 2021

Is there anything else we need to do to get those server-side errors surfaced as well?

No, server side checks like this cannot be surfaced client side.
The most important checks should all exist client side.

It's mostly when we do things out-of-band that doing them serverside first is a good idea. Or if we do checks we might want to revert.


Regarding this specific check, it:

  • Only affects Flutter plugin authors,
  • Who supports Flutter SDKs prior to 1.20,
  • Have decided to remove their ios/ folder.

I'm not sure it's worth the effort to implement this validation two places. Please reopen if I'm missing a good reason.

It's not ideal to first get errors when publishing, but number of users affected by this seems relatively small.
I could even imagine that we might remove this check server-side one day, because if you publish plugins with lower bound constraint on flutter that allows <1.20, then you better test against such flutter versions too -- which would reveal this issue.

@jonasfj jonasfj closed this as completed Mar 12, 2021
@jonasfj
Copy link
Member

jonasfj commented Mar 12, 2021

I don't disagree with the overall issue:

pub publish --dry-run not surfacing all package errors

And when we see other issues I'm happy to add more validators to pub.

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

No branches or pull requests

5 participants