8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files #23923

alexeysemenyukoracle · 2025-03-05T21:13:44Z

jpackage app laucnher will expand environment variables in .cfg files.

Previously jpackage app launcher only replaced $APPDIR, $BINDIR, and $ROOTDIR tokens with the corresponding path values. With this patch, any environment variable can be expanded. The syntax is shell-like $ENV_VAR_NAME or ${ENV_VAR_NAME}. If $ENV_VAR_NAME syntax is used, the variable name stops at the first character outside of [a-zA-Z0-9_] character range. If ${ENV_VAR_NAME} syntax is used, the variable name stops at the first } character after ${ substring. E.g:

String	Variables	Variable Values	Expanded String	Note
Welcome $USER!	USER	USER=John	Welcome John!
Welcome $USER!	USER	not set	Welcome $USER!	Unset variables are not expanded.
Welcome $USER2	USER2	USER2=John	Welcome John!
Welcome ${USER}2!	USER	USER=John	Welcome John2!
Welcome $USER-2!	USER	USER=John	Welcome John-2!
Welcome ${USER-2}!	USER-2	USER-2=John	Welcome John!	`USER-2` is likely to be an invalid env variable name, but jpackage launcher is not validating names.

\$ character combination prevents variable expansion:

String	Variables	Variable Values	Expanded String
Hello \$A! Bye $B	B	B=John	Hello $A! Bye John
Hello \${A}! Bye $B	B	B=John	Hello ${A}! Bye John

\\ character combination escapes \:

String	Variables	Variable Values	Expanded String
Hello \\$A! Bye $B	A, B	A=Ana B=John	Hello \Ana! Bye John

If \ character is not followed by another \ character or $ character, it is interpreted as a regular character:

String	Expanded string
a\b\c	a\b\c

Expansion is non-recursive:

String	Variables	Variable Values	Expanded String	Note
Hello $A!	A	A=$B B=John	Hello $B	Variable "B" will not be expanded

Values of APPDIR, BINDIR, and ROOTDIR environment variables are ignored, and these names are substituted by values calculated by jpackage app launcher as previously.

$APPDIR is equivalent to ${APPDIR}.
$BINDIR is equivalent to ${BINDIR}.
$ROOTDIR is equivalent to ${ROOTDIR}.

You will find two entities dealing with token replacement in this patch:

StringProcessing.cpp/.h - code that performs variable substitution in .cfg files;
TokenReplace.java - helper class for jpackage tests. It will be later used with other jpackage tests and to implement jdk.jpackage.internal.OverridableResource.substitute() method.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8351447 to be approved

Issues

JDK-8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files (Enhancement - P4)
JDK-8351447: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files (CSR)

Reviewers

Alexander Matveev (@sashamatveev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23923/head:pull/23923
$ git checkout pull/23923

Update a local copy of the PR:
$ git checkout pull/23923
$ git pull https://git.openjdk.org/jdk.git pull/23923/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23923

View PR using the GUI difftool:
$ git pr show -t 23923

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23923.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-05T21:14:23Z

👋 Welcome back asemenyuk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-05T21:15:12Z

@alexeysemenyukoracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files

Reviewed-by: almatvee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 9 new commits pushed to the master branch:

5c438c5: 8352748: Remove com.sun.tools.classfile from the JDK
e87ff32: 8351034: Add AVX-512 intrinsics for ML-DSA
e3f26b0: 8351319: AOT cache support for custom class loaders broken since JDK-8348426
... and 6 more: https://git.openjdk.org/jdk/compare/5f2a604b633c0cd24f897f828a7c928c3d2b651c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-03-05T21:15:54Z

@alexeysemenyukoracle The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

alexeysemenyukoracle · 2025-03-06T01:05:04Z

@sashamatveev PTAL

mlbridge · 2025-03-06T01:08:44Z

Webrevs

sashamatveev · 2025-03-06T02:18:56Z

If you do echo on macOS in terminal for unset variable it will be expanded to empty string, so for "Unset variables are not expanded." should we have Welcome ! instead of Welcome $USER!?

sashamatveev

Looks good overall with minor comment and question.

sashamatveev · 2025-03-06T03:05:46Z

src/jdk.jpackage/share/native/applauncher/StringProcessing.cpp

+    for (; it != end; ++it) {
+        if (it->type() == ESCAPED_CHAR) {
+            ss << it->value().substr(1);
+        }


} else { in one line for consistency.

src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/TokenReplace.java

openjdk · 2025-03-06T15:22:30Z

⚠️ @alexeysemenyukoracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

alexeysemenyukoracle · 2025-03-06T19:17:32Z

This is true for any shell behavior. It is also true that some shells fail to expand undefined variables if configured accordingly. Bash, for example:

bash -c 'FOO=10; echo FOO=$FOO; unset FOO; set -u; echo FOO=$FOO'

Output:

FOO=10
bash: line 1: FOO: unbound variable

.cfg files are not scripts and jpackage app launcher is not an interpreter. Performed string substitution is less variable expansion and more token replacement; if token value is not available, it is not replaced.
Regardless of handling undefined variables, users have to deal with the consequences. Say they pass --java-options -DmyAppData=$HOME/.myData option to jackage and HOME variable happens undefined on a machine where installed app is launched. How would /.myData be better than $HOME/.myData? At least it is clear something went wrong in the second case.

alexeysemenyukoracle · 2025-03-06T22:35:33Z

/csr

openjdk · 2025-03-06T22:35:49Z

@alexeysemenyukoracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@alexeysemenyukoracle please create a CSR request for issue JDK-8341641 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2025-03-18T02:54:31Z

@alexeysemenyukoracle this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8341641-master
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

sashamatveev

Latest changes looks good.

alexeysemenyukoracle · 2025-04-03T22:18:37Z

I'll update the tests to cover case sensitivity differences on Unix and Windows.

openjdk · 2025-04-09T20:43:31Z

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

… *.cfg files

…nReplace.java Co-authored-by: Andrey Turbanov <[email protected]>

openjdk · 2025-04-09T20:47:21Z

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

… are case-insesitive on Windows)

openjdk · 2025-04-09T20:53:04Z

@alexeysemenyukoracle Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

alexeysemenyukoracle · 2025-04-09T22:40:17Z

Updated the implementation and tests to comply with the CSR. @sashamatveev PTAL

sashamatveev

Looks good.

alexeysemenyukoracle · 2025-04-10T01:38:10Z

/integrate

openjdk · 2025-04-10T01:38:51Z

Going to push as commit 45b7c74.
Since your change was applied there have been 9 commits pushed to the master branch:

5c438c5: 8352748: Remove com.sun.tools.classfile from the JDK
e87ff32: 8351034: Add AVX-512 intrinsics for ML-DSA
e3f26b0: 8351319: AOT cache support for custom class loaders broken since JDK-8348426
... and 6 more: https://git.openjdk.org/jdk/compare/5f2a604b633c0cd24f897f828a7c928c3d2b651c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-10T01:39:03Z

@alexeysemenyukoracle Pushed as commit 45b7c74.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

magicus · 2025-04-10T14:42:26Z

src/jdk.jpackage/share/man/jpackage.md

+
+    An expandable substring should be enclosed between the dollar
+    sign character ($) and the first following non-alphanumeric
+    character. Alternatively, it can be enclosed between "${" and "}"


This did not sit well with pandoc, that reports:

[WARNING] Could not convert TeX math ) and the first following non-alphanumeric character. Alternatively, it can be enclosed between ", rendering as TeX: e enclosed between " ^ unexpected '"' expecting "\\bangle", "\\brace", "\\brack", "\\choose", "\\displaystyle", "\\textstyle", "\\scriptstyle", "\\scriptscriptstyle", "{", "\\operatorname", letter, digit, ".", "!", "'", "''", "'''", "''''", "*", "+", ",", "-", ".", "/", ":", ":=", ";", "<", "=", ">", "?", "@", "~", "_", "^", "\\left", "\\", "\\hyperref" or end of input

I'm guessing a possible fix would be to use

`${`

or

"`${`"

instead.

Alternatively, we need to tell pandoc to not try and use any encoded TeX math in markdown files.

I filed https://bugs.openjdk.org/browse/JDK-8354320 to fix the warning

Adding backslash character (\) before the dollar character ("${" -> "\${") suppressed the warning, and the man page looks as expected.

PR #24585

As I said in that PR, yes, it silences the warning, but it is not standard markdown, and will be rendered incorrectly by a standard markdown renderer.

openjdk bot added the core-libs label Mar 5, 2025

alexeysemenyukoracle marked this pull request as ready for review March 6, 2025 01:04

openjdk bot added the rfr label Mar 6, 2025

sashamatveev reviewed Mar 6, 2025

View reviewed changes

turbanoff reviewed Mar 6, 2025

View reviewed changes

src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/TokenReplace.java Outdated Show resolved Hide resolved

openjdk bot added the csr label Mar 6, 2025

alexeysemenyukoracle mentioned this pull request Mar 7, 2025

8351372: Improve negative tests coverage of jpackage #23936

Closed

3 tasks

openjdk bot added the merge-conflict label Mar 18, 2025

openjdk bot added rfr and removed rfr merge-conflict csr labels Apr 1, 2025

sashamatveev approved these changes Apr 3, 2025

View reviewed changes

openjdk bot added the ready label Apr 3, 2025

Move token substitution in generic TokenReplace class.

0bd411b

alexeysemenyukoracle force-pushed the JDK-8341641-master branch from 2697acd to 3b439c9 Compare April 9, 2025 20:43

openjdk bot removed the ready label Apr 9, 2025

alexeysemenyukoracle and others added 8 commits April 9, 2025 16:46

Moved $APPDIR, $BINDIR, and $ROOTDIR macros to JPackageCommand

a533e3a

8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in…

8b23689

… *.cfg files

Applied bin/blessed-modifier-order.sh

8739db9

Update copyright year

8ccdc6a

A bit of javadoc

190b54f

Shorter test desc

1f738cb

Update src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/Toke…

b23359a

…nReplace.java Co-authored-by: Andrey Turbanov <[email protected]>

Update help

642ad20

alexeysemenyukoracle force-pushed the JDK-8341641-master branch from 3b439c9 to 01270d3 Compare April 9, 2025 20:46

Update tests and implementation to comply with the CSR (env var names…

Loading
Loading status checks…

2b22c14

… are case-insesitive on Windows)

alexeysemenyukoracle force-pushed the JDK-8341641-master branch from 01270d3 to 2b22c14 Compare April 9, 2025 20:52

sashamatveev approved these changes Apr 10, 2025

View reviewed changes

openjdk bot added the ready label Apr 10, 2025

openjdk bot added the integrated label Apr 10, 2025

openjdk bot closed this Apr 10, 2025

openjdk bot removed ready rfr labels Apr 10, 2025

magicus reviewed Apr 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files #23923

8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files #23923

alexeysemenyukoracle commented Mar 5, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 5, 2025

openjdk bot commented Mar 5, 2025 •

edited

Loading

openjdk bot commented Mar 5, 2025

alexeysemenyukoracle commented Mar 6, 2025

mlbridge bot commented Mar 6, 2025 •

edited

Loading

sashamatveev commented Mar 6, 2025

sashamatveev left a comment

sashamatveev Mar 6, 2025

openjdk bot commented Mar 6, 2025

alexeysemenyukoracle commented Mar 6, 2025

alexeysemenyukoracle commented Mar 6, 2025

openjdk bot commented Mar 6, 2025

openjdk bot commented Mar 18, 2025

sashamatveev left a comment

alexeysemenyukoracle commented Apr 3, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

alexeysemenyukoracle commented Apr 9, 2025

sashamatveev left a comment

alexeysemenyukoracle commented Apr 10, 2025

openjdk bot commented Apr 10, 2025

openjdk bot commented Apr 10, 2025

magicus Apr 10, 2025

magicus Apr 10, 2025

alexeysemenyukoracle Apr 10, 2025

alexeysemenyukoracle Apr 10, 2025 •

edited

Loading

magicus Apr 11, 2025

8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files #23923

8341641: Make %APPDATA% and %LOCALAPPDATA% env variables available in *.cfg files #23923

Conversation

alexeysemenyukoracle commented Mar 5, 2025 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Mar 5, 2025

openjdk bot commented Mar 5, 2025 • edited Loading

openjdk bot commented Mar 5, 2025

alexeysemenyukoracle commented Mar 6, 2025

mlbridge bot commented Mar 6, 2025 • edited Loading

Webrevs

sashamatveev commented Mar 6, 2025

sashamatveev left a comment

Choose a reason for hiding this comment

sashamatveev Mar 6, 2025

Choose a reason for hiding this comment

openjdk bot commented Mar 6, 2025

alexeysemenyukoracle commented Mar 6, 2025

alexeysemenyukoracle commented Mar 6, 2025

openjdk bot commented Mar 6, 2025

openjdk bot commented Mar 18, 2025

sashamatveev left a comment

Choose a reason for hiding this comment

alexeysemenyukoracle commented Apr 3, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

alexeysemenyukoracle commented Apr 9, 2025

sashamatveev left a comment

Choose a reason for hiding this comment

alexeysemenyukoracle commented Apr 10, 2025

openjdk bot commented Apr 10, 2025

openjdk bot commented Apr 10, 2025

magicus Apr 10, 2025

Choose a reason for hiding this comment

magicus Apr 10, 2025

Choose a reason for hiding this comment

alexeysemenyukoracle Apr 10, 2025

Choose a reason for hiding this comment

alexeysemenyukoracle Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

magicus Apr 11, 2025

Choose a reason for hiding this comment

alexeysemenyukoracle commented Mar 5, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 5, 2025 •

edited

Loading

mlbridge bot commented Mar 6, 2025 •

edited

Loading

alexeysemenyukoracle Apr 10, 2025 •

edited

Loading