Release 0.20.1 #842

Pat-Lafon · 2023-10-16T21:49:57Z

This is an attempt to "kick-the-tires" on a new release based on https://github.com/orgs/lalrpop/discussions/823.

I did an initial draft of the release notes in RELEASES.md and ran ./version.sh to update the version number in all the places.

dburgener

Thanks for putting this together! I think it looks mostly good from my perspective. Just a couple of minor notes on the changelog.

RELEASES.md

dburgener · 2023-10-16T22:37:53Z

The notes look good, however I gave it one final test, and I'm seeing problems. If I build using the commit before #814, everything works, but #814 without unicode support is translating my regexes containing ^ into an enumeration of all the possible unicode characters except excluded ones, and then panicing at run time upon encountering the unicode. I'm looking into a root cause and fix, but I'm not sure if I'll be able to get to it tonight.

dburgener · 2023-10-17T00:13:36Z

I'm out of time for tonight unfortunately. I'm getting weird failures both with and without unicode when trying to run tests in https://github.com/dburgener/cascade against lalrpop e2eb361, but not against e2eb361^.

When I run without unicode, I get the unexpected unicode on ^ patterns I mentioned above. When I run with unicode, a few of my error tests for non-parsing input don't terminate in a reasonable amount of time.

I'm trying to get a simple reproducer that's more pure-lalrpop to narrow down what's wrong, but I haven't been successful at that yet. It's not clear to me at this point why my code is failing but the lalrpop tests aren't. Perhaps its user error on my end. I'll take a look in the morning and follow up then.

yannham

LGTM, beside side suggestions. I don't approve in order to wait for @dburgener to sort his tests out before potentially merging this.

yannham · 2023-10-17T15:14:29Z

doc/calculator/Cargo.toml

@@ -1,12 +1,12 @@
 [package]
 name = "calculator"
-version = "0.20.0"
+version = "0.20.1"


Can't we use something like:

version.workspace = true

if this crate always follows the version of the main package of the workspace? This would save future version bumps.

That's a fair point. It doesn't seem like any of these examples get rendered in the lalrpop book so it seems fine.

Actually on that point, maybe we should be pointing to these examples in the book instead of the tests when possible(referring to http://lalrpop.github.io/lalrpop/tutorial/index.html). Otherwise, I'm not sure how much visibility some of these get.

I've spawned this off to it's own pr to be discussed/merged separately.

yannham · 2023-10-17T15:15:29Z

doc/calculator/Cargo.toml


 [dependencies]
-lalrpop-util = { version = "0.20.0", path = "../../lalrpop-util", features = ["lexer", "unicode"] }
+lalrpop-util = { version = "0.20.1", path = "../../lalrpop-util", features = ["lexer", "unicode"] }


Similarly, can we do something like:

lalrpop-util = { workspace = true, features = [ "lexer", "unicode" ] }

here, and put in the main Cargo.toml something like:

lalrpop-util = { version = "0.20.1", path = "./lalrpop-util"}

Sorry, it might be out of scope of this PR, but I've been doing that in other projects (define workspace-level dependencies for workspace members, and a workspace-level version, and use that everywhere else so that you only have to bump each member's, and only if it doesn't inherit the workspace's main version) and I wonder if it could make the workflow simpler for LALRPOP. Feel free to tell me that we can see that later 🙂

yannham · 2023-10-17T15:22:19Z

version.sh

@@ -3,6 +3,8 @@
 # A script to bump the version number on all Cargo.toml files etc in
 # an atomic fashion.

+set -e -o pipefail


Ah, I only see this script now. So maybe it's not really a hassle if it's automatized, although the script feels a bit ad-hoc.

It is somewhat automated, in a kinda hacky way. I think we could do a little bit better here in terms of setting up the release process steps. Not sure what big crates do in this regard. Maybe something that includes a mini-crater run or cargo-semvar-checks to help catch any last minute issues.

dburgener · 2023-10-17T17:48:37Z

I think I have a solution for the issues I'm seeing. I haven't tested it end to end yet, but limited testing I've done looks promising.

I believe the problem is that even before #814, when we use regex-syntax to parse our regexes, we didn't correctly disable unicode on non-unicode builds, which resulted in unicode regexes ending up in the generated parser. Prior to #814 this seems to have not mattered, I think because internals of regex compile the unicode away anyways (based on this reply to a similar, but different issue. The issue there was a bug in regex-syntax, but this is a bug in our code, that happens to hit a similar symptom).

Anyways, I'm going to tidy up my fix and do end to end testing. If all goes well, I'll have a PR up in a bit.

dburgener · 2023-10-17T17:49:52Z

Oh, the other thing is that I'm still kind of mystified as to why our existing lalrpop tests didn't catch this at all. I'm going to prioritize getting the fix in to unblock the release, but then I'll try to loop back and identify the testing gap.

dburgener · 2023-10-17T20:01:58Z

Okay, the second issue was user error in the sense of "why on earth would you do that, don't do that", and lalrpop error in the sense of "we should handle this bad input more sanely".

I had a skip pattern that could match zero length regexes. #814 seems to have inadvertently dropped a check that returned "InvalidToken" if our longest match was a zero length skip pattern.

Note that the lalrpop book does recommend possibly zero length whitespace matches: https://lalrpop.github.io/lalrpop/lexer_tutorial/001_lexer_gen.html#customizing-skipping-between-tokens

I think the fix should be to restore the original check to return InvalidToken on a zero length skip match. I'll submit a PR for that as well.

Pat-Lafon · 2023-10-17T20:05:27Z

All this sounds good to me! Thanks for the detailed look.

dburgener · 2023-10-17T20:17:46Z

#845 is up

Everything seems good as far as I can tell. Once we get those merged, I'll retest.

youknowone · 2023-10-20T01:10:55Z

I'd like to add credits for new maintainers, who contributed a lot not only for this version but for longer time.

youknowone · 2023-10-20T19:30:30Z

is #844 the last piece of new release?

dburgener · 2023-10-20T20:25:24Z

is #844 the last piece of new release?

That plus this PR of course is all I'm tracking on. It looks like everyone is okay with #844, so I'll go ahead and merge that later today.

dburgener · 2023-10-20T21:09:05Z

#844 is merged.

I ran the same tests I did previous that uncovered the bugs against the latest master, and they passed this time. This seems ready to go from a functional perspective in my opinion.

youknowone

Released 0.20.1 👍

youknowone · 2023-10-23T01:50:42Z

https://crates.io/crates/lalrpop/0.20.1

Pat-Lafon added 3 commits October 16, 2023 17:42

Release 0.20.1 notes

ebf30ec

Add set -e -o to version.sh

e688a32

Update version numbers

6d89fc0

Pat-Lafon requested review from youknowone, dburgener and yannham October 16, 2023 21:50

dburgener reviewed Oct 16, 2023

View reviewed changes

RELEASES.md Outdated Show resolved Hide resolved

RELEASES.md Outdated Show resolved Hide resolved

Create bugfix section and msrv nit

1c2f106

yannham reviewed Oct 17, 2023

View reviewed changes

Pat-Lafon mentioned this pull request Oct 17, 2023

Include a pointer from the tutorial to a collection of examples #843

Merged

Add new maintainer note... which copies the 0.19.9 maintainer note

ae68242

youknowone approved these changes Oct 23, 2023

View reviewed changes

youknowone added this pull request to the merge queue Oct 23, 2023

Merged via the queue into lalrpop:master with commit 0c3599b Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 0.20.1 #842

Release 0.20.1 #842

Pat-Lafon commented Oct 16, 2023

dburgener left a comment

dburgener commented Oct 16, 2023

dburgener commented Oct 17, 2023

yannham left a comment •

edited

Loading

yannham Oct 17, 2023

Pat-Lafon Oct 17, 2023

Pat-Lafon Oct 17, 2023

yannham Oct 17, 2023

Pat-Lafon Oct 17, 2023

yannham Oct 17, 2023 •

edited

Loading

Pat-Lafon Oct 17, 2023

dburgener commented Oct 17, 2023

dburgener commented Oct 17, 2023

dburgener commented Oct 17, 2023

Pat-Lafon commented Oct 17, 2023

dburgener commented Oct 17, 2023

youknowone commented Oct 20, 2023

youknowone commented Oct 20, 2023

dburgener commented Oct 20, 2023

dburgener commented Oct 20, 2023

youknowone left a comment

youknowone commented Oct 23, 2023

Release 0.20.1 #842

Release 0.20.1 #842

Conversation

Pat-Lafon commented Oct 16, 2023

dburgener left a comment

Choose a reason for hiding this comment

dburgener commented Oct 16, 2023

dburgener commented Oct 17, 2023

yannham left a comment • edited Loading

Choose a reason for hiding this comment

yannham Oct 17, 2023

Choose a reason for hiding this comment

Pat-Lafon Oct 17, 2023

Choose a reason for hiding this comment

Pat-Lafon Oct 17, 2023

Choose a reason for hiding this comment

yannham Oct 17, 2023

Choose a reason for hiding this comment

Pat-Lafon Oct 17, 2023

Choose a reason for hiding this comment

yannham Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Pat-Lafon Oct 17, 2023

Choose a reason for hiding this comment

dburgener commented Oct 17, 2023

dburgener commented Oct 17, 2023

dburgener commented Oct 17, 2023

Pat-Lafon commented Oct 17, 2023

dburgener commented Oct 17, 2023

youknowone commented Oct 20, 2023

youknowone commented Oct 20, 2023

dburgener commented Oct 20, 2023

dburgener commented Oct 20, 2023

youknowone left a comment

Choose a reason for hiding this comment

youknowone commented Oct 23, 2023

yannham left a comment •

edited

Loading

yannham Oct 17, 2023 •

edited

Loading