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

Enable dart non nullable by default (NNBD) #3116

Merged
merged 33 commits into from
May 6, 2021
Merged

Conversation

renancaraujo
Copy link

@renancaraujo renancaraujo commented Mar 11, 2021

This PR (tries) to make the dart runtime comply with the standards of its 2.12 version.

#3114

@renancaraujo renancaraujo changed the title Enable dart non nullable by default (NNBD) [WIP] Enable dart non nullable by default (NNBD) Mar 11, 2021
@lingyv-li
Copy link
Member

@renancaraujo @canastro thanks for your contribution!

So I used a trick to cache the dart packages mapping (.packages file), so we don't need to run pub get for each test case to reduce the test execution time. Find the code around cacheDartPackages in BaseDartTest.java.

Since a later Dart version, there was one more file that needs to be cached .dart_tool/package_config.json, otherwise it will not have the dart version information . Here is the code I got to do that:
lingyv-li@d78f54e#diff-eaff4da8b01719d6eba9cba3629f8760482002ffe3bde96729b31181ce7b1f7d

@@ -19,14 +19,14 @@ class DFASerializer {

@override
String toString() {
if (dfa.s0 == null) return null;
if (dfa.s0 == null) return 'null';
Copy link
Contributor

Choose a reason for hiding this comment

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

Copied this from a earlier PR that attempted to upgrade to null safety

runtime/Dart/lib/src/tree/src/tree.dart Outdated Show resolved Hide resolved
runtime/Dart/lib/src/tree/src/tree.dart Outdated Show resolved Hide resolved
@renancaraujo renancaraujo changed the title [WIP] Enable dart non nullable by default (NNBD) Enable dart non nullable by default (NNBD) Mar 24, 2021
@canastro
Copy link
Contributor

@ericvergnaud this PR is ready for review, please let us know if you can help out. Thanks

@stefanschaller
Copy link

@canastro @ericvergnaud @lingyv-li Any news? Would be great to see progress with this pr

@ericvergnaud
Copy link
Contributor

@canastro @ericvergnaud @lingyv-li Any news? Would be great to see progress with this pr

Hi,
no comment, but the CI isn't green.
Maybe you need to rebase?

@stefanschaller
Copy link

stefanschaller commented Apr 20, 2021

Maybe you need to rebase?

@canastro
Some steps of the pipeline still seems to fail.

@canastro
Copy link
Contributor

hey @stefanschaller and @ericvergnaud missed that last notification. I'll look into it

@renancaraujo
Copy link
Author

Since master is unstable, we got some red builds here as well.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented May 6, 2021

The dart CI has moved from travis to circle-ci so you would need to rebase and get a green build before a merge can be envisaged

@stefanschaller
Copy link

stefanschaller commented May 6, 2021

The dart CI has moved from travis to circle-ci so you would need to rebase and get a green build before a merge can be envisaged

@ericvergnaud
The go-runtime step of the continuous-integration/appveyor/pr will fail for sure because there seems to be an issue in the master branch.
But this issue has nothing to do with this branch, correct? So what do you propose?

All pipeline steps that are related to the dart project, the feedback from the reviewers and contributors and everything is properly working / is done. What should we do? I mean, it's not the best idea that a go noob try to fix the appveyor pipeline for go, right?

@ericvergnaud
Copy link
Contributor

The dart CI has moved from travis to circle-ci so you would need to rebase and get a green build before a merge can be envisaged

@ericvergnaud
The go-runtime step of the continuous-integration/appveyor/pr will fail for sure because there seems to be an issue in the master branch.
But this issue has nothing to do with this branch, correct? So what do you propose?

All pipeline steps that are related to the dart project, the feedback from the reviewers and contributors and everything is properly working / is done. What should we do? I mean, it's not the best idea that a go noob try to fix the appveyor pipeline for go, right?

I'm asking that you reenable circle-ci such that we can check if the dart target works fine on both Linux and Windows.
Some other guys are taking care of the go target.

@stefanschaller
Copy link

The dart CI has moved from travis to circle-ci so you would need to rebase and get a green build before a merge can be envisaged

@ericvergnaud
The go-runtime step of the continuous-integration/appveyor/pr will fail for sure because there seems to be an issue in the master branch.
But this issue has nothing to do with this branch, correct? So what do you propose?
All pipeline steps that are related to the dart project, the feedback from the reviewers and contributors and everything is properly working / is done. What should we do? I mean, it's not the best idea that a go noob try to fix the appveyor pipeline for go, right?

I'm asking that you reenable circle-ci such that we can check if the dart target works fine on both Linux and Windows.
Some other guys are taking care of the go target.

How to enable the pipeline that you are talking about? Is there a README, tutorial or something like that?

@canastro
Copy link
Contributor

canastro commented May 6, 2021

Not quite sure if this is it, but I added dart here: c201905

@ericvergnaud
Copy link
Contributor

The dart CI has moved from travis to circle-ci so you would need to rebase and get a green build before a merge can be envisaged

@ericvergnaud
The go-runtime step of the continuous-integration/appveyor/pr will fail for sure because there seems to be an issue in the master branch.
But this issue has nothing to do with this branch, correct? So what do you propose?
All pipeline steps that are related to the dart project, the feedback from the reviewers and contributors and everything is properly working / is done. What should we do? I mean, it's not the best idea that a go noob try to fix the appveyor pipeline for go, right?

I'm asking that you reenable circle-ci such that we can check if the dart target works fine on both Linux and Windows.
Some other guys are taking care of the go target.

How to enable the pipeline that you are talking about? Is there a README, tutorial or something like that?

You might need to rebase
I see the go PRs get built but not this one.

@canastro
Copy link
Contributor

canastro commented May 6, 2021

@ericvergnaud I just re-checked to be sure, its rebased

@parrt
Copy link
Member

parrt commented May 6, 2021

Looks like DART is green and @lingyv-li appears to approve. Merging. thanks guys!

@parrt parrt merged commit 0a1c3e3 into antlr:master May 6, 2021
@jnhyatt
Copy link

jnhyatt commented May 6, 2021

What ANTLR version will this make it into?

@parrt
Copy link
Member

parrt commented May 6, 2021

We can shoot for a version in June, right @ericvergnaud ?

@ericvergnaud
Copy link
Contributor

ericvergnaud commented May 6, 2021 via email

@ericvergnaud
Copy link
Contributor

Hi,

looks like the merge broke the master build?
@lingyv-li @canastro would you mind having a look?

@canastro
Copy link
Contributor

@ericvergnaud in circleci? can you point me a link to the broken build? the only thing I found seemed to be green: https://app.circleci.com/pipelines/github/antlr/antlr4?branch=master

@ericvergnaud
Copy link
Contributor

@canastro
Copy link
Contributor

@ericvergnaud that link is pointing to a build of a PR that is still open: #3191

@lingyv-li
Copy link
Member

lingyv-li commented May 28, 2021

+1 I believe that PR introduced the issue

@canastro
Copy link
Contributor

@lingyv-li can we have a release with this to pub.dev?

@lingyv-li
Copy link
Member

I could only make a prerelease, since the version needs to match the antlr version.
Would you be able to use the it as git dependency source?

@canastro
Copy link
Contributor

Yup, going with that for now 👍🏽

@jnhyatt
Copy link

jnhyatt commented Oct 11, 2021

What's the schedule for a 4.9.3 release?

@parrt
Copy link
Member

parrt commented Oct 11, 2021

@ericvergnaud Maybe we should try for a release this Friday?

@parrt parrt added this to the 4.9.3 milestone Oct 11, 2021
@ericvergnaud
Copy link
Contributor

ericvergnaud commented Oct 11, 2021 via email

@parrt
Copy link
Member

parrt commented Oct 11, 2021

I decided to start working towards the process now... If you could find things we need to merge or any other cleanup it would be great. then this week when everything looks stable, I can cut the release when I find time.

thanks again to everyone for all of your hard work!

@canastro
Copy link
Contributor

canastro commented Oct 14, 2021

@lingyv-li can you make a pre-release of the dart runtime to pub.dev?
I'm having some issues with installing from git in our pipeline (I've already tried to fix it by adding a ssh private key, but no luck).

@lingyv-li
Copy link
Member

Sure, 4.9.3-dev.1 was released using 23f93e0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants