8288838: jpackage: file association additional arguments

[akashche]

jpackage implementation of file association on Windows currently passes a selected filename as an only argument to associated executable.

It is proposed to introduce additional option in file association property file to allow optionally support additional arguments using %* batch wildcard.

Note, current implementation, while fully functional, is only a DRAFT one, it is not ready for integration in this form. I would appreciate any guidance on the following points:

• option naming inside a properties file, currently pass-all-args is used
• option naming in a bundler parameter implementation, it is not clear if it should introduce a new group of "file association windows specific options" next to the existing "file association mac specific options" group
• test organization to cover the new option: currently it is included inside FileAssociationTest and piggybacks on the existing (and unrelated) includeDescription parameter; it is not clear whether it should be done in a separate test and whether to include runs for every parameter combination
• test run implementation: currently arguments are checked when a file with associated extension is invoked from command line; it is not clear whether it would be more appropriate instead to create a desktop shortcut with the same command as a target and to invoke it with java.awt.Desktop

Also please note, that full install/uninstall run is currently enabled in FileAssociationTest, it is intended to be used only in a draft code during the development and to be removed (to use the same "install or unpack" logic as other tests) in a final version.

Testing:

• test to cover new logic is included
• ran jtreg:jdk/tools/jpackage with no new failures

---

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

Issue

• JDK-8288838: jpackage: file association additional arguments

Reviewers

• Alexey Semenyuk (@alexeysemenyukoracle - Reviewer)
• Alexander Matveev (@sashamatveev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9224

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

Using diff file

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

[bridgekeeper]

👋 Welcome back akasko! 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]

@akashche 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.

[mlbridge]

Webrevs

• 03: Full (51206df6)
• 02: Full - Incremental (7e14f614)
• 01: Full - Incremental (3b4f77e8)
• 00: Full (427ff849)

[alexeysemenyukoracle]

What would be the alternative to pass-all-args?
What if we unconditionally add %* batch wildcard?

[alexeysemenyukoracle]

Wondering if simple "%*" would be sufficient to preserve arguments with spaces.

[akashche]

I've added argument with spaces to the test, they seem to be passed correctly (with existing unquoted %*) with both command line invocation and with a shortcut.

[akashche]

What would be the alternative to pass-all-args?

As this is intended to be a public property, I assume there should be a CSR and some consistent name, perhaps with win- prefix - I assume we will need an additional similar property for a verb label displayed in a context menu. pass-all-args was intended only as a working title.

What if we unconditionally add %* batch wildcard?

Can we change the existing default behaviour? Should we? It won't break existing usage, but is it actually always desired to support passing arbitrary arguments from the desktop shortcut to the target java app? I myself cannot answer any of these questions.

[alexeysemenyukoracle]

I assume we will need an additional similar property for a verb label displayed in a context menu.

Agree. At least we need to provide l10n for it.

Can we change the existing default behaviour? Should we? It won't break existing usage, but is it actually always desired to support passing arbitrary arguments from the desktop shortcut to the target java app? I myself cannot answer any of these questions.

The existing behavior simply cuts off all, but the first argument from the argument array passed to the app launcher. Changing this looks more like a fix of a bug, than an enhancement.
I'd keep it simple: replace the value of the Argument attribute from %1 to %*. Besides, "%1 %*" seems to duplicate the first argument (%1 is not referencing a path to app launcher executable, but the name of a file to open).
If fixed in this way, we don't need to introduce pass-all-args.

[akashche]

I assume we will need an additional similar property for a verb label displayed in a context menu.

Agree. At least we need to provide l10n for it.

For runtime installers it would be great to have a custom verb label, to be able to use something like: "Open with VendorName OpenJDK". I intend to prototype this and file in a separate PR (there is a related point about the "ampersand for keyboard shortcut" in this label, that is currently unclear).

[akashche]

The existing behavior simply cuts off all, but the first argument from the argument array passed to the app launcher. Changing this looks more like a fix of a bug, than an enhancement. I'd keep it simple: replace the value of the Argument attribute from %1 to %*. Besides, "%1 %*" seems to duplicate the first argument (%1 is not referencing a path to app launcher executable, but the name of a file to open). If fixed in this way, we don't need to introduce pass-all-args.

I've experimented with this, used following example app:

• hello.jar - java application
• hello.exe - launcher created by jpackage
• hello.foo - file with a .foo extension associated with hello.exe

Arguments placeholders specified In a progid[...]\shell\open\command registry entry are received in java app in a following form:

• %0 points to c:\path\too\hello.foo, that is a selected file path (for context menu invocation) or a shortcut.TargetPath property (for LNK shortcut invocation)
• %1 is the same as %0
• %2 points to the first argument in shortcut.Arguments property
• %* contains all the arguments beginning from %2

%1 must be quoted, otherwise it only contains the part of path before the first space and other path parts are passed as %2 and following arguments.

%* must not be quoted, it passes arguments with spaces correctly.

Based on the above I've updated the patch dropping the property and changing the Argument to "%1" %*.

I would appreciate the guidance on the test changes (note, packageTest.forTypes and packageTest.run changes are temporary). Foremost, should we cover all three ways to invoke the associated file (command line, desktop open associated file, desktop open LNK) and (if yes) should they be split into separate test runs?

[alexeysemenyukoracle]

