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.

Sign up for GitHub

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

Jump to bottom

rizin-shell-parser: fix build with newest tree-sitter (again) #4325

Closed
wants to merge 1 commit into from
Closed

rizin-shell-parser: fix build with newest tree-sitter (again) #4325

wants to merge 1 commit into from

Conversation

ajakk
Copy link
Contributor

@ajakk ajakk commented Mar 4, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

    PR #4365 originally fixed this build failure with newer tree-sitter
    versions, but the relevant change for this was partially inadvertently
    reverted in #4165.

See #4365 for full rationale.
...

Test plan

Build passes with newest tree-sitter.
...

Closing issues

...

@ajakk
rizin-shell-parser: fix build with newest tree-sitter (again)
9dd696a 
PR rizinorg#4365 originally fixed this build failure with newer tree-sitter
versions, but the relevant change for this was partially inadvertently
reverted in rizinorg#4165.

Signed-off-by: John Helmert III <[email protected]>
@ajakk ajakk requested a review from ret2libc as a code owner March 4, 2024 04:49
@ajakk
Copy link
Contributor Author

ajakk commented Mar 4, 2024

I'd try upgrading tree-sitter but that's a bit beyond my capabilities here, sorry!

XVilka
XVilka approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

cc @kazarmy

@kazarmy kazarmy self-requested a review March 4, 2024 12:29
kazarmy
kazarmy approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@kazarmy kazarmy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

#4365?

Oh it's a generated file ... perhaps best if it was patched automatically post-hoc

ret2libc
ret2libc requested changes Mar 4, 2024
View reviewed changes
Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

Marking this red. The file is auto-generated.

@kazarmy kazarmy mentioned this pull request Mar 5, 2024
Merged
5 tasks
@XVilka
Copy link
Member

XVilka commented Mar 5, 2024

@ajakk @arrowd, we published a new 0.7.2 release, which should also fix this problem in the release. As for dev, we will just regenerate parser using newer tree-sitter version.

https://github.com/rizinorg/rizin/releases/tag/v0.7.2

@kazarmy kazarmy mentioned this pull request Mar 6, 2024
Merged
5 tasks
@kazarmy
Copy link
Member

kazarmy commented Mar 6, 2024

Superseded by #4338.

@kazarmy kazarmy closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ret2libc ret2libc ret2libc requested changes

@kazarmy kazarmy kazarmy approved these changes

@XVilka XVilka XVilka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ajakk @XVilka @kazarmy @ret2libc