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

Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc #311

Merged
merged 3 commits into from May 10, 2022
Merged

Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc #311

merged 3 commits into from May 10, 2022

Conversation

nordlow
Copy link
Contributor

@nordlow nordlow commented May 9, 2022

No description provided.

@nordlow nordlow changed the title Qualify pegged/peg.d usig @safe: at the top Qualify pegged/peg.d May 9, 2022
@nordlow nordlow changed the title Qualify pegged/peg.d WIP: Qualify pegged/peg.d May 9, 2022
@nordlow nordlow changed the title WIP: Qualify pegged/peg.d Qualify pegged/peg.d as @safe and when possible pure May 9, 2022
@nordlow nordlow changed the title Qualify pegged/peg.d as @safe and when possible pure Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc May 9, 2022
@nordlow nordlow changed the title Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc WIP: Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc May 9, 2022
@nordlow nordlow changed the title WIP: Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc Qualify pegged/peg.d as @safe and when possible pure nothrow @nogc May 9, 2022
@nordlow
Copy link
Contributor Author

nordlow commented May 9, 2022

./ci.sh passes for me on Ubuntu 20.04 now, @John-Colvin.

Copy link
Collaborator

@veelo veelo left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

pegged/dynamic/peg.d Show resolved Hide resolved
pegged/parser.d Show resolved Hide resolved
pegged/peg.d Show resolved Hide resolved
pegged/peg.d Show resolved Hide resolved
@veelo
Copy link
Collaborator

veelo commented May 9, 2022

It seems to me that the CI failures are due to deficiencies in moderately recent (2 years old) druntime.

I am thinking of determining the release at which this starts working a bit more precisely, putting that in a toolchainRequirements section, and releasing this with a mayor version bump. Would that work for Symmetry @John-Colvin? What is the oldest D frontend release that you depend on?

@nordlow
Copy link
Contributor Author

nordlow commented May 9, 2022

It seems to me that the CI failures are due to deficiencies in moderately recent (2 years old) druntime.
I am thinking of determining the release at which this starts working a bit more precisely, putting that in a toolchainRequirements section, and releasing this with a mayor version bump. Would that work for Symmetry @John-Colvin? What is the oldest D frontend release that you depend on?

Yes, specifically when AA.keys being @system. I've added some versioning to solve some of them at 9fb4d41.
I doubt we are in need of supporting a pre-2.091 toolchain. Right @John-Colvin? If so I can remove the versioning I just added at 9fb4d41.

@nordlow
Copy link
Contributor Author

nordlow commented May 9, 2022

Hitting 2.091-version compiler bugs aswell at https://ci.appveyor.com/project/PhilippeSigaud/pegged/builds/43483820/job/3503ke4jawrotjej#L83.

@John-Colvin
Copy link
Contributor

We only care about the compilers we actively support (@nordlow the ones our setup scripts install)

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

CI is green now. In appveyor.yml, I suggest removing support for dmd 2.087.1 and bump 2.090.1 to 2.091.0 having @safe AA.keys which avoids the need for the versioning added at 9fb4d41.

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

We only care about the compilers we actively support (@nordlow the ones our setup scripts install)

The oldest compiler version "we" need support for is version 2.099.1. So to which compiler version less or equal to <= 2.099.1 do you want to bump matrix-entries in appveyor.yml to, @veelo?

Copy link
Collaborator

@veelo veelo left a comment

Choose a reason for hiding this comment

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

Can you please react to #311 (comment)? I think it fits into this PR. If you don't want to do it then that's fine, and I'll see if I can do a follow-up PR.

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

Can you please react to #311 (comment)? I think it fits into this PR. If you don't want to do it then that's fine, and I'll see if I can do a follow-up PR.

Sorry, missed that. I'll have look.

@veelo
Copy link
Collaborator

veelo commented May 10, 2022

So to which compiler version less or equal to <= 2.099.1 do you want to bump matrix-entries in appveyor.yml to, @veelo?

I think we should try to support as far back as we reasonably can, so like you suggest:

removing support for dmd 2.087.1 and bump 2.090.1 to 2.091.0

but then also make that explicit with a toolchainRequirements section section in the Dub configuration.

I'll have to think about whether that demands a major version bump, since technically that would brake the build for people that don't update their compiler regularly. I'll ask the forum.

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

Correction: AA.keys() became @safe version in 2.098.0 as per https://dlang.org/changelog/2.098.0.html#bugfix-list referencing https://issues.dlang.org/show_bug.cgi?id=14439. Gonna update.

@veelo
Copy link
Collaborator

veelo commented May 10, 2022

I'll ask the forum.

https://forum.dlang.org/post/pxyqgiyosabquyjzwxex@forum.dlang.org

@veelo veelo merged commit 5c5b9e9 into PhilippeSigaud:master May 10, 2022
@veelo
Copy link
Collaborator

veelo commented May 10, 2022

I cannot release this yet, since parser.d is a generated parser (Pegged generates its own parser). Whenever parser.d is regenerated, the @safe annotations will be reverted from it. Pegged will need to be modified to generate @safe parsers.

I really need to review the CI so that this is covered in the future.

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

I cannot release this yet, since parser.d is a generated parser (Pegged generates its own parser). Whenever parser.d is regenerated, the @safe annotations will be reverted from it. Pegged will need to be modified to generate @safe parsers.

Can you give me a sample command line call that does this? Then I can dig into having the generated parser be @safe as well.

@veelo
Copy link
Collaborator

veelo commented May 10, 2022

