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

Get command path dynamically for restarting #262

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

Ape
Copy link
Contributor

@Ape Ape commented Jan 5, 2025

The previous version assumes certain installation location. This version works regardless of where the jar file was installed or if it's just a development build that wasn't really installed at all.

I didn't remove the previously used getInstallationPath() as it's also used to Windows-specific writeRegistryKey().

@sblantipodi
Copy link
Owner

sblantipodi commented Jan 5, 2025

Hi @Ape,
thanks again for the PR, much appreciated.

Using System.getProperty("java.home") and java -jar more in general
isn't good for all the possible use cases because Luciferin must run even without a JDK installed.

Current rpm, deb, Windows installer, snap and flatpak version runs without adding an additional JDK because all the packages contains all the needed java modules to run luciferin thanks to jpackage and jlink.

I like this approach for two reasons:

  • I can bundle a very stripped JDK with the app
  • I don't want to ask users to install Java to run Luciferin (Luciferin runs on the latest version of Java and very few users has that runtime installed)

With the current approach you can install Luciferin wherever you want but you need to put the .jar file in the lib/app/ folder, that's it.
If you have a better idea to do it we can obviously improve this.

As I said, with the current approach, you can install luciferin wherever you want, but you need to put the jar file in the lib/app folder and use the generated binary from jpackage to launch Luciferin.

You can have an idea of this with the
https://github.com/sblantipodi/build_tools/blob/a057b5ab2a49d59c48c28e3c0aec715285919276/flatpak/org.dpsoftware.FireflyLuciferin.json#L41
or
https://github.com/sblantipodi/build_tools/blob/a057b5ab2a49d59c48c28e3c0aec715285919276/snapcraft/snap/snapcraft.remote.yaml#L82
you can try to extract the .deb package with
ar x FireflyLuciferinLinux.deb
to have an idea on the folders structure I use-

Extracting a .deb package like I do for Flatpak or snap for example,
will save you to update the jdk when I update it, removes one deps that needs to be managed by the user and let you use a stripped JDK tailor-made for Luciferin.

...and it solves this problem :)

if you can't download the .deb file from the release in your pkgbuild, you could consider to jpackage the app using the command I use in my CI:

jpackage -i target --main-class org.dpsoftware.JavaFXStarter --main-jar FireflyLuciferin-jar-with-dependencies.jar --icon data/img/luciferin_logo.png --linux-shortcut --copyright "Davide Perini" --name FireflyLuciferin --vendor DPsoftware --app-version "${{steps.get-id.outputs.id}}" --java-options "-XX:+UseZGC -XX:+UseStringDeduplication -Xms64m -Xmx1024m --add-modules=jdk.incubator.vector --enable-native-access=org.dpsoftware --enable-native-access=ALL-UNNAMED"

this will create an executable binary with the stripped jdk from source.

please let me know if I answered you in a reasonable way...

@Ape
Copy link
Contributor Author

Ape commented Jan 5, 2025

I see, this doesn't work with the jpackage approach. However, I would like to be able to also run the app without using jpackage.

What do you think about a solution like this?

String jpackageApp = System.getProperty("jpackage.app-path");

if (jpackageApp != null) {
    commands.add(jpackageApp);
} else {
    commands.add(System.getProperty("java.home") + File.separator + "bin" + File.separator + "java");
    commands.addAll(ManagementFactory.getRuntimeMXBean().getInputArguments());
    commands.add("-jar");
    commands.add(System.getProperty("sun.java.command").split("\\s+")[0]);
}

I think something like this would also improve the jpackage solution since it would be path independent. Moreover, this way it's also possible to launch the app as an Java application with any custom JVM parameters and they would be retained on restart.

@sblantipodi
Copy link
Owner

I agree that this is something that must be improved,
it's a good idea but who is supposed to set the "jpackage.app-path" variable, when, where and how?

@Ape
Copy link
Contributor Author

Ape commented Jan 5, 2025

it's a good idea but who is supposed to set the "jpackage.app-path" variable, when, where and how?

Isn't that set automatically by jpackage at runtime?

@sblantipodi
Copy link
Owner

Isn't that set automatically by jpackage at runtime?

ooops you're right, that very good.
if you change the PR I'll merge it on the next release.

I know that this is a question of tastes, but can you move the strings in Constants.java file please?

The previous version assumes certain installation location. This version
works regardless of where the jar file was installed or if it's just a
development build that wasn't really installed at all.
@Ape Ape force-pushed the pr/crossplatform_restart branch from 2849327 to 4c699a9 Compare January 5, 2025 22:10
@Ape Ape changed the title Get cross-platform command path for restarting Get command path dynamically for restarting Jan 5, 2025
@Ape
Copy link
Contributor Author

Ape commented Jan 5, 2025

Any comments on this version?

I have tested that it works for the Java app case, but could you test it with jpackage and on Windows before merging.

I believe that this solution would also work on snap and flatpak, but I left those as they were.

@sblantipodi
Copy link
Owner

very cool... give me some time to test on windows, snap, flatpak, deb...

@sblantipodi
Copy link
Owner

weirdly it doesn't work on snap and flatpak but it works well for other use cases.
the returned path from System.getProperty("jpackage.app-path") on snap/flatpak
it's not an absolute one and it doesn't work.

no problem. merging it as it is, thanks.

@sblantipodi sblantipodi merged commit b96c17c into sblantipodi:master Jan 5, 2025
2 checks passed
@sblantipodi sblantipodi added the enhancement New feature or request label Jan 5, 2025
@Ape Ape deleted the pr/crossplatform_restart branch January 6, 2025 01:45
sblantipodi added a commit to sblantipodi/glow_worm_luciferin that referenced this pull request Jan 19, 2025
- ***Breaking changes***: requires `Firefly Luciferin` firmware (v2.20.5).   
- **Color accuracy has been significantly improved when using HDR contents.** Closes [#268](sblantipodi/firefly_luciferin#268).
- New [color orders](https://github.com/sblantipodi/firefly_luciferin/wiki/RGB-and-RGBW-support#how-to-change-color-order) to support newer LED strips.
- Get command path dynamically for restarting. Closes [#262](sblantipodi/firefly_luciferin#262). Thanks @Ape for the PR.
- Fix "Sources not selected" crash on Wayland. Closes [#264](sblantipodi/firefly_luciferin#264). Thanks @Ape for the PR.
- OnBoard-Led managment for esp8266. Closes [#266](sblantipodi/firefly_luciferin#266).
- When setting a low brightness color on Firefly Luciferin, it’s not possible to increase the brightness in the [web interface](https://github.com/sblantipodi/firefly_luciferin/wiki/Remote-Access#luciferin-web-interface). Fixed.
- Fix "Slow rainbow" effect switching to "Solid" automatically.
sblantipodi added a commit that referenced this pull request Jan 19, 2025
- ***Breaking changes***: requires `Glow Worm Luciferin` firmware (v5.19.4).   
- **Color accuracy has been significantly improved when using HDR contents.** Closes [#268](#268).
- New [color orders](https://github.com/sblantipodi/firefly_luciferin/wiki/RGB-and-RGBW-support#how-to-change-color-order) to support newer LED strips.
- Get command path dynamically for restarting. Closes [#262](#262). Thanks @Ape for the PR.
- Fix "Sources not selected" crash on Wayland. Closes [#264](#264). Thanks @Ape for the PR.
- OnBoard-Led managment for esp8266. Closes [#266](#266).
- When setting a low brightness color on Firefly Luciferin, it’s not possible to increase the brightness in the [web interface](https://github.com/sblantipodi/firefly_luciferin/wiki/Remote-Access#luciferin-web-interface). Fixed.
- Fix "Slow rainbow" effect switching to "Solid" automatically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants