[Fix] JDK is successfully installed if `bin/java` files exist #452

crogoz · 2024-10-30T16:01:52Z

Before this PR

Context: https://pl.ntr/2p9
We consider that a JDK was successfully installed if the directory exists.
However, this is not always true. eg. if an exception happens while the JDK is copied over to the destination directory (https://github.com/palantir/gradle-jdks/blob/develop/gradle-jdks-setup/src/main/java/com/palantir/gradle/jdks/setup/GradleJdkInstallationSetup.java#L130). The JDK directory might not contain the bin/java executable.

After this PR

A JDK directory is considered successfully installed if bin/java exists .
I am also doing a copy to a temporary directory and an atomic move to the destination directory.
A sentinel file is added at the end of a successful installation - I think we might want to use this in the future to check if the jdk was successfully installed or not.

Note: I am not considering for now only the sentinel file to be the source of truth because of backwards compatibility (the jdks we installed so far won't have the sentinel file).

==COMMIT_MSG==
[Fix] JDK is successfully installed if bin/java exists
==COMMIT_MSG==

Possible downsides?

changelog-app · 2024-10-30T16:01:56Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

[Fix] JDK is successfully installed if bin/java or sentinel files exist

Check the box to generate changelog(s)

Generate changelog entry

…adle-jdks into cr/more-sanity-checks-script

crogoz · 2024-10-31T10:12:25Z

gradle-jdks-setup/src/main/resources/gradle-jdks-functions.sh

-      echo "Successfully installed JDK distribution in $jdk_installation_directory"
+    elif [ ! -f "$jdk_installation_directory/bin/java" ]; then
+      echo "Java executable not found in $jdk_installation_directory/bin/java, re-installing the JDK...."
+    else
+      echo "JDK installation $jdk_installation_directory was already configured"
+      continue
+    fi


the github diff is weird, the only changes I have made is adding the if-else conditions to check if the directory and/or bin/java files exists at the top. the rest of the code is unchanged

CRogers · 2024-10-31T11:43:48Z

...ationTest/java/com/palantir/gradle/jdks/setup/GradleJdkInstallationSetupIntegrationTest.java

@@ -153,7 +159,8 @@ private static void writeFileContent(Path path, String content) throws IOExcepti
        Files.writeString(path, content + "\n");
    }

-    private void dockerBuildAndRunTestingScript(String baseImage, String shell, boolean installCurl)
+    private void dockerBuildAndRunTestingScript(
+            String baseImage, String shell, boolean installCurl, boolean addEmptyGradleJdkDir)


I assumed this meant the ~/.gradle/gradle-jdks dir - maybe call this just addJdkDir?

CRogers · 2024-10-31T11:45:29Z

gradle-jdks-setup/src/main/java/com/palantir/gradle/jdks/setup/GradleJdkInstallationSetup.java

+                logger.log("Cannot move the directory to the final directory, attempting to do a non-atomic operation");
+                Files.move(tempDirWithPrefix, destinationJdkInstallationDirectory, StandardCopyOption.REPLACE_EXISTING);
+            }
+            createSentinelFile(destinationJdkInstallationDirectory);


What is this sentinel file used for?

I don't think it's used elsewhere in this diff and now we have the atomic move the directory/java binary existing should be enough.

I am thinking that the sentinel file is the flag that tells us if the installation finished successfully. I was thinking that in the future (if we add the check now , we would re-install jdks that are already correctly installed on user's laptops) we might assume that the installation was successful if the file was created.

I would hope that since we move the JDK now it will definitely be fine as the move will mean the directory is either there and correct or not there, rather than just partially copied.

ack, I'll remove this then

CRogers · 2024-10-31T11:50:02Z

gradle-jdks-setup/src/main/java/com/palantir/gradle/jdks/setup/GradleJdkInstallationSetup.java

                logger.log(String.format("Distribution URL %s already exists", destinationJdkInstallationDirectory));
                return false;
            }
            logger.log(
                    String.format("Copying JDK from %s into %s", currentJavaHome, destinationJdkInstallationDirectory));
-            FileUtils.copyDirectory(currentJavaHome, destinationJdkInstallationDirectory);
+            Path tempDirWithPrefix = Files.createTempDirectory("gradle-jdks");


Atomic moves generally only work when the source and destination for the filesystem object are on the same filesystem. If they're not it's usually actually a copy. I think here this risks copying to a tmp filesystem, then copying again to the filesystem ~/.gradle/gradle-jdks is mounted on rather than moving to the final destination.

To work around this, I'd make a tmp dir at ~/.gradle/gradle-jdks/tmp and copy it to a subdir in there first before moving. It should definitely be on the same filesystem then.

CRogers · 2024-10-31T11:51:06Z

gradle-jdks-setup/src/main/java/com/palantir/gradle/jdks/setup/GradleJdkInstallationSetup.java

                logger.log(String.format("Distribution URL %s already exists", destinationJdkInstallationDirectory));
                return false;
            }
            logger.log(
                    String.format("Copying JDK from %s into %s", currentJavaHome, destinationJdkInstallationDirectory));
-            FileUtils.copyDirectory(currentJavaHome, destinationJdkInstallationDirectory);
+            Path tempDirWithPrefix = Files.createTempDirectory("gradle-jdks");


I would also delete the temp dir if it still exists in a try-finally to make sure it's cleaned even if there's an exception. Don't want an ever increasing number of these.

CRogers · 2024-10-31T11:53:19Z

gradle-jdks-setup/src/main/java/com/palantir/gradle/jdks/setup/FileUtils.java

@@ -65,13 +66,13 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes _attrs) t

            @Override
            public FileVisitResult visitFile(Path file, BasicFileAttributes _attrs) throws IOException {
-                Files.copy(file, destination.resolve(source.relativize(file)));
+                Files.copy(file, destination.resolve(source.relativize(file)), StandardCopyOption.REPLACE_EXISTING);


Do we still need to replace-existing with the copy to a temp location and then move?

I think it is still better to keep the replace-existing, otherwise if some crash leaves the tmp location un-deleted, then we can recover from it.

CRogers · 2024-10-31T12:00:16Z

gradle-jdks-setup/src/main/resources/gradle-jdks-functions.sh

-      "$java_home"/bin/java -cp "$scripts_dir"/gradle-jdks-setup.jar com.palantir.gradle.jdks.setup.GradleJdkInstallationSetup jdkSetup "$jdk_installation_directory" || die "Failed to set up JDK $jdk_installation_directory"
-      echo "Successfully installed JDK distribution in $jdk_installation_directory"
+    elif [ ! -f "$jdk_installation_directory/bin/java" ]; then
+      echo "Java executable not found in $jdk_installation_directory/bin/java, re-installing the JDK...."


I think worth having a comment about the possibly broken existing JDKs.

this file will be added to all the repos (it is used both by the docker images and the gradle jdk enabled repos). Is it OK to mention the possibly broken JDKs in a comment ?

CRogers · 2024-11-01T15:51:09Z

gradle-jdks-setup/src/main/java/com/palantir/gradle/jdks/setup/GradleJdkInstallationSetup.java

+                        tempCopyDir,
+                        destinationJdkInstallationDirectory,
+                        StandardCopyOption.REPLACE_EXISTING,
+                        StandardCopyOption.ATOMIC_MOVE);


The common filesystems (ieg APFS/NTFS/ext4) all support atomic moves... but perhaps we should handle the AtomicMoveNotSupportedException like before just incase this ends up on a different filesystem that doesn't?

autorelease3 · 2024-11-04T16:34:06Z

Released 0.54.0

crogoz added 6 commits October 30, 2024 13:14

test

7de72ff

.

d3c4c45

.

4337b1d

check

e825d59

check

b8b5b81

.

03f909f

Add generated changelog entries

6ef6bc2

crogoz changed the title ~~[Fix] JDK is successfully installed if bin/java exists~~ [Fix] JDK is successfully installed if bin/java or sentinel files exist Oct 30, 2024

Add generated changelog entries

3a49186

crogoz marked this pull request as ready for review October 30, 2024 16:03

crogoz requested a review from CRogers October 30, 2024 16:07

crogoz added 3 commits October 30, 2024 17:30

tab

b23d320

Merge branch 'cr/more-sanity-checks-script' of github.com:palantir/gr…

d25dfc3

…adle-jdks into cr/more-sanity-checks-script

copy & move

c0cc327

crogoz changed the title ~~[Fix] JDK is successfully installed if bin/java or sentinel files exist~~ [Fix] JDK is successfully installed if bin/java files exist Oct 31, 2024

crogoz added 2 commits October 31, 2024 11:03

.

0a62990

just create for now

cad62f2

crogoz commented Oct 31, 2024

View reviewed changes

CRogers reviewed Oct 31, 2024

View reviewed changes

.

f5865ae

crogoz requested a review from CRogers October 31, 2024 13:39

CRogers reviewed Nov 1, 2024

View reviewed changes

catch atomic move ex

132a4f0

CRogers approved these changes Nov 4, 2024

View reviewed changes

CRogers added autorelease merge when ready labels Nov 4, 2024

bulldozer-bot bot merged commit 8114390 into develop Nov 4, 2024
6 checks passed

bulldozer-bot bot deleted the cr/more-sanity-checks-script branch November 4, 2024 16:34

CRogers mentioned this pull request Nov 5, 2024

Remove noisy output #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] JDK is successfully installed if `bin/java` files exist #452

[Fix] JDK is successfully installed if `bin/java` files exist #452

crogoz commented Oct 30, 2024 •

edited

Loading

changelog-app bot commented Oct 30, 2024 •

edited by crogoz

Loading

crogoz Oct 31, 2024

CRogers Oct 31, 2024 •

edited

Loading

CRogers Oct 31, 2024

CRogers Oct 31, 2024 •

edited

Loading

crogoz Oct 31, 2024

CRogers Oct 31, 2024

crogoz Oct 31, 2024

CRogers Oct 31, 2024 •

edited

Loading

CRogers Oct 31, 2024

CRogers Oct 31, 2024

crogoz Oct 31, 2024

CRogers Oct 31, 2024

crogoz Oct 31, 2024

CRogers Nov 1, 2024

autorelease3 bot commented Nov 4, 2024

[Fix] JDK is successfully installed if bin/java files exist #452

[Fix] JDK is successfully installed if bin/java files exist #452

Conversation

crogoz commented Oct 30, 2024 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Oct 30, 2024 • edited by crogoz Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

CRogers Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRogers Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CRogers Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autorelease3 bot commented Nov 4, 2024

[Fix] JDK is successfully installed if `bin/java` files exist #452

[Fix] JDK is successfully installed if `bin/java` files exist #452

crogoz commented Oct 30, 2024 •

edited

Loading

changelog-app bot commented Oct 30, 2024 •

edited by crogoz

Loading

Generate changelog in `changelog/@unreleased`

CRogers Oct 31, 2024 •

edited

Loading

CRogers Oct 31, 2024 •

edited

Loading

CRogers Oct 31, 2024 •

edited

Loading