-
Notifications
You must be signed in to change notification settings - Fork 75
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
Reset webdev
, dwds
, test_common
after publishing
#2048
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Elliott, this is very helpful for releases!
I left a couple of comments.
test_common/pubspec.yaml
Outdated
@@ -6,16 +6,11 @@ environment: | |||
sdk: ">=3.0.0-134.0.dev <4.0.0" | |||
|
|||
dependencies: | |||
dwds: 18.0.2 | |||
dwds: ^18.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could just say "any" so this never breaks? This package is never published and the packages that import it have their own dwds constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,3 @@ | |||
dependency_overrides: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file needed? Looks like we already import local dwds in the pubspec file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, deleted this file!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, with this removed I see:
Because every version of test_common from path depends on dwds from hosted and frontend_server_common depends on dwds from path, test_common from path is forbidden.
So, because frontend_server_common depends on test_common from path, version solving failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's strange:) Some weirdness of the constraint solver I guess. We can also try "any" for dwds in the pubspec (since this package is never published an its users already have their constraints for dwds I think it will work), or bring the pubspec_overrides back, if you you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think we just can follow what you did for test_common - change the pubspec one to "any" and keep the pubspec_overrides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
@@ -21,8 +21,3 @@ dev_dependencies: | |||
test_descriptor: ^2.0.0 | |||
test_process: ^2.0.0 | |||
vm_service: ^8.0.0 | |||
|
|||
dependency_overrides: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this break on release of frontent_server_client if the condition in the comment is not met yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think this can now be removed, I see dart-lang/test#1753 and #1762 (comment) both indicating that. @jakemac53 to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left another comment but it looks good anyway!
@@ -0,0 +1,3 @@ | |||
dependency_overrides: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's strange:) Some weirdness of the constraint solver I guess. We can also try "any" for dwds in the pubspec (since this package is never published an its users already have their constraints for dwds I think it will work), or bring the pubspec_overrides back, if you you prefer.
Also closes #2040 to use
pubspec_overrides
for packages