For runtime installers it would be great to have a custom verb label, to be able to use something like: "Open with VendorName OpenJDK". I intend to prototype this and file in a separate PR

Sounds good.

%* contains all the arguments beginning from %2

Oh, interesting. I didn't know that! Thank you for providing a detailed explanation.

I would appreciate the guidance on the test changes

I put proposed changes to test classes in #9331 PR. I hope the approach gives enough flexibility to configure testing of fa. We can configure all possible test configurations in FileAssociationTest test and have basic testing of fa in tests where it is not primary focus. Code in #9331 is a sketch, it compiles, but I didn't run it.

[openjdk-notifier]

@akashche Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[akashche]

I've rebased the changes on top of PR 9331 and rebased the result on top of recent master (to have JDK-8288961 to be able to run MSI test). I think the patch if ready for review now.

Note on non-ASCII arguments support: I've found that Windows system locale needs to be changed to support specific language. Checked manually that such args are passed successfully with both command-line and LNK shortcut arguments. Left a note on this in test.

[alexeysemenyukoracle]

Looks good! I'll try the patch locally and get back to you.
Please provide a detailed test scenario for non-ASCII args. We can add this scenario to manual test runs done by SQE.

[akashche]

About the non-ASCII args: in FileAssociations.TestRun::openFiles non-ASCII argument can be supplied along with other arguments (in WinCommandLine and WinDesktopOpenShortcut runs). The arguments passing and the check in the output file works correctly for such arguments, but only when the Language for non-Unicode programs system setting in Windows is set to the corresponding language. Otherwise the argument is received by java app in ????????? form and the test expectedly fails. I've checked that this behaviour of jpackage launcher is the same as in the main java.exe launcher. I've added the note comment and the concrete example of an arg to FileAssociations.TestRun::openFiles.

If it is desired to be able to run such non-ASCII test manually (without changing the test code), I assume this can be done by making the test to read arguments from filesystem, or to add some kind of a test option to run the branch with predefined non-ASCII arguments only when requested.

[alexeysemenyukoracle]

I confirm the patch works and there are no regressions.

SQE uses only the part of jpackage jtreg tests that create test packages, so there is no point in customizing tests with reading args from the file system. I think I have enough information to help SQE update test spec and cover Unicode args.

[alexeysemenyukoracle]

FYI: the default test steps can be overridden with the value of jpackage.test.action system property.
Its value would be create,install,verify_install,uninstall for this case.

UPD: Nevermind this comment. It applied to the old version of the PR.

[akashche]

Thanks for the info! I've seen jpackage.test.action property in run_tests.sh, but never used it.

[openjdk]

@akashche 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:

8288838: jpackage: file association additional arguments

Reviewed-by: asemenyuk, 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 46 new commits pushed to the master branch:

• 569de45: 8289620: gtest/MetaspaceUtilsGtests.java failed with unexpected stats values
• 403a9bc: 8289436: Make the redefine timer statistics more accurate
• a40c17b: 8289775: Update java.lang.invoke.MethodHandle[s] to use snippets
• 2a6ec88: Merge
• 0526402: 8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc
• b3a0e48: 8289439: Clarify relationship between ThreadStart/ThreadEnd and can_support_virtual_threads capability
• 0b6fd48: 8288128: S390X: Fix crashes after JDK-8284161 (Virtual Threads)
• 30e134e: 8289091: move oop safety check from SharedRuntime::get_java_tid() to JavaThread::threadObj()
• 29ea642: 8287847: Fatal Error when suspending virtual thread after it has terminated
• f640fc5: 8067757: Incorrect HTML generation for copied javadoc with multiple @throws tags
• ... and 36 more: https://git.openjdk.org/jdk/compare/649f2d8835027128c6c8cf37236808094a12a35f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@alexeysemenyukoracle, @sashamatveev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[alexeysemenyukoracle]

/sponsor

[openjdk]

@alexeysemenyukoracle The change author (@akashche) must issue an integrate command before the integration can be sponsored.

[akashche]

/integrate

[akashche]

Thanks for the reviews!

[openjdk]

@akashche
Your change (at version 51206df) is now ready to be sponsored by a Committer.

[alexeysemenyukoracle]

/sponsor

[openjdk]

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

• 95e3190: 8210708: Use single mark bitmap in G1
• 8e7b45b: 8282986: Remove "system" in boot class path names
• 74ca6ca: 8289800: G1: G1CollectionSet::finalize_young_part clears survivor list too early
• 86f63f9: 8289164: Convert ResolutionErrorTable to use ResourceHashtable
• 013a5ee: 8137280: Remove eager reclaim of humongous controls
• 77ad998: 8289778: ZGC: incorrect use of os::free() for mountpoint string handling after JDK-8289633
• 532a6ec: 7124313: [macosx] Swing Popups should overlap taskbar
• e05b2f2: 8289856: [PPC64] SIGSEGV in C2Compiler::init_c2_runtime() after JDK-8289060
• cce77a7: 8289799: Build warning in methodData.cpp memset zero-length parameter
• d1249aa: 8198668: MemoryPoolMBean/isUsageThresholdExceeded/isexceeded001/TestDescription.java still failing
• ... and 47 more: https://git.openjdk.org/jdk/compare/649f2d8835027128c6c8cf37236808094a12a35f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@alexeysemenyukoracle @akashche Pushed as commit a694e9e.

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