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

Make pest no_std compatible. #498

Merged
merged 5 commits into from
May 2, 2021
Merged

Make pest no_std compatible. #498

merged 5 commits into from
May 2, 2021

Conversation

01mf02
Copy link
Contributor

@01mf02 01mf02 commented Mar 13, 2021

This is another attempt to fix #240 and make pest no_std compatible, allowing it to be used for example in web applications via WASM. I started slowly by converting the core pest crate only.
All tests pass.

There is however one breaking change: In my new version, pest's Error type does not implement std::error::Error, because this trait is part of std. If implementing this trait is important, it could be implemented behind a feature flag.

@CAD97
Copy link
Contributor

CAD97 commented Mar 13, 2021

In order to avoid breaking changes, please do put the std error impl behind a (default enabled) std feature flag.

Technically doing so is still breaking for anyone using no-default-features, but it's a breaking change that I'm much more willing to treat as "soft;" the outright removal of the impl is unquestionably objectively "hard" breaking.

Since we currently do not have any default features, there's no reason for someone to be using no-default-features, so I lean on the side of this being "soft breaking" (i.e. doesn't need a semver bump) at the moment, but I often change sides here. I'll defer to @dragostis here.

(Basic argument for "hard breaking:" the idea of "soft breaking" is that the fully elaborated form wouldn't break, and works both before and after the change. For features, the fully elaborated form would be to use no-default-features and enable every feature automatically.)

(As that's the case, I'm basically for every crate that thinks they might support no_std in the future to add a default-on std feature that breaks compilation when off. And maybe just do the work to use non-std stuff from core/alloc to begin with.)

This will also allow you to reënable the std-requiring tests.

@01mf02
Copy link
Contributor Author

01mf02 commented Mar 15, 2021

Thanks for your suggestion, @CAD97.
I thus added a std feature that is enabled by default.
pest's Error type implements std::error::Error when the std feature is enabled.
Therefore, from a user's point of view, this version of pest should now be indistinguishable from the current master.
Furthermore, I re-enabled the hash test that I previously disabled.

To test no_std, it suffices to run cargo test --no-default-features.

@dragostis
Copy link
Contributor

Soft breaking with a 2.x release sounds good. Thank you so much for enabling this, @01mf02.

pest/src/lib.rs Outdated Show resolved Hide resolved
@01mf02
Copy link
Contributor Author

01mf02 commented Mar 19, 2021

Just to let you know: I have now also ported derive and generator to no_std. Please let me know how you would like me to proceed. Should I push my new changes to this branch? Or should I wait until the current pull request is merged, then open another one?

@01mf02
Copy link
Contributor Author

01mf02 commented Apr 2, 2021

@CAD97 & @dragostis, what is the status on this? Is there anything I can do to help my pull request getting merged?

@CAD97
Copy link
Contributor

CAD97 commented Apr 2, 2021

Ah sorry, I meant to r+ this earlier. I don't think derive / generator need to be no_std, since they only run on the host machine? (With the new cargo feature resolver that properly isolates features, anyway.)

If you have a use case for it, feel free to open a new PR with an explanation of what it unlocks.

bors: r=@CAD97, @dragostis

bors bot added a commit that referenced this pull request Apr 2, 2021
498: Make pest no_std compatible. r=CAD97,dragostis a=01mf02

This is another attempt to fix #240 and make pest `no_std` compatible, allowing it to be used for example in web applications via WASM. I started slowly by converting the core `pest` crate only.
All tests pass.

There is however one breaking change: In my new version, pest's `Error` type does not implement `std::error::Error`, because this trait is part of `std`. If implementing this trait is important, it could be implemented behind a feature flag.

Co-authored-by: Michael Färber <01mf02@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 2, 2021

Build failed:

@CAD97
Copy link
Contributor

CAD97 commented Apr 2, 2021

Oh right, new clippy warnings. The quickest way to get this merged is a new PR to fix those @01mf02 , otherwise I might get around to doing those eventually.

(The clippy warning should just be an #[allow], the non-fmt-panic should be fixed to be a fmt panic.)

@01mf02
Copy link
Contributor Author

01mf02 commented Apr 3, 2021

I just checked these clippy warnings.
They can be in principle be resolved by replacing every instance of

#[allow(dead_code, non_camel_case_types)]

in generator/src/generator.rs by

#[allow(dead_code, non_camel_case_types, upper_case_acronyms)]

However, I think that this alone will not solve the clippy warning, because the bootstrap package uses an version of pest_generator from crates.io. Therefore, I think the workflow should be something like:

  1. Change generator.rs as outlined above
  2. Bump pest_generator to a new version, i.e., 2.1.4, and publish on crates.io
  3. Update bootstrap/Cargo.toml to use the new pest_generator version (this step might not be necessary)

I think that someone with more privileges and experience than me should check these steps. :)

@01mf02
Copy link
Contributor Author

01mf02 commented Apr 3, 2021

Regarding the other clippy warning: The change required would be:

diff --git a/meta/src/lib.rs b/meta/src/lib.rs
index 1f9c5bc..f049e0b 100644
--- a/meta/src/lib.rs
+++ b/meta/src/lib.rs
@@ -30,11 +30,11 @@ where
 {
     result.unwrap_or_else(|e| {
         panic!(
-            "grammar error\n\n".to_owned()
-                + &e.into_iter()
-                    .map(|error| format!("{}", error))
-                    .collect::<Vec<_>>()
-                    .join("\n\n")
+            "grammar error\n\n{}",
+            e.into_iter()
+                .map(|error| error.to_string())
+                .collect::<Vec<_>>()
+                .join("\n\n")
         )
     })
 }

Sorry, I'm reluctant to open a new PR because of the current state of my working tree.
I still hope that I made it convenient for you to do the required changes. :)

