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.

Sign up for GitHub

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

Jump to bottom

allow the null safe built_* package versions #2967

Merged
merged 5 commits into from
Jan 7, 2021
Merged

allow the null safe built_* package versions #2967

merged 5 commits into from
Jan 7, 2021

Conversation

jakemac53
Copy link
Contributor

This overrides the deps to point at the git repo since these are not yet published for now.

A few open questions before we land/publish though:

  • Is the generated code from the null safe built_value_generator compatible with the older (7.x.x) built_value and built_collection versions? If not we won't be able to do a range like this.
  • Do we want to publish built_value/built_value_generator or this package first? One or the other will be in a bit of a weird state at least initially (publishing a version that nobody can get immediately), due to the dependency cycle.
  • Who should own migrating code_builder? That also uses built_value/built_collection and we also depend on it.

@jakemac53 jakemac53 marked this pull request as draft January 5, 2021 16:57
@google-cla google-cla bot added the cla: yes Google is happy with the PR contributors label Jan 5, 2021
@jakemac53
Copy link
Contributor Author

Tests answered the question for me, lol https://github.com/dart-lang/build/pull/2967/checks?check_run_id=1651376388. So the newly generated code is not compatible.

I will try the old generated code with the new libs next.

@jakemac53
Copy link
Contributor Author

It looks like the new libs with the old generated code does work - so we can go ahead with that if @davidmorgan promises not to break that compatibility :).

@natebosch
Copy link
Member

I'm OK with the approach as long as @davidmorgan is.

davidmorgan
davidmorgan approved these changes Jan 6, 2021
View reviewed changes
build_daemon/CHANGELOG.md Outdated
# 2.1.5

- Allow the null safe built_collection, built_value, and built_value_generator
packages, and regenerate serialization code with them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment?

@davidmorgan
Copy link
Contributor

Yes, old generated code will continue to work with new built_value; I hadn't thought about it before, but I guess that's going to be needed by other packages with generated code checked in, otherwise they won't be able to depend on ranges and everything will be very stuck.

Thanks.

@jakemac53 jakemac53 marked this pull request as ready for review January 6, 2021 18:59
@jakemac53
Copy link
Contributor Author

I realized that there is a published version of built_value so I am trying CI with that and then I can remove the overrides and land this assuming it is all good.

@jakemac53 jakemac53 mentioned this pull request Jan 6, 2021
Merged
@jakemac53 jakemac53 changed the title allow the null safe built_* package versions, and regenerate code using them allow the null safe built_* package versions Jan 6, 2021
@davidmorgan
Copy link
Contributor

Yes, should have mentioned built_value was published. LG, thanks!

@jakemac53 jakemac53 merged commit 0eab40e into master Jan 7, 2021
@jakemac53 jakemac53 deleted the allow-latest-built-value branch January 7, 2021 15:16
jakemac53 added a commit that referenced this pull request Jan 7, 2021
@jakemac53
Update many packages to allow the latest null safety deps (#2970)
1cc434b 
- Removes json_serializable annotations and the entire dependency in some cases - we were already no longer depending on the source generation and the new version has changes that break our CI due to deprecations.
- Did not update the `glob` package since we can't support the old and new version
- built_value and built_collection were updated separately in #2967
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natebosch natebosch natebosch approved these changes

@davidmorgan davidmorgan davidmorgan approved these changes

Assignees
No one assigned
Labels
cla: yes Google is happy with the PR contributors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakemac53 @natebosch @davidmorgan