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

Allow sdk packages #7609

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Allow sdk packages #7609

merged 2 commits into from
Apr 5, 2024

Conversation

jonasfj
Copy link
Member

@jonasfj jonasfj commented Apr 2, 2024

Fixing the issue where packages with SDK-dependencies can't be published.

CC @jakemac53 (I believe we'll have a workaround for _macros out today'ish; this will ship thursday or next week)


Noticed some slow tests and ended up filing:
#7608

@jonasfj jonasfj requested a review from isoos April 2, 2024 12:27
final tarball = await packageArchiveBytes(
pubspecContent: generatePubspecYaml('xyz', '1.2.3') +
' my_sdk_dep:\n'
' sdk: dart\n');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allow package to have any dependency as long as they have sdk: dart (or sdk:flutter)?

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Should we remove the special-case allow-listing of the macros package also?

@isoos
Copy link
Collaborator

isoos commented Apr 2, 2024

Should we remove the special-case allow-listing of the macros package also?

We should probably have a tool or test that checks allow-listing regularly and indicates which to remove.

@jonasfj
Copy link
Member Author

jonasfj commented Apr 2, 2024

As far as SDK packages are concerned this will allow dependency on any SDK package.

I think that's fine, you can pick one that doesn't exist, and then the package won't work for anyone. That's sad, but probably people will test their package first 🙈

We could make a list of allowed SDK-dependencies. But I'm not sure it's necessary.

@jakemac53
Copy link
Contributor

LGTM, will the other fix (to add _macros to the reserved name list) not fix the problem for publishing then? Or do you think it will?

@jonasfj jonasfj merged commit 19ded2e into dart-lang:master Apr 5, 2024
31 checks passed
@jonasfj jonasfj deleted the allow-sdk-packages branch April 5, 2024 08:42
@jonasfj
Copy link
Member Author

jonasfj commented Apr 5, 2024

will the other fix (to add _macros to the reserved name list) not fix the problem for publishing then?

That fix will allow you to publish _macros

@jakemac53
Copy link
Contributor

That fix will allow you to publish _macros

Did you mean macros? Fwiw, I still can't publish right now, but I don't know if it has been deployed yet.

@isoos
Copy link
Collaborator

isoos commented Apr 5, 2024

Did you mean macros? Fwiw, I still can't publish right now, but I don't know if it has been deployed yet.

We were not able to deploy it yet. (needed to roll back)

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

Successfully merging this pull request may close these issues.

4 participants