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

Non-nullable Migration #24

Closed
AKushWarrior opened this issue Jun 10, 2020 · 18 comments
Closed

Non-nullable Migration #24

AKushWarrior opened this issue Jun 10, 2020 · 18 comments

Comments

@AKushWarrior
Copy link
Contributor

Dart is migrating to Non-Nullable By Default. This should speed up PointyCastle significantly when it is compiled AOT, so we should begin migrating this library soon.

The feature is still in preview, so there's no hurry, but it's worth considering. We could use bc-kotlin as a model for the transition.

https://medium.com/dartlang/announcing-sound-null-safety-defd2216a6f3

@AKushWarrior
Copy link
Contributor Author

AKushWarrior commented Jun 10, 2020

A Dart dev I know said that Current Dart -> NNBD Dart is sort of like Java -> Kotlin.

Thought it was a good comparison; the code is completely interoperable, but the languages are pretty fundamentally different.

@mwcw
Copy link
Collaborator

mwcw commented Jul 15, 2020

Ok,

While it is good to be on the front foot, I think wait until the language feature is stable.

MW

@AKushWarrior
Copy link
Contributor Author

Ok,

While it is good to be on the front foot, I think wait until the language feature is stable.

MW

Agreed.

@AKushWarrior
Copy link
Contributor Author

@mwcw the null-safety feature is now in beta, and it's recommended for packages to create a prerelease branch which is ported to nullsafety. There has also been a promise of runtime performance improvements, since the language no longer has to perform null checks.

Associated reading:
Feature overview: https://dart.dev/null-safety
Migration guide: https://dart.dev/null-safety/migration-guide

This library is a "core" dependency, so any downstream packages/applications will end up needing this package migrated before they can do so. By starting the process now, we can "get ahead of the curve", so that this library is fully migrated when the feature reaches stable and production applications begin to migrate. It looks like the Dart team has built a migration tool, so this process should be mostly a careful inspection and correction on auto-migration.

@mwcw
Copy link
Collaborator

mwcw commented Nov 26, 2020

Hi,

Ok, I am trying to get back onto the dart project, we are heading towards towards a new round of FIPS certifications and it is consuming all of my time at the moment, although the end is in sight.

I will try and take a look at those links in the next day (Melbourne AU).

MW

@acoutts
Copy link

acoutts commented Dec 14, 2020

@AKushWarrior @mwcw please migrate this with a pre-release like other packages have done. All packages that consume this package cannot migrate until this one is migrated, so all downstream packages are blocked until you set out a nullsafety pre-release as outlined in the migration guide: https://dart.dev/null-safety/migration-guide

@AKushWarrior
Copy link
Contributor Author

@mwcw looks like there's been some organic roadblocks. It'll take a while, but we should definitely start the process.

@acoutts
Copy link

acoutts commented Dec 14, 2020

You can see here that there are currently 108 packages which depend on pointycastle on pub.dev, so all of them are in the same blocked state: https://pub.dev/packages?q=dependency%3Apointycastle

@AKushWarrior
Copy link
Contributor Author

It's also worth noting that pointycastle is one of the most used packages on pub. This is both because of downstream package dependencies and direct users. I think that this is high-urgency given how much code we'd have to port, but this library doesn't use null extensively (so the port should be okay).

@AKushWarrior
Copy link
Contributor Author

Working on this on my fork. Will update with more details as I have them. You can view progress at https://github.com/AKushWarrior/pc-dart

@mwcw to avoid merge conflicts, could you not merge PRs into this repository for a few days? I want to make sure that my fork is fast-forward-safe with this one.

@mwcw
Copy link
Collaborator

mwcw commented Jan 20, 2021

Hi,

Yep I can do that, did you by chance see an email from me?

MW

@AKushWarrior
Copy link
Contributor Author

Haven't seen one. What address did you send it to?

Typically I use akushwarrior@gmail.com for open source stuff.

@mwcw
Copy link
Collaborator

mwcw commented Jan 20, 2021

Hi,

I have send you another email, I am not sure what happened.

MW

@AKushWarrior
Copy link
Contributor Author

AKushWarrior commented Jan 20, 2021

It looks like the majority of tests are passing on my fork. It's at 435 passing, 16 failing currently. I think that this should be ready for prerelease by the end of tomorrow, assuming all goes well.

EDIT: 449 passing, 2 failing. The remaining issues are with AES/CBC_CMAC/PKCS7 and RSA key generation. I'm not really sure how to solve them, but I'll give it a shot later.

@mwcw
Copy link
Collaborator

mwcw commented Jan 21, 2021

Sure let me know!

Given the size of the change I think it should go out as 2.1.0

MW

@AKushWarrior
Copy link
Contributor Author

Sure let me know!

Given the size of the change I think it should go out as 2.1.0

MW

It should be in the form "2.1.0-null-safety" or something of that sort. The dart docs specify; I'll get it set up before I finalize the PR.

@AKushWarrior
Copy link
Contributor Author

@mwcw this is taking a really long time. Not sure where these last couple errors are coming from. Going to do some diffs with the old code today and see if that helps.

@AKushWarrior
Copy link
Contributor Author

AKushWarrior commented Jan 28, 2021

@mwcw @acoutts The migration is finished and the PR is filed. I've taken great care to ensure that the package is publish-ready; all the logistical problems, dependency management, versioning, etc. have been taken care of along with the migration itself.

I declared this version to be 3.0.0 because of this guideline: https://dart.dev/null-safety/migration-guide#package-version. That versioning bump makes sense to me because this is fundamentally a breaking change for downstream users, in that their code would have to be migrated to use this package. That they would have migrated anyways is immaterial to semantic versioning :)

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

No branches or pull requests

4 participants