Revert https://github.com/dart-lang/pub/pull/3782

[natebosch]

See https://github.com/dart-lang/pub/pull/3782#issuecomment-1478519186

Opening as an issue to have something to track since the PR is closed.

[sigurdm]

cc @jonasfj we need to discuss this. There was a reason we made the change in the first place...

[sigurdm]

I think maybe we could do both resolutions:

• We could do a full resolution using dependency overrides (that includes pubspec_overrides) for the purpose of dart analyzing the project.
• Then do a second resolution against only non-dev dependencies, ignoring all dependency overrides, and if that fails issue a warning (such that it can be ignored for the rare case of cyclic non-dev-dependencies)

@natebosch @jakemac53 WDYT?

[natebosch]

• for the purpose of dart analyzing the project.

If we analyze only the lib/ and bin/ directories, and ignore analysis errors in test/, I'd expect that we'd get the most value from resolving only non-dev dependencies, ignoring overrides, and analyzing the Dart code with that resolution.

[sigurdm]

If we analyze only the lib/ and bin/ directories, and ignore analysis errors in test/, I'd expect that we'd get the most value from resolving only non-dev dependencies, ignoring overrides, and analyzing the Dart code with that resolution.

That would mostly work, but would make it impossible to publish packages with circular non-dev dependencies. Disallowing those might not be bad, but it would be a breaking change.

[jonasfj]

I also lean towards not doing too much analysis here.

Most users won't have dependency_overrides or pubspec_overrides.yaml, and those that do can probably make a tool/publish.sh script that runs appropriate tests before they publish.

Note. if you have pubspec_overrides.yaml, don't check it into git and publish from github actions, then you won't be using pubspec_overrides.yaml when publishing.

[jakemac53]

Note. if you have pubspec_overrides.yaml, don't check it into git and publish from github actions, then you won't be using pubspec_overrides.yaml when publishing.

But we want it to be used for all other actions (and also to be there when you check out the repo).

[Salakar]

FYI, FlutterFire is now blocked and can no longer ship releases, we have several non-dev cyclic dependencies & make use of pubspec_overrides.yaml for local development linking - and can no longer publish a lot of our packages;

Example:

Attempting to publish 2 new versions of packages that are cyclic;

Package Name                  Registry    Local
firebase_ui_oauth             1.2.3       1.2.4
firebase_ui_auth              1.2.3       1.2.4

Result;

firebase_ui_oauth:
Warning: pubspec.yaml has overrides from pubspec_overrides.yaml

Resolving dependencies...
Because firebase_ui_oauth depends on firebase_ui_auth ^1.2.4 which doesn't match any versions, version solving failed.

This has worked for some time until recently (and FlutterFire has had cyclic non-dev dependencies for years), but pub now prevents us from publishing 😢

[domesticmouse]

@mit-mit this is a code red because FlutterFire is currently broken for Flutter 3.10. We need a solution for Firebase this week.

/cc @puf

[jonasfj]

As a workaround you can publish using an old version of the Dart/Flutter SDKs.

@sigurdm, I suspect we need a --skip-validation for publishing, so people don't feel blocked.

As for circular dependencies, we should check pub.dev to see how common they are before we make them harder.

Note: I don't think we're entirely aware why you need circular dependencies. Why not just merge the packages?

[mit-mit]

I wonder if we should revert the change from the beta branch?

[sigurdm]

@Salakar I don't understand what is happening here, what is the content of your pubspec_overrides.yaml?

Taking the pubspec_overrides into account (as 3782 does) should make it easier to publish packages with cyclic dependencies - reverting it should make publishing harder.

[sigurdm]

@mit-mit I don't think this is worth a cherry-pick.

This should not make publishing with circular dependencies harder. This makes the upfront validation a bit looser.

(unless there is an unintended bug of course)

[sigurdm]

Trying to reproduce with a checkout of flutterfire

flutterfire/packages/firebase_ui_oauth > flutter pub publish
[....]
Validating package... (27.7s)
Package validation found the following hints:
* Non-dev dependencies are overridden in pubspec_overrides.yaml.

  This indicates you are not testing your package against the same versions of its
  dependencies that users will have when they use it.

  This might be necessary for packages with cyclic dependencies.

  Please be extra careful when publishing.
* The declared SDK constraint is '>=2.18.0 <3.0.0', this is interpreted as '>=2.18.0 <4.0.0'.

  Consider updating the SDK constraint to:

  environment:
    sdk: '>=2.18.0 <4.0.0'

