Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-69570

Java version check incorrect in init.d/jenkins

    • 2.368

      The current Ubuntu/Debian LTS installation (2.361.1) includes a startup script under /etc/init.d/jenkins

      The function check_arguments() contains a check on the Java version, which doesn't accept the (supported) Java 17. We agree that under native Debian/Ubuntu this is a minor issue, since this startup script is not supposed to be used anymore, but under Windows WSL it is common to start Jenkins still like that (which currently fails without adapting the script for Java 17)

          [JENKINS-69570] Java version check incorrect in init.d/jenkins

          Basil Crow added a comment -

          Fixed in jenkinsci/packaging#335 toward 2.368.

          Basil Crow added a comment - Fixed in jenkinsci/packaging#335 toward 2.368.

          Basil Crow added a comment -

          Proposing for backport to 2.361.2 LTS with the following justification: this is a low-risk and simple fix for an annoying bug that prevents usage with Java 17 on System V init Debian based systems. The "bonus" part of the linked PR (failing fast on Java 1.8 on systemd-based systems) also seems safe and desirable, since failing fast helps users identify problems more easily rather than driving on and failing later on.

          Basil Crow added a comment - Proposing for backport to 2.361.2 LTS with the following justification: this is a low-risk and simple fix for an annoying bug that prevents usage with Java 17 on System V init Debian based systems. The "bonus" part of the linked PR (failing fast on Java 1.8 on systemd-based systems) also seems safe and desirable, since failing fast helps users identify problems more easily rather than driving on and failing later on.

          Ian Williams added a comment -

          I find it rather interesting in jenkinsci/packaging#335  that:
          init.d/jenkins has the affirmative condition,

          	if [ "${JAVA_VERSION}" = "17" ]; then
          		echo "Correct java version found" >&2
          	elif [ "${JAVA_VERSION}" = "11" ]; then
          		echo "Correct java version found" >&2
          	else
          		echo "Found an incorrect Java version" >&2
          		return 2
          	fi
          

          while systemd/jenkins.sh has the double negative condition

          	if [ -z "${java_version}" ]; then
          		return 1
          	elif [ "${java_version}" != "17" ] && [ "${java_version}" != "11" ]; then
          		return 1
          	else
          		return 0
          	fi
          

          I would have expected similar test conditions in both.

          Ian Williams added a comment - I find it rather interesting in jenkinsci/packaging#335   that : init.d/jenkins has the affirmative condition, if [ "${JAVA_VERSION}" = "17" ]; then echo "Correct java version found" >&2 elif [ "${JAVA_VERSION}" = "11" ]; then echo "Correct java version found" >&2 else echo "Found an incorrect Java version" >&2 return 2 fi while systemd/jenkins.s h has the double negative condition if [ -z "${java_version}" ]; then return 1 elif [ "${java_version}" != "17" ] && [ "${java_version}" != "11" ]; then return 1 else return 0 fi I would have expected similar test conditions in both.

          Basil Crow added a comment -

          Why is it interesting, and why would you have that expectation?

          Basil Crow added a comment - Why is it interesting, and why would you have that expectation?

          Ian Williams added a comment -

          Not saying it needs to be changed; just commenting here since the PR is already merged.

          The check is appropriate in both locations (so no DRY), and init.d and systemd have their own historic styles but ..

          It increases the cognitive load required to "make the same change" in two different locations applying different logic. It increases the likelihood of introducing an error, especially given the default "else" condition being different.

          If you were driving and came across the following signs at 3 successive intersections, the cognitive load is far higher than seeing the same sign(s) at each intersection, though the resulting allowed action is the same.


          (The 3rd sign is not an approved sign.)

          If I were implementing it,I'd apply the limited affirmative ..

          if [ "17" = "${java_version}"  ] || [ "11" = "${java_version}"  ]; then
             return 0
          else
             return 1
          fi
          

           

          Ian Williams added a comment - Not saying it needs to be changed; just commenting here since the PR is already merged. The check is appropriate in both locations (so no DRY), and init.d and systemd have their own historic styles but .. It increases the cognitive load required to "make the same change" in two different locations applying different logic. It increases the likelihood of introducing an error, especially given the default "else" condition being different. If you were driving and came across the following signs at 3 successive intersections, the cognitive load is far higher than seeing the same sign(s) at each intersection, though the resulting allowed action is the same. (The 3rd sign is not an approved sign.) If I were implementing it,I'd apply the limited affirmative .. if [ "17" = "${java_version}" ] || [ "11" = "${java_version}" ]; then return 0 else return 1 fi  

          Basil Crow added a comment -

          I do not think it is reasonable to expect code implemented by multiple people over a range of time spanning more than a decade to be symmetric. If the lack of symmetry of these implementations bothers you, I would encourage you to open a pull request.

          Basil Crow added a comment - I do not think it is reasonable to expect code implemented by multiple people over a range of time spanning more than a decade to be symmetric. If the lack of symmetry of these implementations bothers you, I would encourage you to open a pull request.

            basil Basil Crow
            tomvst Tom Van Steertegem
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: