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

fix: Search for grammar file from CARGO_MANIFEST_DIR #702

Merged
merged 1 commit into from Aug 23, 2022

Conversation

Nukesor
Copy link
Contributor

@Nukesor Nukesor commented Aug 23, 2022

This is a proposeal to fix #325.

The new logic looks for the [grammar = "$path"] path in CARGO_MANIFEST_DIR/ before using the fallback of CARGO_MANIFEST_DIR/src.

This allows projects that don't have a src folder to use a grammar file, while staying backward compatible.

Alternative implementation approach:

One could theoretically use the absolute function.
Sadly, this function is still nightly, which makes this approach unfeasible.

Current workarounds:

  • Restructure the project so it has a src folder
  • Create a stub src folder, so one can use relative paths
  • Inline all of the grammar via grammar_inline

@Nukesor Nukesor requested a review from a team as a code owner August 23, 2022 12:58
@Nukesor Nukesor requested review from tomtau and removed request for a team August 23, 2022 12:58
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

thanks! I think that works -- it seems both options are covered by tests, so it should be fine. CC @MarijnS95 this fix should be backwards-compatible, but you can have a quick look.

//
// If we cannot find the expected file over there, fallback to the
// `CARGO_MANIFEST_DIR/src`, which is the old default and kept for convenience
// reasons.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe can add a TODO with a link to https://doc.rust-lang.org/std/path/fn.absolute.html ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@tomtau tomtau merged commit 4fde90e into pest-parser:master Aug 23, 2022
@Nukesor
Copy link
Contributor Author

Nukesor commented Sep 6, 2022

Hey @tomtau :)

I kind of need this fix for a feature I currently work on. It's a bit tricky to work with this, as pest-meta needs some special handling when building the project with a patched source.
It would be great to get a patch release that contains this fix. Do you have a rough timeline when the next release is planned?

Thanks in advance!

@tomtau
Copy link
Contributor

tomtau commented Sep 6, 2022

@Nukesor I think it's all right to make a quick point release after this is merged (plus perhaps after one small issue fix): #705 (to test the fixed CI release flow).

@Nukesor
Copy link
Contributor Author

Nukesor commented Sep 6, 2022

Awesome, thanks for your swift response and for taking care of this project :)

@tomtau tomtau mentioned this pull request Sep 10, 2022
@MarijnS95
Copy link

CC @MarijnS95 this fix should be backwards-compatible, but you can have a quick look.

@tomtau Fwiw I haven't looked into pest in detail but only got blocked on the previous release accidentally breaking transitive dependencies while updating them. Any contributor that intends to make such changes as part of a patch release should make sure to (build-)test those broken crates to make sure this change can safely be made without a breaking release.

@tomtau
Copy link
Contributor

tomtau commented Sep 12, 2022

@Nukesor it's out now: https://crates.io/crates/pest/2.3.1

@MarijnS95 I included this flow using cargo-semver-checks https://github.com/pest-parser/pest/blob/master/semvercheck.sh to check it didn't introduce (API-level) breaking changes, but it wouldn't probably catch that v2.2.0 breaking change (it'd catch the one in v2.2.1 though).

@MarijnS95
Copy link

@tomtau Indeed, those checks are nice to have but wouldn't have caught the build failure in external crates. That said, the recent update doesn't prevent any of my crates from building so we're all good here 🎉

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.

Grammar file not found on workspace
3 participants