https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Bug ID: 2193261 Summary: Review Request: tree-sitter-java - Java grammar for Tree-sitter Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: mavit@mavit.org.uk QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://mavit.fedorapeople.org/rpm/tree-sitter-java.spec SRPM URL: https://mavit.fedorapeople.org/rpm/tree-sitter-java-0.20.1-1.fc37.src.rpm Description: Add support for Java to Tree-sitter, an incremental parsing system for programming tools. Fedora Account System Username: mavit
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Marc-Andre Lureau marcandre.lureau@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value CC| |marcandre.lureau@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Marc-Andre Lureau marcandre.lureau@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |marcandre.lureau@redhat.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Marc-Andre Lureau marcandre.lureau@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(mavit@mavit.org.u | |k)
--- Comment #1 from Marc-Andre Lureau marcandre.lureau@redhat.com --- I like the short generic spec! Given that we are likely going to see dozens of those, we should set a good example.
Things to look after according to fedora-review: libtree-sitter-java.x86_64: E: shared-library-without-dependency-information /usr/lib64/libtree-sitter-java.so.0.0 libtree-sitter-java.x86_64: E: explicit-lib-dependency libtree-sitter libtree-sitter-java-devel.x86_64: E: double-slash-in-pkgconfig-path /usr/lib64/pkgconfig/tree-sitter-java.pc URL: https///github.com/tree-sitter/tree-sitter-java
could you take a look?
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Peter Oliver mavit@mavit.org.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(mavit@mavit.org.u | |k) |
--- Comment #2 from Peter Oliver mavit@mavit.org.uk --- (In reply to Marc-Andre Lureau from comment #1)
I like the short generic spec! Given that we are likely going to see dozens of those, we should set a good example.
If this works out okay, I intend to turn it into a template: https://pagure.io/fork/mavit/rpmdevtools/diff/main..tree-sitter
Things to look after according to fedora-review: libtree-sitter-java.x86_64: E: shared-library-without-dependency-information /usr/lib64/libtree-sitter-java.so.0.0
I don't know enough about C development to answer this, but I think this is somehow expected because this isn't a stand-alone library, but rather something you have to load with libtree-sitter. Perhaps we shouldn't be installing it directly into %{_libdir}?
libtree-sitter-java.x86_64: E: explicit-lib-dependency libtree-sitter
I think we need the explicit dependency here, because of the above: libtree-sitter-java.so doesn't have a dependency on libtree-sitter.so even though it requires it.
libtree-sitter-java-devel.x86_64: E: double-slash-in-pkgconfig-path /usr/lib64/pkgconfig/tree-sitter-java.pc URL: https///github.com/tree-sitter/tree-sitter-java
This is an bug in the upstream Makefile, which assumes that all Git URLs look like git@github.com:tree-sitter/tree-sitter-java.git rather than https://github.com/tree-sitter/tree-sitter-java.git:
ifeq (, $(PARSER_URL)) PARSER_URL := $(subst :,/,$(PARSER_REPO_URL)) PARSER_URL := $(subst git@,https://,$(PARSER_URL)) PARSER_URL := $(subst .git,,$(PARSER_URL)) endif
I'll prepare a patch.
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
--- Comment #3 from Marc-Andre Lureau marcandre.lureau@redhat.com --- (In reply to Peter Oliver from comment #2)
(In reply to Marc-Andre Lureau from comment #1)
I like the short generic spec! Given that we are likely going to see dozens of those, we should set a good example.
If this works out okay, I intend to turn it into a template: https://pagure.io/fork/mavit/rpmdevtools/diff/main..tree-sitter
looks ok (I wish they would use meson instead, but a bit late now..)
Things to look after according to fedora-review: libtree-sitter-java.x86_64: E: shared-library-without-dependency-information /usr/lib64/libtree-sitter-java.so.0.0
I don't know enough about C development to answer this, but I think this is somehow expected because this isn't a stand-alone library, but rather something you have to load with libtree-sitter. Perhaps we shouldn't be installing it directly into %{_libdir}?
yeah, it looks like tree-sitter is not explicit about where the libraries should be located, and users of the libraries can decide that. This could probably be changed... emacs seems to look at the same location as libtree-sitter... it should rather be $libdir/tree-sitter/ instead imho.
libtree-sitter-java.x86_64: E: explicit-lib-dependency libtree-sitter
I think we need the explicit dependency here, because of the above: libtree-sitter-java.so doesn't have a dependency on libtree-sitter.so even though it requires it.
Well, it doesn't link to it, then no. However there are is specific tree-sitter ABI (TSLanguage etc).
libtree-sitter-java-devel.x86_64: E: double-slash-in-pkgconfig-path /usr/lib64/pkgconfig/tree-sitter-java.pc URL: https///github.com/tree-sitter/tree-sitter-java
This is an bug in the upstream Makefile, which assumes that all Git URLs look like git@github.com:tree-sitter/tree-sitter-java.git rather than https://github.com/tree-sitter/tree-sitter-java.git:
ifeq (, $(PARSER_URL)) PARSER_URL := $(subst :,/,$(PARSER_REPO_URL)) PARSER_URL := $(subst git@,https://,$(PARSER_URL)) PARSER_URL := $(subst .git,,$(PARSER_URL)) endif
I'll prepare a patch.
thanks
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
--- Comment #4 from Peter Oliver mavit@mavit.org.uk --- SRPM URL: https://mavit.fedorapeople.org/rpm/tree-sitter-java-0.20.1-2.fc37.src.rpm
Fixes URL in tree-sitter-java.pc.
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
--- Comment #5 from Peter Oliver mavit@mavit.org.uk --- (In reply to Marc-Andre Lureau from comment #3)
(In reply to Peter Oliver from comment #2)
(In reply to Marc-Andre Lureau from comment #1)
I like the short generic spec! Given that we are likely going to see dozens of those, we should set a good example.
If this works out okay, I intend to turn it into a template: https://pagure.io/fork/mavit/rpmdevtools/diff/main..tree-sitter
looks ok (I wish they would use meson instead, but a bit late now..)
Perhaps it’s not too late. The current approach of copying Makefiles into each grammar’s repository isn’t working well. https://github.com/tree-sitter/tree-sitter/issues/1488
Things to look after according to fedora-review: libtree-sitter-java.x86_64: E: shared-library-without-dependency-information /usr/lib64/libtree-sitter-java.so.0.0
I don't know enough about C development to answer this, but I think this is somehow expected because this isn't a stand-alone library, but rather something you have to load with libtree-sitter. Perhaps we shouldn't be installing it directly into %{_libdir}?
yeah, it looks like tree-sitter is not explicit about where the libraries should be located, and users of the libraries can decide that. This could probably be changed... emacs seems to look at the same location as libtree-sitter... it should rather be $libdir/tree-sitter/ instead imho.
Do we have a sense of how serious this is? Could we successfully explain to upstream the benefit of moving it?
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
--- Comment #6 from Peter Oliver mavit@mavit.org.uk --- SRPM URL: https://mavit.fedorapeople.org/rpm/tree-sitter-java-0.20.1-3.fc40.src.rpm Eliminate rpmlint explicit-lib-dependency error.
I did some investigating, at it turns out that the shared-library-without-dependency-information issue doesn't occur upstream. It happens because we put '-Wl,--as-needed' in LDFLAGS.
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
--- Comment #7 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 1988187 --> https://bugzilla.redhat.com/attachment.cgi?id=1988187&action=edit The .spec file difference from Copr build 5874390 to 6394970
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Marc-Andre Lureau marcandre.lureau@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #8 from Marc-Andre Lureau marcandre.lureau@redhat.com --- lgtm
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
--- Comment #9 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/tree-sitter-java
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Peter Oliver mavit@mavit.org.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |RAWHIDE Last Closed| |2023-11-19 19:53:50
--- Comment #10 from Peter Oliver mavit@mavit.org.uk --- Thanks for the review!
https://bugzilla.redhat.com/show_bug.cgi?id=2193261
Peter Oliver mavit@mavit.org.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2258924
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2258924 [Bug 2258924] Tree-sitter language libraries are not available in Fedora
package-review@lists.fedoraproject.org