Introdude `unicode` and fix various test cases when not all features are enabled. #786

ithinuel · 2023-04-28T19:08:14Z

This is a "frontport" of #783.

This sligtly deviates from the afore-mention PR to adapt to the changes on master and hopfully for the better.

doc/calculator/Cargo.toml

tools/ci.sh

doc/src/quick_start_guide.md

youknowone · 2023-04-28T21:14:21Z

doc/calculator/Cargo.toml

+regex-syntax = { version = "0.6", default-features = false, features = ["unicode-perl"] }
+
+[dev-dependencies]
+regex = { version = "1.8", default-features = false, features = ["unicode-perl"] }


Suggested change

regex = { version = "1.8", default-features = false, features = ["unicode-perl"] }

regex = "1.8"

Rather than making tutorial so heavy, how about just like this? The default feature is unicode, so it will be ok.

Since not having unicode enabled as a blanket feature (let alone default features) is the plan for the future version, wouldn't that be a good opportunity to document it?

Edit: I kept one of the two occurences in this file and added a little explanation comment, let me know what you think about it and whether it needs to be replicated somewhere else in the doc.

Cargo.toml

Because there is no crate defined at the root of the workspace, cargo defaults to resolver="1" (despite all crates being edition="2021").

- Fix cfg tests (they were not actually testing the intended feature) - Add missing use when building without std. - Remove unnecessary runtime check for lexer feature - Check the message is correct before validating the spans This makes debugging the tests easier as there is no point fighting with spans if the error isn't the expected one to start with.

Some tests have had a cfg guard added because they require the unicode feature to be enabled on regex (which is only true when unicode is enabled on lalrpop). lalrpop-test always enable lexer & unicode which may hide under-specified feature requirement on parts of the code.

ithinuel · 2023-04-29T17:21:52Z

I rewrote the history of that PR to remove obsolete changes and give a bit more context about each changeset.

As of now cargo tree -p lalrpop-test shows:

lalrpop-test v0.19.10 (/home/ithinuel/Documents/rust/lalrpop/lalrpop-test)
├── diff v0.1.13
└── lalrpop-util v0.19.10 (/home/ithinuel/Documents/rust/lalrpop/lalrpop-util)
    └── regex v1.8.1
        └── regex-syntax v0.7.1
[build-dependencies]
└── lalrpop v0.19.10 (/home/ithinuel/Documents/rust/lalrpop/lalrpop)
[…]
    ├── lalrpop-util v0.19.10 (/home/ithinuel/Documents/rust/lalrpop/lalrpop-util)
    │   └── regex v1.8.1 (*)
[…]
    ├── regex v1.8.1 (*)
    ├── regex-syntax v0.6.29
[…]

youknowone

Thank you so much. I have a concern about removing pico-args from default, but other stuffs are looking so great and a lot better organized then before.

One more question. Do we need unicode as a default feature only for lalrpop but not for lalrpop-util?

Cargo.toml

youknowone · 2023-04-29T19:08:31Z

lalrpop/Cargo.toml

@@ -33,26 +33,21 @@ term = { version = "0.7", default_features = false }
 tiny-keccak = { version = "2.0.2", features = ["sha3"] }
 unicode-xid = { version = "0.2", default_features = false }

-# This dependency is only needed for binary builds, if you use LALRPOP as
-# library, disable it in your project by setting default_features = false.


As I remember, this is related to cargo install. By removing this feature from default feature, cargo install lalrpop will fail and have to be cargo install lalrpop --features pico-args.
rust-lang/cargo#10409

I'd like to keep this feature as default unless cargo add a feature or we make a new crate lalrpop-cli

It will "succeed" (exit with 0) without building/installing the lalrpop binary and display:

warning: none of the package's binaries are available for install using the selected features bin "lalrpop" requires the features: `pico-args` Consider enabling some of the needed features by passing, e.g., `--features="pico-args"`

