Modify

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22088 closed defect (fixed)

[RFC PATCH] Debian start script does not properly check if a path does not exist or is not executable

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 22.06
Component: Core Version:
Keywords: linux Cc:

Description (last modified by taylor.smock)

I've been working on getting downstreams to use our start script (with some minor patches) to hopefully avoid future issues where we get bug reports that we've fixed in the start script.

Steps to reproduce (on Debian/Ubuntu):

  1. If /usr/lib/jvm/default-java/bin/java exists, move it (example: mv /usr/lib/jvm/default-java/bin/java /usr/lib/jvm/default-java/bin/java.bak)
  2. Attempt to run the start script
  3. If you moved /usr/lib/jvm/default-java/bin/java in step 1, move it back (example: mv /usr/lib/jvm/default-java/bin/java.bak /usr/lib/jvm/default-java/bin/java)

You may also be able to reproduce by unsetting JAVA_HOME or setting it to an empty string (e.g., JOSM_HOME="" ./josm)

From https://github.com/flathub/org.openstreetmap.josm/issues/55,

➜  ~ flatpak install flathub org.openstreetmap.josm                                                       
Looking for matches…

org.openstreetmap.josm permissions:
    ipc   network   x11   file access [1]

    [1] xdg-download


        ID                              Branch          Op          Remote          Download
 1. [✓] org.openstreetmap.josm          stable          i           flathub         78.5 MB / 76.9 MB

Installation complete.
➜  ~ flatpak run org.openstreetmap.josm
which: no dpkg in (/usr/bin:/app/bin:/app/jre/bin)
Using /usr/lib/jvm/default-java/bin/java to execute josm.
/app/bin/josm: line 91: /usr/lib/jvm/default-java/bin/java: No such file or directory

This is due to source:trunk/native/linux/tested/usr/bin/josm@18297:33,41-57#L33

Attachments (3)

22088.patch (8.6 KB ) - added by taylor.smock 2 years ago.
22088.2.patch (10.6 KB ) - added by taylor.smock 2 years ago.
Use java -version to determine JAVA_OPTS. This fixes an issue where the JAVACMD path did not include java-\d+, and should work with minimal modification for Java 20+ (presuming we do not have to add additional default JAVA_OPTS in the future). java -version is used since it works with Java 8.
22088.3.patch (12.7 KB ) - added by taylor.smock 2 years ago.
More commonality between the start scripts, allow overriding of some paths with environment variables

Download all attachments as: .zip

Change History (22)

by taylor.smock, 2 years ago

Attachment: 22088.patch added

comment:1 by taylor.smock, 2 years ago

Description: modified (diff)
Milestone: 22.06
Summary: Debian start script does not properly check if a path does not exist or is not executable[RFC PATCH] Debian start script does not properly check if a path does not exist or is not executable

attachment:22088.patch does the following:

  • Ensure that the java command will be executable (L43)
  • Makes the script more resistant to missing links or executables (dpkg and /etc/alternatives/java specifically)

This will hopefully allow us to encourage downstream distributors to use the start script, with minor modifications (hopefully just L71).

by taylor.smock, 2 years ago

Attachment: 22088.2.patch added

Use java -version to determine JAVA_OPTS. This fixes an issue where the JAVACMD path did not include java-\d+, and should work with minimal modification for Java 20+ (presuming we do not have to add additional default JAVA_OPTS in the future). java -version is used since it works with Java 8.

comment:2 by skyper, 2 years ago

Speaking of Debian plus its derivatives, I doubt that it is possible to install a *.deb without dpkg.

But well, the file is under /linux and not Debian or Ubuntu.

in reply to:  2 ; comment:3 by taylor.smock, 2 years ago

Replying to skyper:

Speaking of Debian plus its derivatives, I doubt that it is possible to install a *.deb without dpkg.

dpkg install -i *.deb && rm -f $(which dpkg)...
OK. That is a bit contrived.

But well, the file is under /linux and not Debian or Ubuntu.

I just want downstreams to use our start script, minimally patched for their purposes. That way we can hopefully avoid all the bug reports where we tell users "change your Java options" when they are using a packaged version (arch/flatpak).

in reply to:  3 ; comment:4 by skyper, 2 years ago

Replying to taylor.smock:

Replying to skyper:

Speaking of Debian plus its derivatives, I doubt that it is possible to install a *.deb without dpkg.

dpkg install -i *.deb && rm -f $(which dpkg)...
OK. That is a bit contrived.

and you should know how to fix you system.

But well, the file is under /linux and not Debian or Ubuntu.

I just want downstreams to use our start script, minimally patched for their purposes. That way we can hopefully avoid all the bug reports where we tell users "change your Java options" when they are using a packaged version (arch/flatpak).

trunk/native/linux/tested/usr/bin/josm?rev=18297&marks=64#L64

JAVA_OPTS="--module-path /usr/share/openjfx/lib --add-modules java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web

is another honey pot. I still wonder why JOSM depends on libopenjfx-java. In my eyes it should be downgraded to suggests or at least recommends.

in reply to:  4 comment:5 by taylor.smock, 2 years ago

Replying to skyper:

trunk/native/linux/tested/usr/bin/josm@18297:64#L64

JAVA_OPTS="--module-path /usr/share/openjfx/lib --add-modules java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web

is another honey pot. I still wonder why JOSM depends on libopenjfx-java. In my eyes it should be downgraded to suggests or at least recommends.

At one point, two somewhat popular plugins depended upon JavaFX: Mapillary and Microsoft Streetside. Mapillary used it for a date picker (no longer, I got tired of dealing with the bug reports) and Microsoft Streetside has been broken at various points over the years (I should probably look at porting it to the new image viewer -- they use a "cube map" for 360 images instead of an equirectangular projection like Mapillary, the cube map is implemented in JavaFX, IIRC).

I think some of the other devs might be thinking about using JavaFX in JOSM proper at some point in time, which might be the reason for the JavaFX dependency.

comment:6 by taylor.smock, 2 years ago

Ticket #22133 has been marked as a duplicate of this ticket.

by taylor.smock, 2 years ago

Attachment: 22088.3.patch added

More commonality between the start scripts, allow overriding of some paths with environment variables

comment:7 by Don-vip, 2 years ago

Keywords: linux added

comment:8 by taylor.smock, 2 years ago

Resolution: fixed
Status: newclosed

In 18497/josm:

Fix #22088: Debian start script does not properly check if a path does not exist or is not executable

This also fixes #22157: java options from start script not passed

#22157 had two issues:

  1. The start script fell back to /bin/java, which did not allow for version information (we were using the path to java to get the version information)
  2. #!/bin/bash was not working properly

(1) was fixed in the patch for #22088, while (2) was a single-line change.

#22088 largely made the start scripts more portable, and some variables can be
overridden via environment variables.

  • JAVAFX_HOME (defaults to /usr/share/openjfx/lib)
  • JOSM_PATH (defaults to /usr/share/${JOSM_VERSION}/${JOSM_VERSION}.jar, with JOSM_VERSION being one of josm or josm-latest)
  • JAVA_VERSION is now parsed from $JAVACMD -version instead of the binary path
  • The JAVACMD should now always be executable
  • dpkg is no longer required, although it is still checked for, which allows us to prioritize specific Java versions
  • The differences between the josm-latest and josm binaries is 2 lines

comment:9 by skyper, 2 years ago

Regarding r18498:

  • set -ex seems to be wrong
  • Are the two different paths #!/usr/bin/env bash != #!/usr/bin/bash between latest and tested intended?

My hope was that in longterm we could use only on startup script for both, latest and tested, and set all variables in /etc/default/josm resp. /etc/default/josm-latest

in reply to:  9 comment:10 by taylor.smock, 2 years ago

Replying to skyper:

Regarding r18498:

  • set -ex seems to be wrong
  • Are the two different paths #!/usr/bin/env bash != #!/usr/bin/bash between latest and tested intended?

Not wholly -- I was intending to make them the same. That was changed to make certain the checkbashisms command passed with at least one of them (it passed locally in r18497 on Mac, but failed locally with Ubuntu). With that said, the set -ex for latest isn't wholly wrong (AKA, better debug output). It is just slightly more verbose.

My hope was that in longterm we could use only on startup script for both, latest and tested, and set all variables in /etc/default/josm resp. /etc/default/josm-latest

They are (pretty much) the same script. With slightly different lines. I thought about trying to use the name of the script so they would be identical, but decided not to in order to account for someone doing something like ln -s $(which josm) josm-old.

Version 1, edited 2 years ago by taylor.smock (previous) (next) (diff)

comment:11 by skyper, 2 years ago

Resolution: fixed
Status: closedreopened

Oh, I was too fast and did not check carefully enough:
r18497 changed the -Djosm.dir.name from JOSM-latest to "josm-latest", e.g. small letters instead of capital ones for JOSM and surrounded by ".
As tested was not updated, yet, I can not test it but the name for the directories is JOSM with capital letters and I fear it will change to small letters

EDIT:
At least java options are recognized and JAVA_OPTS="-Djosm.dir.name=JOSM-latest" josm-latest works.

Last edited 2 years ago by skyper (previous) (diff)

comment:12 by taylor.smock, 2 years ago

In 18500/josm:

See #22088: use hardcoded josm.dir.name to avoid casing issues

comment:13 by taylor.smock, 2 years ago

Resolution: fixed
Status: reopenedclosed

comment:14 by skyper, 2 years ago

Thanks, Taylor, it is working now.
Additionally, I manually downloaded trunk/native/linux/tested/usr/bin/josm and it is working, too.

in reply to:  8 comment:15 by sebastic, 2 years ago

Replying to taylor.smock:

  1. #!/bin/bash was not working properly

Why? /bin/bash is the canonical location (on non-usrmerge systems):

Using /usr/bin/bash by default on Debian systems is wrong:

$ ls -l /bin/bash
-rwxr-xr-x 1 root root 1230360 May 12 17:05 /bin/bash
$ ls -l /usr/bin/bash
ls: cannot access '/usr/bin/bash': No such file or directory

Your changes broke /usr/bin/josm instead of fixing it.

On usrmerge systems both are available:

# ls -l /bin/bash
-rwxr-xr-x 1 root root 1230360 May 12 15:05 /bin/bash
# ls -l /usr/bin/bash
-rwxr-xr-x 1 root root 1230360 May 12 15:05 /usr/bin/bash

comment:16 by taylor.smock, 2 years ago

I thought usrmerge was mandatory. See https://wiki.debian.org/UsrMerge .

in reply to:  16 comment:17 by sebastic, 2 years ago

Replying to taylor.smock:

I thought usrmerge was mandatory. See https://wiki.debian.org/UsrMerge .

Only new installations use usrmerge by default, upgraded systems need to explicitly install the usrmerge package if they want to adopt Merged /usr.

The dpkg maintainer disagrees about the implementation, so even systems upgraded to bookworm may not be forcibly converted to Merged /usr if no acceptable solution is found before the freeze despite the Technical Committee ruling.

comment:18 by skyper, 2 years ago

Strange, on my bullseye system without usrmerge installed I find both:

# ls -l /bin/bash
-rwxr-xr-x 1 root root 1234376 Aug  4  2021 /bin/bash
# ls -l /usr/bin/bash
-rwxr-xr-x 1 root root 1234376 Aug  4  2021 /usr/bin/bash
# diff -s /bin/bash /usr/bin/bash
Files /bin/bash and /usr/bin/bash are identical

At least on my system, the start script was not working properly, see #22157, but I never understood why I got different results using /usr/bin/bash instead of /bin/bash.

Last edited 2 years ago by skyper (previous) (diff)

comment:19 by taylor.smock, 2 years ago

Let us continue the discussion on #22193.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.