Change jfrconv binary generation from cpp to shell. #1366

roy-soumadipta · 2025-07-07T11:49:56Z

Description

The commit fixes #1344 and includes the following changes:

Rewrite jfrconv launcher script in shell from cpp.
Changes to Makefile to accomodate the shift to shell script.

Related issues

#1344

Motivation and context

How has this been tested?

jfrconv has been tested to show no Move to trash popup by using the binaries from the notarized zip.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The commit fixes async-profiler#1344 and includes the following changes: * Rewrite jfrconv launcher script in shell from cpp. * Changes to Makefile to accomodate the shift to shell script.

apangin · 2025-07-15T13:24:56Z

src/launcher/launcher.sh

+
+set -euo pipefail
+
+VERSION_STRING="JFR converter PROFILER_VERSION built on $(date '+%b %d %Y')"


It is today's date, not the build date, right?

Yes replaced at build time like a macro. Will reflect in next commit

Addressed in commit 444fb5d

apangin · 2025-07-15T13:27:17Z

src/launcher/launcher.sh

+if [[ "$OSTYPE" == "darwin"* ]]; then
+    SCRIPT_PATH="$(cd "$(dirname "$0")" && pwd)/$(basename "$0")"
+else
+    SCRIPT_PATH="$(readlink -f "$0")"
+fi


Is canonicalization necessary? java -jar accepts relative paths too.

this is such a great suggestion. Thanks can be simplified so much

Addressed in commit 444fb5d

src/launcher/launcher.sh

apangin · 2025-07-15T13:34:26Z

src/launcher/launcher.sh

+    SCRIPT_PATH="$(readlink -f "$0")"
+fi
+
+JAVA_CMD=("java" "-Xss2M" "-Dsun.misc.URLClassPath.disableJarChecking")


Why put java in the beginning, if it's always cut later?

yes changed here and other affected places

apangin · 2025-07-15T13:36:16Z

src/launcher/launcher.sh

+# Copyright The async-profiler authors
+# SPDX-License-Identifier: Apache-2.0
+
+#!/bin/bash


bash is not available on all systems. Script must use POSIX compatible /bin/sh.

Addressed in commit 444fb5d

apangin · 2025-07-15T13:42:59Z

src/launcher/launcher.sh

+run_java() {
+    # 1. Try JAVA_HOME
+    if [[ -n "$JAVA_HOME" ]] && find_java "$JAVA_HOME/bin/java"; then
+        exec "$JAVA_HOME/bin/java" "${JAVA_CMD[@]:1}"


Script should continue trying next options if one of exec calls fails.
This is how the original C++ launcher worked.

Addressed in commit 444fb5d

apangin · 2025-07-15T13:45:52Z

src/launcher/launcher.sh

+    exit 1
+}
+
+run_java


Newline here is required not only because of the coding style, but also because it won't work otherwise after concatenation with .jar.

Addressed in commit 444fb5d

apangin · 2025-07-15T13:53:09Z

src/launcher/launcher.sh

+
+JAVA_CMD+=("-jar" "$SCRIPT_PATH" "$@")
+
+find_java() {


Not very meaningful function - there is just one short expression that can be as well inlined.

Addressed in commit 444fb5d

Baraa-Hasheesh · 2025-07-16T07:15:17Z

Makefile


 clean:
-	$(RM) -r build
+	$(RM) -r build


You have styling issue here & in other files
The file should always end with a single empty new line

Addressed in latest commit

Baraa-Hasheesh · 2025-07-16T07:16:10Z

src/launcher/launcher.sh

+
+#!/bin/bash
+
+set -euo pipefail


Do we need this on user script?
This might end up causing weird failures on user side if we're not careful

Isn't this better than silent failures?

Some failures that happen due to set -euo pipefail are due to reasons that would work fine without the flags

Such as unbound variables

I'm fine with keeping this but we should test all possible branches of the script

If all possible branches were not tested then this isn't really safe

Which ones do you suggest removing?

All of them raise some level of risk

The question shouldn't be "which to remove"

The proper question should be were all possible cases tested with these flags?

If we have enough testing then keeping this should be fine

For example the script should continue even if a previous exec fails right?

However if an exec fails wouldn't it cause an exit code of 1?

Wouldn't this break the script?

Let's not delve into philosophy. A simple answer is - leave options that are necessary and delete those that aren't.
To me, -e does not seem useful here: there are basically no commands that can fail except for exec, but I explicitly noted that the script should proceed if exec fails.
The same for pipefail: I don't see why it is here, given there are no pipes in the script.
While -u is generally useful, again, it has no behavior impact in this particular script.
Please correct me if I'm wrong, otherwise just remove the entire line. The less code, the better.

Baraa-Hasheesh · 2025-07-16T07:18:12Z

src/launcher/launcher.sh

+fi
+
+if [[ "$OSTYPE" == "darwin"* ]]; then
+    SCRIPT_PATH="$(cd "$(dirname "$0")" && pwd)/$(basename "$0")"


pwd)?

wouldn't realpath $0 solve for both?

this is removed after Andrei's suggestion

Addressed in commit 444fb5d

Baraa-Hasheesh · 2025-07-16T07:22:19Z

src/launcher/launcher.sh

+    fi
+
+    # 2. Try PATH
+    if command -v java >/dev/null 2>&1; then


Do we really need all this logic for finding java
can't we use PATH always?

if missing we can print a message telling the user to add it?
If the problem is with our jar being compatible with a specific minimum Java version, we can add a check for that as well

This logic might be hiding bugs as if the user has 10 JDKs it could be failing for the first 9 while passing for the last one

@apangin what do you think?

I think the script is doing what it's intended to do - find any working Java Runtime, even if java is not on the PATH.

Baraa-Hasheesh · 2025-07-16T07:33:58Z

src/launcher/launcher.sh

+    exit 0
+fi
+
+if [[ "$OSTYPE" == "darwin"* ]]; then


why not use uname -s as we do in Makefile?

We have an env variable ready to use. Why go through uname -s?

For consistency

I don't get how are we deviating from consistency by using a ready to use variable?

Different approach than what we have in the Makefile

Addressed in aeb211f to maintain uniformity and posix compliance

krk · 2025-07-16T18:54:45Z

src/launcher/launcher.sh

+run_java() {
+    # 1. Try JAVA_HOME
+    if [[ -n "$JAVA_HOME" ]] && [[ -x "$JAVA_HOME/bin/java" ]]; then
+        "$JAVA_HOME/bin/java" "${JAVA_CMD[@]}" && exit 0


Why not exec?

For fallback as per Andrei's comment on the previous commit. With prev commit when exec command itself failed it would proceed to try the next for example if java is not present in the path corresponding to the exec command. But now if the exec succeeds but then encounters an error, it won't try the subsequent alternatives. @apangin is this the change you wanted or the previous impl using exec suffices your expectation?

@apangin I checked again and in the previous launcher cpp, there was no fallback if exec succeeds but then error encountered. So the previous impl in shell script sufficed the need as per the existing one. Should I revert again or do you want the changes now for a better fallback?

Script should proceed with the next attempt if exec itself fails, not the target command. There should be no fallback when java process is launched but fails for whatever reason. Basically, the script should retain the same behavior as in the original launcher.

Ok I will change it accordingly. The previous one however wouldn't have proceeded due to set -e. There is a comment from @Baraa-Hasheesh regarding the set -euo pipefail. @apangin if you can share your thoughts so that I can make this change accordingly

Addressed in 385001b

apangin · 2025-07-16T20:11:03Z

src/launcher/launcher.sh

+# Copyright The async-profiler authors
+# SPDX-License-Identifier: Apache-2.0
+
+#!/bin/sh


Shebang must stay on the first line. License header shall come after.

Addressed in 385001b

apangin · 2025-07-16T20:29:32Z

src/launcher/launcher.sh

+
+#!/bin/bash
+
+set -euo pipefail


Let's not delve into philosophy. A simple answer is - leave options that are necessary and delete those that aren't.
To me, -e does not seem useful here: there are basically no commands that can fail except for exec, but I explicitly noted that the script should proceed if exec fails.
The same for pipefail: I don't see why it is here, given there are no pipes in the script.
While -u is generally useful, again, it has no behavior impact in this particular script.
Please correct me if I'm wrong, otherwise just remove the entire line. The less code, the better.

apangin · 2025-07-16T20:37:04Z

src/launcher/launcher.sh

+
+set -euo pipefail
+
+VERSION_STRING="JFR converter PROFILER_VERSION built on BUILD_DATE"


There is no much value in introducing variables that are used only once in a local context.
Just inline the string where needed.

Addressed in 385001b

apangin · 2025-07-16T20:37:19Z

src/launcher/launcher.sh

+    exit 0
+fi
+
+SCRIPT_PATH="$0"


Same as above.
Good code is like a poem: every word counts.

Addressed in 385001b

apangin · 2025-07-16T20:44:48Z

src/launcher/launcher.sh

+
+JAVA_CMD+=("-jar" "$SCRIPT_PATH" "$@")
+
+run_java() {


Is there a reason to put it in a function, if the following code is executed as is anyway?
E.g., the above argument parsing logic is not in a function, and that's fine.

Since this is not a big script it can be removed

Addressed in 385001b

apangin · 2025-07-17T11:54:32Z

test.html

Added by mistake

apangin · 2025-07-17T11:56:30Z

src/launcher/launcher.sh

+JAVA_CMD+=("-jar" "$0" "$@")
+
+# 1. Try JAVA_HOME
+if [[ -n "$JAVA_HOME" ]] && [[ -x "$JAVA_HOME/bin/java" ]]; then


AFAIK, [[ is also a bash extension. Strictly POSIX-compliant shell only supports [.

Had to make other changes as well for full compliance. Addressed in 52af9cc

apangin · 2025-07-17T13:39:14Z

src/launcher/launcher.sh

+            fi
+            ;;
+        -agent*)
+            if [ "$(echo "$1" | cut -c1-6)" = "-agent" ]; then


What does this expression verify? Don't we get in this case only if the argument starts with -agent?

Yes this is redundant. Removed in 6f07677

Signed-off-by: Andrei Pangin <1749416+apangin@users.noreply.github.com>

apangin · 2025-07-17T15:00:11Z

Merged, thanks!

Change jfrconv binary generation from cpp to shell.

4c14794

The commit fixes async-profiler#1344 and includes the following changes: * Rewrite jfrconv launcher script in shell from cpp. * Changes to Makefile to accomodate the shift to shell script.

roy-soumadipta marked this pull request as draft July 7, 2025 13:44

roy-soumadipta added 11 commits July 7, 2025 14:46

Embed .jar

d79601b

Try fix for Makefile for linux.

d42d829

Try fix2 for Makefile for linux.

186b98d

Try fix for macos.

25c296c

Try fix2 for macos.

431aa05

Use JNI approach.

319d182

Use JNI approach with custom class loader.

eb3844c

Try fix for ljvm

c50c3ed

Fix cland-tidy and macos issues

7ddd8d6

Fix cland-tidy and macos issues

b629ef7

Fix cland-tidy and ljvm

d124ab8

fandreuz mentioned this pull request Jul 7, 2025

Publish clang-tidy comments only for non-draft PRs #1367

Merged

roy-soumadipta added 8 commits July 7, 2025 17:55

Fix jvm lib path

15f9536

Fix jvm lib path

25dbd2a

Fix jvm lib path

4d076a9

Fix jvm lib path for macos

55e9483

Restart workflow

002b9a9

Try shell way again

fab1907

upload notarized zip

9b32aa3

Remove notarized zip.

444fb5d

roy-soumadipta marked this pull request as ready for review July 15, 2025 09:28

apangin reviewed Jul 15, 2025

View reviewed changes

Baraa-Hasheesh reviewed Jul 16, 2025

View reviewed changes

Address comments.

41671de

krk reviewed Jul 16, 2025

View reviewed changes

apangin reviewed Jul 16, 2025

View reviewed changes

roy-soumadipta added 2 commits July 17, 2025 12:23

Simplify script.

385001b

Change to uname for posix compliance.

aeb211f

apangin reviewed Jul 17, 2025

View reviewed changes

Changes to launcher for full posix compliance.

52af9cc

apangin reviewed Jul 17, 2025

View reviewed changes

roy-soumadipta and others added 2 commits July 17, 2025 15:12

Remove redundant length check for agent.

6f07677

Removed extra newline

286cf72

Signed-off-by: Andrei Pangin <1749416+apangin@users.noreply.github.com>

apangin merged commit 5fffdb1 into async-profiler:master Jul 17, 2025
7 checks passed

roy-soumadipta deleted the issue_1344 branch July 17, 2025 17:46


		set -euo pipefail

		VERSION_STRING="JFR converter PROFILER_VERSION built on $(date '+%b %d %Y')"


		JAVA_CMD+=("-jar" "$SCRIPT_PATH" "$@")

		find_java() {


		JAVA_CMD+=("-jar" "$SCRIPT_PATH" "$@")

		run_java() {

Change jfrconv binary generation from cpp to shell. #1366

Change jfrconv binary generation from cpp to shell. #1366

Uh oh!

Conversation

roy-soumadipta commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Motivation and context

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roy-soumadipta Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roy-soumadipta Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roy-soumadipta commented Jul 7, 2025 •

edited

Loading

roy-soumadipta Jul 15, 2025 •

edited

Loading

roy-soumadipta Jul 16, 2025 •

edited

Loading

roy-soumadipta Jul 16, 2025 •

edited

Loading