It seems to me that the binary isn't as often required as lalrpop itself.
Having this enabled by default make every one dependant on lalrpop with the default features (basically everyone having lalrpop in their build-deps I'd imagine) would also build pico-args for direct purpose.

One day maybe cargo will get rust-lang/rfcs#3374

lalrpop/src/normalize/token_check/test.rs

Cargo.toml

youknowone · 2023-04-29T19:20:03Z

@dburgener @Pat-Lafon @jannic would you review this PR?

dburgener · 2023-04-29T20:01:10Z

lalrpop/Cargo.toml

 [features]
-default=["lexer", "pico-args"]


Pretty sure this is a non-issue, since this PR is targeting 0.20, but just to highlight anyways, this is a semver breaking change, so it should definitely only go in a breaking update, and should probably get highlighted in a changelog.

I agree with the logic on removing the default feature, for the reasons @ithinuel expressed elsewhere in this PR, I just want to make sure that we highlight and communicate it properly on the off-chance that someone is affected. One group that this might hit is anyone doing cargo install lalrpop in an automated script somewhere. Presumably when they update from 0.19 to 0.20 they'll retest their script and update the command line, but it never hurts to be cautious and be able to say "look at the clear changelog" if anyone is impacted by the change to defaults.

Since semver regards 0.x minor version changes as breaking change, I don't worry about it.
The concern is the standard user group in rust community don't expect cargo install has complicated requirements with more arguments.
It might be acceptable because lalrpop is more like professional dev tool rather than end user application. I personally will avoid this kind of user experience for any end-user applications.

Yes, I think it's fine to put this in 0.20, since it's a semver change from 0.19. My only point is that it's something worth highlighting in the 0.20 release notes.

dburgener · 2023-04-29T20:21:37Z

This looks great to me. Seems like a lot of great improvements and a clean solution to the regex unicode issues.

I also tested out building a simple rust binary that includes only lalrpop as a dependency, and builds a simple parser that requires unicode support. I verified that I get build errors when disabling the unicode feature and build successfully when enabling it, so that all works as expected.

I didn't look closely at the Span related changes in commit 3, as I don't have any experience with that part of the lalrpop code to evaluate those, but I did read through everything else, and IMHO it seems ready for merge.

youknowone

I reverted only the pico-args part. Other parts are technical and based on confirmed decisions.
But pico-args part is breaking change with pros and cons. I will leave it to @nikomatsakis 's side.
Could you please open another PR for that?

jannic · 2023-04-30T09:21:47Z

lalrpop/Cargo.toml

-# generate LALRPOPs own parser instead of using the saved parser.
-test = []
-
+default=["lexer", "unicode", "pico-args"]


Is it intentional to enable those by default? From other comments I got the impression that they should be disabled in 0.20 even though it's a breaking change.

lalrpop is used during build (in build.rs) so it's not really an issue for it to have more features by default.

Ok. I'm currently reading the changes on my phone, so I might be missing the big picture, literally and figuratively.

jannic · 2023-04-30T09:27:37Z

tools/ci.sh

-cargo build -p lalrpop
-cargo test --all --all-features
+cargo build --bin lalrpop --features pico-args
+# build with minimal amount of features to make sure dependencies can build too.


This comment doesn't exactly match the command, as feature-powerset tests several combinations of features, not only the minimal amount of features.
Btw, does it even work as expected to first build multiple combinations and then test multiple combinations?

you cane see the various combinasion of features for build and tests over here: https://github.com/lalrpop/lalrpop/actions/runs/4843403960/jobs/8630997943#step:5:142

Yes, that's exactly what I expected. But it's not "the minimal amount of features" - it's basically every possible combination of features.
But of course that's only a minor nit pick.
And while I'm nit-picking: What additional value does the cargo hack build provide over the later cargo hack test? Doesn't cargo test implicitly build the package first?

Ah right, when it was introduced, there was extra [dev-dependencies] to enable specific regex-syntax features during tests. So build & test weren't the same.
They should be the same now indeed.
Oh and initially it was using --each-feature rather than --feature-powerset hence the "minimal amount".
But yeah, this comment is a bit off now.

ithinuel force-pushed the frontport-fix-testing branch 2 times, most recently from 9c1ad77 to 982ea11 Compare April 28, 2023 19:16

youknowone reviewed Apr 28, 2023

View reviewed changes

doc/calculator/Cargo.toml Show resolved Hide resolved

Pat-Lafon reviewed Apr 28, 2023

View reviewed changes

tools/ci.sh Outdated Show resolved Hide resolved

Pat-Lafon reviewed Apr 28, 2023

View reviewed changes

tools/ci.sh Outdated Show resolved Hide resolved

Pat-Lafon reviewed Apr 28, 2023

View reviewed changes

doc/src/quick_start_guide.md Outdated Show resolved Hide resolved

ithinuel force-pushed the frontport-fix-testing branch from 9e34a4f to 13a21d4 Compare April 28, 2023 20:46

ithinuel mentioned this pull request Apr 28, 2023

Enable 'unicode' feature on regex-syntax #787

Merged

youknowone reviewed Apr 28, 2023

View reviewed changes

dburgener reviewed Apr 29, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

ithinuel added 4 commits April 29, 2023 18:12

Enable resolver="2" on the workspace.

651daa0

Because there is no crate defined at the root of the workspace, cargo defaults to resolver="1" (despite all crates being edition="2021").

Use cargo-hack to build & tests all feature combinations

5b74b97

ithinuel force-pushed the frontport-fix-testing branch from a0336f5 to 3d52fe5 Compare April 29, 2023 17:21

youknowone mentioned this pull request Apr 29, 2023

Add a default feature 'unicode' #791

Closed

youknowone reviewed Apr 29, 2023

View reviewed changes

ithinuel changed the title ~~Frontport #783~~ Introdude unicode and fix various test cases when not all features are enabled. Apr 29, 2023

youknowone linked an issue Apr 29, 2023 that may be closed by this pull request

Adding a feature unicode #789

Closed

dburgener reviewed Apr 29, 2023

View reviewed changes

dburgener approved these changes Apr 29, 2023

View reviewed changes

Revert pico-args changes

8900e79

youknowone approved these changes Apr 30, 2023

View reviewed changes

youknowone added this pull request to the merge queue Apr 30, 2023

Merged via the queue into lalrpop:master with commit 3de4686 Apr 30, 2023

ithinuel deleted the frontport-fix-testing branch April 30, 2023 08:57

jannic reviewed Apr 30, 2023

View reviewed changes

analog-hors mentioned this pull request Jul 15, 2023

Is it supposed to be possible to use the lexer generator without enabling the unicode feature? #810

Closed

Pat-Lafon mentioned this pull request May 8, 2024

Whoops, I don't think a test feature flag exists #895

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introdude `unicode` and fix various test cases when not all features are enabled. #786

Introdude `unicode` and fix various test cases when not all features are enabled. #786

ithinuel commented Apr 28, 2023

youknowone Apr 28, 2023

ithinuel Apr 28, 2023 •

edited

Loading

ithinuel commented Apr 29, 2023 •

edited

Loading

youknowone left a comment

youknowone Apr 29, 2023

ithinuel Apr 29, 2023 •

edited

Loading

Pat-Lafon Apr 29, 2023

youknowone commented Apr 29, 2023

dburgener Apr 29, 2023

youknowone Apr 29, 2023

dburgener Apr 29, 2023

dburgener commented Apr 29, 2023

youknowone left a comment

jannic Apr 30, 2023

ithinuel Apr 30, 2023 •

edited

Loading

jannic Apr 30, 2023

jannic Apr 30, 2023

ithinuel Apr 30, 2023

jannic Apr 30, 2023

ithinuel Apr 30, 2023

	regex = { version = "1.8", default-features = false, features = ["unicode-perl"] }
	regex = "1.8"

Introdude unicode and fix various test cases when not all features are enabled. #786

Introdude unicode and fix various test cases when not all features are enabled. #786

Conversation

ithinuel commented Apr 28, 2023

Choose a reason for hiding this comment

ithinuel Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

ithinuel commented Apr 29, 2023 • edited Loading

youknowone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ithinuel Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone commented Apr 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dburgener commented Apr 29, 2023

youknowone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ithinuel Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introdude `unicode` and fix various test cases when not all features are enabled. #786

Introdude `unicode` and fix various test cases when not all features are enabled. #786

ithinuel Apr 28, 2023 •

edited

Loading

ithinuel commented Apr 29, 2023 •

edited

Loading

ithinuel Apr 29, 2023 •

edited

Loading

ithinuel Apr 30, 2023 •

edited

Loading