@CAD97
Copy link
Contributor

CAD97 commented Apr 12, 2021

Let's try this again now

bors: r+

@CAD97
Copy link
Contributor

CAD97 commented Apr 12, 2021

Looks like bors crashed.

bors: retry

@CAD97
Copy link
Contributor

CAD97 commented Apr 12, 2021

bors crashed twice with the same error, I've reported it upstream.

@notriddle
Copy link

I'm going to run this again. It won't work, but it will get me a request ID that I can send to github support.

bors r+

@notriddle
Copy link

Opened support ticket #1104028

@MarinPostma
Copy link
Contributor

@CAD97 @notriddle, do we have any update on this? Is this an issue with Bors or Github? The link to the issue provided by @CAD97 is broken for me.

@notriddle
Copy link

Nothing. 12 days ago, they said "it should be working", and haven't added anything new to the issue since.

@MarinPostma
Copy link
Contributor

I wonder what's the issue. We use bors on a daily basis at Meilisearch, with no issue, there must be a workaround.

testing if bors test works:

bors test

@notriddle
Copy link

You want bors try, right?

@CAD97
Copy link
Contributor

CAD97 commented Apr 28, 2021

bors: ping

bors: try

@bors
Copy link
Contributor

bors bot commented Apr 28, 2021

pong

@CAD97
Copy link
Contributor

CAD97 commented Apr 28, 2021

Immediate crash again. Immediate 403 after asking bors to try. The only information I have on what failed is {\"message\":\"Resource not accessible by integration\",\"documentation_url\":\"https://docs.github.com/rest/reference/repos#merge-a-branch\"}; I wish I even knew what resource fetch was failing, to allow me to even begin to try to figure out why it's failing.

@notriddle
Copy link

It's the actual merge operation that's failing. I manually reproduced it with curl -v -L -H "Authorization: token ghs_[censored]" -X POST --data-raw '{"head":"ebc8e85699f55efea0fd57fddf5758640b142088","commit_message":"[ci skip][skip ci][skip netlify] -bors-staging-tmp-498","base":"staging.tmp"}' https://api.github.com/repositories/56964647/merges while I was chatting with GitHub Support.

@CAD97
Copy link
Contributor

CAD97 commented Apr 30, 2021

#468 merged fine so it must be something about this PR...

I know there's some weird permission things about inline notes from actions, and as such I was surprised to see them pop up in this PR (but maybe my remembrance is outdated). I don't see any other possible reason merging this PR specifically could fail with a permissions issue.
Because I'm paranoid and #468 just merged, one more try at bors then I'll probably just give up and manually push the green button.

bors: r+

(Yep it crashed with the same error)

@CAD97
Copy link
Contributor

CAD97 commented May 2, 2021

I've merged master into this PR to test a theory on why bors is failing: permissions requiring the branch to be up to date?

bors: r+

@bors
Copy link
Contributor

bors bot commented May 2, 2021

@bors bors bot merged commit f0a85d1 into pest-parser:master May 2, 2021
@CAD97
Copy link
Contributor

CAD97 commented May 2, 2021

Confirmed that this was the issue.

@notriddle: it'd be nice if bors-ng could catch this error and report a nice "failed to merge; check your permissions" message. Unfortunately I don't have access to this repo's protection settings currently, so I don't know which one broke bors, but I have a suspicion that it's "Require branches to be up to date before merging" as previously suggested.

@dragostis: we definitely need to fix whatever permissions setting is causing bors to not work. Either you need to fix it or give me the permissions to do so.

bors bot added a commit that referenced this pull request Oct 10, 2021
513: Make pest_generator / pest_derive no_std compatible. r=CAD97 a=01mf02

This is the follow-up pull request to #498.
It adds "std" feature flags for both pest_generator and pest_derive.
Currently, the correct function of this can only be tested by running `cargo test --no-default-features` *in the `derive` directory*.
Running `cargo test --no-default-features` in the root directory unfortunately does not run the tests in no_std mode.

Co-authored-by: Michael Färber <michael.faerber@gedenkt.at>
Co-authored-by: Michael Färber <01mf02@gmail.com>
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.

No std?
5 participants