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

Extend analyzer range to 0.34 #1873

Merged
merged 3 commits into from
Oct 8, 2018
Merged

Extend analyzer range to 0.34 #1873

merged 3 commits into from
Oct 8, 2018

Conversation

jcollins-g
Copy link
Contributor

We can't really wait on extending the range anymore to allow for a new analyzer publish to be used with dartdoc. Since we're only publishing alphas at this time this PR shouldn't accidentally add new versions to other users of build unless they specifically request them.

@bwilkerson

@googlebot googlebot added the cla: yes Google is happy with the PR contributors label Oct 2, 2018
@jcollins-g
Copy link
Contributor Author

I'm happy to update pubspecs/CHANGELOG/etc or other housekeeping tasks you'd need done to publish these packages as well. Please let me know if you'd like to do that and if +1 versions are OK for this sort of change for you.

@jakemac53
Copy link
Contributor

The primary issue with doing a constraint like this is we can't shield ourselves against any breakages that come up with the actual 0.33 release (and in fact I think our tests would still just run against the stable 0.32.x versions).

If we need to publish with a version that supports 0.33.0-alpha.0 then I think setting the upper bound to exactly that (<=0.33.0-alpha.0) is preferable. cc @natebosch thoughts?

@natebosch
Copy link
Member

yeah I don't think pub wants to resolve to 0.33.0-alpha.0 if the upper end of the constraint is <0.34.0. I think you want <=0.33.0-alpha.0 on the upper end - but we'll have to bump for every alpha that you need.

A bug fix version bump is fine. That +1 for packages with a leading 0 in the major version, or the last dot for packages with a major version other than 0.

@jcollins-g
Copy link
Contributor Author

@jakemac53 Yes, you're right. If we publish a broken stable package, we'd have to publish a fixed stable package in the same range to correct the issue.

@natebosch That was actually the intent -- to not automatically resolve an alpha version with pub unless a package that depends on build specifies that alpha version intentionally (which would be the case for dartdoc, which is why I am advocating for the change). Right now you can't do that without dependency overrides because of the limited ranges.

@jcollins-g
Copy link
Contributor Author

@natebosch pointed out offline, there is a workaround (removing dev dependencies from dartdoc pre-publish) that would allow a version of dartdoc to be published without this, so this is not technically blocking us.

@natebosch
Copy link
Member

Synced with Jake. We're fine publishing with <0.34.0 if you think there aren't going to be any further breaking changes that will impact us. Though, if that's the case it's not clear why we aren't publishing non-alpha analyzer today - maybe you expect breaking changes that would impact users other than build?

In any case we can merge this once pubspecs and changelogs are updated and I can start publishing.

@jcollins-g
Copy link
Contributor Author

@natebosch Analyzer team tries to avoid publishing non-alpha versions whenever they can, because the impact of non-alpha versions on the ecosystem has historically been very large (each existing version of analyzer directly impacting pub's performance).

@natebosch
Copy link
Member

the impact of non-alpha versions on the ecosystem has historically been very large

Once we publish any package with a constraint of <=0.34.0 I think that impact has been felt. You won't be able to publish 0.33.0-alpha.1 with breaking changes, so you may as well publish 0.33.0 then 0.33.1 etc. I think the new pub solver doesn't care as much if there are a lot of 0.33.x non-alpha versions.

@jcollins-g
Copy link
Contributor Author

Chatted with @bwilkerson and we'll try a stable release of analyzer once some bugfixes currently on deck land. In the meantime, we can hold off on this PR. Sound good?

@natebosch
Copy link
Member

Sounds good. will put it on hold for now.

@jcollins-g
Copy link
Contributor Author

@natebosch Can we take another look now that 0.33.0 is published for analyzer? I have restarted the Travis tests but I don't have access to rekick AppVeyor.

@jakemac53
Copy link
Contributor

Kicked off a new build @jcollins-g, with the new version of analyzer we should be good to go.

@jakemac53 jakemac53 merged commit 2e5803b into master Oct 8, 2018
@jakemac53 jakemac53 deleted the build-analyzer branch October 8, 2018 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google is happy with the PR contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants