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

Build lalrpop without lalrpop binary #4107

Merged
merged 2 commits into from
Aug 21, 2022

Conversation

moreal
Copy link
Contributor

@moreal moreal commented Aug 19, 2022

When I started to work on RustPython project today, I met compile error about python.rs. I guess it occurred because the python.rs, generated lalrpop file, was removed and it became to generate the file every compile. And the error message guided me to install lalrpop by using cargo install lalrpop command.

This pull request makes the parser/build.rs uses lalrpop crates instead of lalrpop binary.

You can test with the below commands:

# Remove lalrpop if it was installed already
cargo uninstall lalrpop

# Remove generated python.rs
rm -rf ./target

cargo build

@moreal moreal changed the title Guide to install lalrpop through cargo Guide to install lalrpop with cargo Aug 19, 2022
@fanninpm fanninpm requested a review from youknowone August 19, 2022 14:25
@youknowone
Copy link
Member

maybe i'd better to find a better way to do it.

@youknowone youknowone added the z-ca-2022 Tag to track contrubution-academy 2022 label Aug 19, 2022
@moreal moreal force-pushed the docs/install-lalrpop branch 2 times, most recently from 0ce8f4b to 8688111 Compare August 19, 2022 14:56
@moreal moreal changed the title Guide to install lalrpop with cargo Build lalrpop without lalrpop binary Aug 19, 2022
@moreal
Copy link
Contributor Author

moreal commented Aug 19, 2022

maybe i'd better to find a better way to do it.

@youknowone With more research, I know it can run the lalrpop in Rust code directly, instead of binary. I made it always use lalrpop in Rust code directly. Is there a case to require using lalrpop binary, that I don't know?

@youknowone
Copy link
Member

No, actually I recently added it as an optional dependency. @coolreader18 wanted to avoid lalrpop dependency from CI side.

@moreal
Copy link
Contributor Author

moreal commented Aug 20, 2022

No, actually I recently added it as an optional dependency. @coolreader18 wanted to avoid lalrpop dependency from CI side.

Ok, I see. Then I think it'll be the best if lalrpop is used in default and the CI excludes the lalrpop with some args.

rust-lang/cargo#3126 seems able to implement my thought but it seems not resolved yet.

At now, I'll make this pull request as draft because I'm not sure what is the solution 🤔

Current my thought is:

@moreal moreal marked this pull request as draft August 20, 2022 05:25
@moreal moreal force-pushed the docs/install-lalrpop branch from 8688111 to 3c794b0 Compare August 20, 2022 07:09
@youknowone youknowone force-pushed the docs/install-lalrpop branch from 64fdc10 to ca256d3 Compare August 20, 2022 15:14
@moreal moreal marked this pull request as ready for review August 20, 2022 17:22
@moreal
Copy link
Contributor Author

moreal commented Aug 20, 2022

I opened again because @youknowone pushed ca256d3 commit and the CI seems passed well.

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

thank you for good idea

@youknowone youknowone merged commit e44ccb0 into RustPython:main Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
z-ca-2022 Tag to track contrubution-academy 2022
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants