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

Add Jansson dependency #36

Merged
merged 4 commits into from
Jan 18, 2021
Merged

Add Jansson dependency #36

merged 4 commits into from
Jan 18, 2021

Conversation

manuq
Copy link
Contributor

@manuq manuq commented Jan 15, 2021

Emacs 27.1 uses this library if available.

Fixes #35

Emacs 27.1 uses this library if available.

Fixes flathub#35
@flathubbot
Copy link

Started test build 36802

@flathubbot
Copy link

Build 36802 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/35430/org.gnu.emacs.flatpakref

@muep
Copy link
Collaborator

muep commented Jan 16, 2021

@manuq thank you for the PR.

Are there other effects from this, apart from JSON operations becoming faster? If the changes are limited to performance, which use cases in particular suffer from the elisp-based JSON support?

I am mainly asking to balance the benefit against the added work of keeping the bundled libjansson up to date and the risk that would result from neglecting that maintenance.

@muep muep mentioned this pull request Jan 16, 2021
@muep
Copy link
Collaborator

muep commented Jan 16, 2021

One thing that bothers me about this PR is that it does not seem to build correctly with flatpak-builder 1.0.5 as shipped in Debian. It does get though the automatic test, which makes me suspect that this is related to the exact version of flatpak-builder.

build.log

@hfiguiere
Copy link

It did build with flatpak-builder 1.0.5 on Debian Buster (Raspberry Pi, 64-bits). Not sure what happened on your system.

@muep
Copy link
Collaborator

muep commented Jan 17, 2021

Looks like my initial attempt at logging the build was missing its stderr. Here is a build log that actually has the error visible.
build.log

@muep
Copy link
Collaborator

muep commented Jan 17, 2021

So now after some more studying, it seems to me that flatpak in Debian stable does not do anything to activate content from /app/lib during the build. It does get used when the build results are run, though /etc/ld.so.conf.

On a Fedora 33 VM that I set up as a comparison, LD_LIBRARY_PATH is set up to refer the dynamic linker to /app/lib:

LD_LIBRARY_PATH=/app/lib:/usr/lib/x86_64-linux-gnu/GL/default/lib:/usr/lib/x86_64-linux-gnu/openh264/extra

This seems to depend more on the version of flatpak rather than flatpak-builder.

@muep
Copy link
Collaborator

muep commented Jan 17, 2021

The problems I saw seem to be a recently introduced problem in the Debian package. I filed it as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323

@manuq , now that I understand what this confusing problem resulted from, I would like to ask a small addition to this change. Namely, the files not needed from Jansson at runtime should be removed. There seems to be an example of what to specify at https://github.com/flathub/com.github.paolostivanin.OTPClient/blob/fe78de8dc086a6b80ac6f4080182584b93f8b880/com.github.paolostivanin.OTPClient.yaml#L31

@muep muep assigned muep and manuq Jan 17, 2021
@flathubbot
Copy link

Started test build 37080

@manuq
Copy link
Contributor Author

manuq commented Jan 18, 2021

@manuq , now that I understand what this confusing problem resulted from, I would like to ask a small addition to this change. Namely, the files not needed from Jansson at runtime should be removed. There seems to be an example of what to specify at https://github.com/flathub/com.github.paolostivanin.OTPClient/blob/fe78de8dc086a6b80ac6f4080182584b93f8b880/com.github.paolostivanin.OTPClient.yaml#L31

Ah thanks for the reference, I couldn't find any. PR updated.

Are there other effects from this, apart from JSON operations becoming faster? If the changes are limited to performance, which use cases in particular suffer from the elisp-based JSON support?

All I know is from the NEWS file and my personal experience. I sometimes have buffers with giant JSON data, and I can perceive a noticeable performance boost in JSON operations. Without this Emacs UI freezes in those buffers. Other than that, I don't see any other effects, and there's nothing mentioned in the NEWS file as well.

I am mainly asking to balance the benefit against the added work of keeping the bundled libjansson up to date and the risk that would result from neglecting that maintenance.

Yes I will understand and happily accept your decision, if you think this shouldn't be merged because of the costs of maintenance.

@flathubbot
Copy link

Build 37080 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/35700/org.gnu.emacs.flatpakref

@muep
Copy link
Collaborator

muep commented Jan 18, 2021

Hi @manuq - there is still still one file that I would like to be absent. The /app/lib/libjansson.a file should be unnecessary at runtime.

Otherwise, I think the change looks good.

@flathubbot
Copy link

Started test build 37083

@manuq
Copy link
Contributor Author

manuq commented Jan 18, 2021

Hi @manuq - there is still still one file that I would like to be absent. The /app/lib/libjansson.a file should be unnecessary at runtime.

Sounds good. Fixup added.

"/share",
"/lib/pkgconfig",
"/lib/*.la",
"/app/lib/libjansson.a"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably should be just /lib/libjansson.a - similar to how you remove the other bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, yes, sorry.

@flathubbot
Copy link

Build 37083 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/35703/org.gnu.emacs.flatpakref

@flathubbot
Copy link

Started test build 37085

@flathubbot
Copy link

Build 37085 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/35705/org.gnu.emacs.flatpakref

@muep muep merged commit d40d0ed into flathub:master Jan 18, 2021
@muep
Copy link
Collaborator

muep commented Jan 18, 2021

Thanks you @manuq for the PR and your patience 😄

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.

Please build with native JSON parsing support
4 participants