It is in https://github.com/PhilippeSigaud/Pegged/tree/master/pegged/dev. But that doesn't work anymore because I have accepted too many PRs that weren't sound :-(
The command is currently

cd pegged/dev
rdmd -I../.. -I../../examples/peggedgrammar/src -I../../examples/misc/src regenerate.d

but that reverses some changes from some earlier PRs. It's a mess.

If you are waiting for this, I can tag a new release in the mean time while I clean this up. It has been this way since a few releases. Would a beta work for you?

@veelo
Copy link
Collaborator

veelo commented May 10, 2022

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

https://github.com/PhilippeSigaud/Pegged/releases/tag/v0.4.6

Awesome! Thank you!

Is there anything more you need help with?

@nordlow
Copy link
Contributor Author

nordlow commented May 10, 2022

Can you please bump https://code.dlang.org/packages/pegged tag aswell to 0.4.6? It's still at 0.4.5.

@veelo
Copy link
Collaborator

veelo commented May 10, 2022

Is there anything more you need help with?

Seriously? There is a lot to do actually, but most of it requires a deeper dive. We can talk about those if you are interested.

Otherwise, I'd really like an automated check on PRs to make sure the committed parser.d is the same as one that is generated.

@veelo
Copy link
Collaborator

veelo commented May 10, 2022

Can you please bump https://code.dlang.org/packages/pegged tag aswell to 0.4.6? It's still at 0.4.5.

To my knowledge, it should update automatically. It probably scans for new tags every so many hours.

@nordlow
Copy link
Contributor Author

nordlow commented May 11, 2022

To my knowledge, it should update automatically. It probably scans for new tags every so many hours.

Still no bump to 0.4.6 at https://code.dlang.org/packages/pegged. Can you bump it yourself, please? Are you the only one who has permissions to do so? FYI, @skoppe.

@veelo
Copy link
Collaborator

veelo commented May 11, 2022

No, I don't even have an account. I think that one is still with @PhilippeSigaud. I am wondering if it is because the other releases have a commit signature icon, but I don't know why the latest doesn't.

@nordlow
Copy link
Contributor Author

nordlow commented May 11, 2022

No, I don't even have an account. I think that one is still with @PhilippeSigaud. I am wondering if it is because the other releases have a commit signature icon, but I don't know why the latest doesn't.

Do you have any clues as to why, @John-Colvin @skoppe?

@veelo
Copy link
Collaborator

veelo commented May 11, 2022

I don't think the signature has anything to do with it, other packages that did update in the Dub registry within the last 16 hours don't have that icon either. I've checked the syntax of the tag multiple times, should be no problems there. I made sure that the pre-release checkbox is unchecked. The Dub docs say that tags are scanned about twice an hour. This is not my first release, and I don't think I did anything different from last time.

@nordlow
Copy link
Contributor Author

nordlow commented May 11, 2022

I don't think the signature has anything to do with it, other packages that did update in the Dub registry within the last 16 hours don't have that icon either. I've checked the syntax of the tag multiple times, should be no problems there. I made sure that the pre-release checkbox is unchecked. The Dub docs say that tags are scanned about twice an hour. This is not my first release, and I don't think I did anything different from last time.

Do you have any clues, @CyberShadow?

@CyberShadow
Copy link

I remember that the updater used to sometimes get stuck. Sonke would come in and restart it. However, I just tried to update my own package and it succeeded, so I don't think it's that in this case. It seems to have trouble with this specific package.

A package admin could go to https://code.dlang.org/my_packages/pegged and perhaps that page would show an error message.

@s-ludwig Seems that we need your help again; also, if there's anyone else who could help here, perhaps they should be listed on https://wiki.dlang.org/People#Points_of_contact .

@nordlow
Copy link
Contributor Author

nordlow commented May 11, 2022

Further note that https://github.com/PhilippeSigaud has been inactive for at least one year.

@veelo
Copy link
Collaborator

veelo commented May 11, 2022

Maybe it's time to transfer Pegged to https://github.com/dlang-community? I'm fine with continuing as a maintainer of Pegged. At least then there is a group of people that are able to pick up the ball it case it is dropped. Is there a shared dub account for those projects? What is the procedure for such a transfer?

@nordlow
Copy link
Contributor Author

nordlow commented May 11, 2022

Maybe it's time to transfer Pegged to https://github.com/dlang-community? I'm fine with continuing as a maintainer of Pegged. At least then there is a group of people that are able to pick up the ball it case it is dropped. Is there a shared dub account for those projects? What is the procedure for such a transfer?

I vote for that. Ping, @John-Colvin @RazvanN7.

@skoppe
Copy link
Contributor

skoppe commented May 11, 2022

I remember that the updater used to sometimes get stuck

Yes, I have had this happen repeatably the last year. Sometimes it works, sometimes it doesn't. The page says its processing, but nothing happens (for days) unless I manually hit the button, all the while other packages are just updating.

@veelo
Copy link
Collaborator

veelo commented May 12, 2022

It finally got through now.

@nordlow
Copy link
Contributor Author

nordlow commented May 12, 2022

It finally got through now.

Thank you. Was it thanks to somebody manually refreshing the tag on code.dlang.org?

@veelo
Copy link
Collaborator

veelo commented May 12, 2022

Was it thanks to somebody manually refreshing the tag on code.dlang.org?

Don't know. There was an issue with an expired certificate, but I doubt it is related because other packages had no issue.

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

Successfully merging this pull request may close these issues.

None yet

5 participants