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: Build failures on M1 Mac and C-only libraries, memory leak #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gharris1727
Copy link

This contains three separate fixes that are combined into one PR, let me know if you'd prefer them separated out.

  1. I am using an M1 mac, and the build.py was not able to correctly build the shared object for my architecture. I tweaked the gradle build to pass in the architecture, and the build.py to map aarch64 to arm64 to compensate. When the dylib was built, it became stale and would not get cleaned, and wouldn't get picked up by the tests, so I fixed those as well.
  2. I noticed that deleting tree cursors did not call the associated ts_tree_cursor_delete, so I added that call to the JNI shim.
  3. If you build with only c-based parser libraries, the cpp flag is set to false, despite the JNI shim itself making use of CPP new/delete calls. To fix this, I removed the option for c-only compilation, effectively setting cpp to always true.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
@sogaiu sogaiu mentioned this pull request Mar 22, 2023
@sogaiu
Copy link

sogaiu commented Mar 22, 2023

I'm not directly impacted by 1 (though it seems like it would be good to address), but I also noticed and was affected by 3 [1].

I didn't look closely at 2, so I don't have an informed opinion about it, but on the surface it seems reasonable to me.

@tmacwill Would you consider looking over this PR?


[1] In my case the build didn't fail, but I had an error during execution.

@brodmo
Copy link

brodmo commented Jun 22, 2023

I can confirm that building is currently broken on M1 and this PR makes it work, is it getting merged anytime soon?

@sogaiu
Copy link

sogaiu commented Jun 26, 2023

On a kind of related note, tree-sitter-python's scanner has been rewritten in C.

IIUC, if this or similar PR is not merged, newer versions of tree-sitter-python will not be usable from java-tree-sitter.

It was mentioned here that there is a fork that may have appropriate fixes applied though.

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.

4 participants