Skip to content

Fix: Build failures on M1 Mac and C-only libraries, memory leak#18

Open
gharris1727 wants to merge 3 commits intoserenadeai:masterfrom
gharris1727:basic-fixes
Open

Fix: Build failures on M1 Mac and C-only libraries, memory leak#18
gharris1727 wants to merge 3 commits intoserenadeai:masterfrom
gharris1727:basic-fixes

Conversation

@gharris1727
Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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