[Salakar]

@Salakar I don't understand what is happening here, what is the content of your pubspec_overrides.yaml?

dart pub get && dart pub run melos bootstrap at the root of the repository will generate all the overrides for local linking purposes in the monorepo, here's the one for firebase_ui_oauth:

# melos_managed_dependency_overrides: _flutterfire_internals,firebase_auth,firebase_auth_platform_interface,firebase_auth_web,firebase_core,firebase_core_platform_interface,firebase_core_web,firebase_dynamic_links,firebase_dynamic_links_platform_interface,firebase_ui_auth,firebase_ui_localizations,firebase_ui_shared
dependency_overrides:
  _flutterfire_internals:
    path: ../_flutterfire_internals
  firebase_auth:
    path: ../firebase_auth/firebase_auth
  firebase_auth_platform_interface:
    path: ../firebase_auth/firebase_auth_platform_interface
  firebase_auth_web:
    path: ../firebase_auth/firebase_auth_web
  firebase_core:
    path: ../firebase_core/firebase_core
  firebase_core_platform_interface:
    path: ../firebase_core/firebase_core_platform_interface
  firebase_core_web:
    path: ../firebase_core/firebase_core_web
  firebase_dynamic_links:
    path: ../firebase_dynamic_links/firebase_dynamic_links
  firebase_dynamic_links_platform_interface:
    path: ../firebase_dynamic_links/firebase_dynamic_links_platform_interface
  firebase_ui_auth:
    path: ../firebase_ui_auth
  firebase_ui_localizations:
    path: ../firebase_ui_localizations
  firebase_ui_shared:
    path: ../firebase_ui_shared

here's the one for firebase_ui_auth:

# melos_managed_dependency_overrides: _flutterfire_internals,firebase_auth,firebase_auth_platform_interface,firebase_auth_web,firebase_core,firebase_core_platform_interface,firebase_core_web,firebase_dynamic_links,firebase_dynamic_links_platform_interface,firebase_ui_localizations,firebase_ui_oauth,firebase_ui_shared
dependency_overrides:
  intl: ^0.18.0
  _flutterfire_internals:
    path: ../_flutterfire_internals
  firebase_auth:
    path: ../firebase_auth/firebase_auth
  firebase_auth_platform_interface:
    path: ../firebase_auth/firebase_auth_platform_interface
  firebase_auth_web:
    path: ../firebase_auth/firebase_auth_web
  firebase_core:
    path: ../firebase_core/firebase_core
  firebase_core_platform_interface:
    path: ../firebase_core/firebase_core_platform_interface
  firebase_core_web:
    path: ../firebase_core/firebase_core_web
  firebase_dynamic_links:
    path: ../firebase_dynamic_links/firebase_dynamic_links
  firebase_dynamic_links_platform_interface:
    path: ../firebase_dynamic_links/firebase_dynamic_links_platform_interface
  firebase_ui_localizations:
    path: ../firebase_ui_localizations
  firebase_ui_oauth:
    path: ../firebase_ui_oauth
  firebase_ui_shared:
    path: ../firebase_ui_shared

I wonder if it's this change specifically now that has caused this, my team tells me it has happened sometime since 3.7.x Flutter, and downgrading to Flutter 3.3 allows us to publish - so it maybe it is something else that has been introduced recently that is causing this?

cc @Lyokone who may be able to provide more insight. Also, cc @lesnitsky re why we use cyclic dependencies (at least for the UI packages) - my understanding is that it's a workaround due to the lack of peer dependency support in Dart/Pub.

[blaugold]

@Salakar Publishing works with Flutter 3.3 because in it pub publish has less strict requirements for resolving dependencies than in Flutter 3.7. pub publish in Flutter 3.7 did not consider pubspec_overrides.yaml, which would have allowed pub to resolve all dependencies in the FlutterFire repo.

This issue is about reverting a change that makes pub publish take into account pubspec_overrides.yaml. Keeping that change is what would work best for Melos users, such as FlutterFire.

With the version of pub that is included in Flutter 3.10 pubspec_overrides should be used during publishing and publishing should work again.

[Lyokone]

@blaugold thanks for the heads up! We are also in the process of removing the cyclic dependencies in FlutterFire so it's not a problem in the future.

[sigurdm]

Currently we are inclined to not do this.

We like that dependency_overrides and pubspec_overrides are treated the same everywhere.

We suggest a script that temporarily removes the pubspec_overrides file before publishing to validate that a resolution is achievable.