8349350: Unable to print using InputSlot and OutputBin print attributes at the same time by GennadiyKrivoshein · Pull Request #23457 · openjdk/jdk

GennadiyKrivoshein · 2025-02-05T07:03:26Z

Fix for https://bugs.openjdk.org/browse/JDK-8349350. It's impossible to use more that one print option.

Reason of the bug:
execCmd array uses one index per print flag, but 'OPTIONS' flag can use two indexes for the options.

Fix description:
make the size of the execCmd array dependent on the number of options.

Test:
new test PrintExecCmdOptionTest.java created to check execution with multiple options. (run on MacOS, Windows and linux x86_64)

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8349350: Unable to print using InputSlot and OutputBin print attributes at the same time (Bug - P4)

Reviewers

Prasanta Sadhukhan (@prsadhuk - Reviewer) Review applies to 9b35b93a
Phil Race (@prrace - Reviewer)
Alexander Zuev (@azuev-java - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23457

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-05T07:03:50Z

👋 Welcome back GennadiyKrivoshein! 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-02-05T07:04:14Z

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

8349350: Unable to print using InputSlot and OutputBin print attributes at the same time

Reviewed-by: prr, kizune, psadhukhan

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 196 new commits pushed to the master branch:

1d147cc: 8351484: Race condition in max stats in MonitorList::add
4412c07: 8351639: Improve debuggability of test/langtools/jdk/jshell/JdiHangingListenExecutionControlTest.java test
1dd9cf1: 8349099: java/awt/Headless/HeadlessMalfunctionTest.java fails on CI with Compilation error
64464ea: 8351673: Clean up a case of if (LockingMode == LM_LIGHTWEIGHT) in a legacy-only locking mode function
9a49418: 8345940: Migrate security-related resources from Java classes to properties files
e71f327: 8351045: ClassValue::remove cannot ensure computation observes up-to-date state
cef3693: 8351656: Problemlist gc/TestAllocHumongousFragment#generational
da2b4f0: 8351606: Use build_platform for graphviz dependency
030c85d: 8350106: [PPC] Avoid ticks_unknown_not_Java AsyncGetCallTrace() if JavaFrameAnchor::_last_Java_pc not set
0ad9a10: 8351146: JFR: JavaMonitorInflate event should default to no threshold and be disabled
... and 186 more: https://git.openjdk.org/jdk/compare/b17c0b63a15246967f7cb24ba6089f2ef13e900e...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 (@prsadhuk, @prrace, @azuev-java) 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).

openjdk · 2025-02-05T07:04:36Z

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

client

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 · 2025-02-05T16:42:48Z

Webrevs

prsadhuk · 2025-02-07T07:38:30Z

src/java.desktop/share/classes/sun/print/PSPrinterJob.java

        if (options != null && !options.isEmpty()) {
-            pFlags |= OPTIONS;
-            ncomps+=1;
+            optionArgs = options.trim().split("\\s+");


In what format the options are expected for this regex to work? Is it same for both linux and mac?

The format is space delimited values, each value is a "key=value".
There is no difference between Linux and Mac and there are no changes in the format of the options.

If it's a space delimited values, then can't we use "options.trim().split(" ")"
same as what we have been using below?

Sure, done.

prsadhuk · 2025-02-11T03:13:42Z

/reviewers 2 reviewer

openjdk · 2025-02-11T03:14:12Z

@prsadhuk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

azuev-java · 2025-02-11T21:43:53Z

src/java.desktop/share/classes/sun/print/PSPrinterJob.java

            }
-            if ((pFlags & OPTIONS) != 0) {
-                for (String option : options.trim().split(" ")) {
+            if (optionArgs != null) {


I see this block is the same in both branches of the if condition. Can we just move it outside the condition to keep it simpler?

Yes, these are the same blocks, thanks. I moved options after the if condition.

prrace

I notice that UnixPrintJob is even worse .. it would have the same problem except it doesn't even try to iterate over options, even though they can be specified !

I think it makes sense to fix that too.

prrace · 2025-03-05T21:53:13Z

test/jdk/javax/print/PrintExecCmdOptionTest.java

+        OutputBin outputBin = null;
+        MediaTray mediaTray = null;
+        for (PrintService ps : printServices) {
+            Media[] medias = (Media[]) ps.


It looks to me as if this loop needs to reset mediaTray = null.
Otherwise if printer 'N' has a mediaTray, printer 'N+1' will inherit it, even if it does not have one.

Thank you. Fixed.

prrace · 2025-03-05T22:05:51Z

src/java.desktop/share/classes/sun/print/PSPrinterJob.java

-            pFlags |= OPTIONS;
-            ncomps+=1;
+            optionArgs = options.trim().split(" ");
+            ncomps+=optionArgs.length;


I would have gone for the simpler one line fix of
ncomps+=options.trim().split(" ").length

Done. I tried to avoid using of the options.trim().split(" ") twice.

GennadiyKrivoshein · 2025-03-10T13:33:18Z

@prrace UnixPrintJob updated.
I found only one way to test UnixPrintJob and validate lpr command arguments - parse debug output of the print service and compare expected and actual options.

prrace · 2025-03-10T17:02:16Z

test/jdk/javax/print/UnixPrintJobOptionsTest.java

+ * @test
+ * @bug 8349350
+ * @key printer
+ * @summary lpr command syntax for options. lpr [ -o option[=value] ]


This test needs an @requires tag.

Sure, added @requires (os.family...

prrace · 2025-03-10T18:18:48Z

test/jdk/javax/print/UnixPrintJobOptionsTest.java

+                break;
+            }
+        }
+        if (!success) {


Maybe it would be more robust to wrap everything in a try block with a finally to restore the printstream.

prrace · 2025-03-11T22:43:32Z

test/jdk/javax/print/UnixPrintJobOptionsTest.java

+ * @summary lpr command syntax for options. lpr [ -o option[=value] ]
+ * @run main/manual/othervm -Dsun.print.ippdebug=true UnixPrintJobOptionsTest
+ */
+


You also made the test manual, yet it has no user interaction. It ought to be enough that you tagged it as a printer test. I'll approve, but maybe it could be reverted to automated ??

I notice that it actually prints - although it is a blank page.
I suppose it has to, in order to get the command line to be used.

GennadiyKrivoshein · 2025-03-12T07:19:31Z

/integrate

openjdk · 2025-03-12T07:20:37Z

@GennadiyKrivoshein
Your change (at version 4ff9384) is now ready to be sponsored by a Committer.

prrace · 2025-03-13T20:25:55Z

/sponsor

openjdk · 2025-03-13T20:27:13Z

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

aa047ee: 8319192: Remove javax.swing.plaf.synth.SynthLookAndFeel.load(URL url)
cd1be91: 8319055: JCMD should not buffer the whole output of commands
248c373: 8351266: JFR: -XX:StartFlightRecording:report-on-exit
03ef79c: 8346470: Improve WriteBarrier JMH to have old-to-young refs
b50fe9b: 8280818: Expand bug8033699.java to iterate over all LaFs
771e160: 8351323: Parameterize compiler and linker flags for iconv
0ff1c08: 8349984: (jdeps) jdeps can use String.repeat instead of String.replaceAll
37ec796: 8351500: G1: NUMA migrations cause crashes in region allocation
4e51a8c: 8307513: C2: intrinsify Math.max(long,long) and Math.min(long,long)
7e3bc81: 8351216: ZGC: Store NUMA node count
... and 223 more: https://git.openjdk.org/jdk/compare/b17c0b63a15246967f7cb24ba6089f2ef13e900e...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-13T20:27:21Z

@prrace @GennadiyKrivoshein Pushed as commit 3da5e3f.

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

GennadiyKrivoshein added 2 commits February 4, 2025 11:24

Fix ArrayIndexOutOfBoundsException at PSPrinterJob printExecCmd

c1fa526

use array for option args

0382507

openjdk bot added the client client-libs-dev@openjdk.org label Feb 5, 2025

GennadiyKrivoshein marked this pull request as ready for review February 5, 2025 16:38

openjdk bot added the rfr Pull request is ready for review label Feb 5, 2025

prsadhuk reviewed Feb 7, 2025

View reviewed changes

replace regexp s+ with space

9b35b93

prsadhuk approved these changes Feb 11, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 11, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Feb 11, 2025

azuev-java reviewed Feb 11, 2025

View reviewed changes

GennadiyKrivoshein and others added 2 commits February 13, 2025 13:35

remove code duplication

621bf2e

Merge branch 'openjdk:master' into print_options_idx_out_of_rng

291ed33

prrace reviewed Mar 5, 2025

View reviewed changes

GennadiyKrivoshein added 4 commits March 10, 2025 12:33

reset mediaTray to null for each PrintService

ddeee9e

pass print options in UnixPrintJob as [ -o option[=value] ]

3ea14d6

simplify changes

cb9fa25

align UnixPrintJob with PSPrinterJob

7316607

prrace reviewed Mar 10, 2025

View reviewed changes

restore original printstream

4ff9384

prrace reviewed Mar 11, 2025

View reviewed changes

prrace approved these changes Mar 11, 2025

View reviewed changes

azuev-java approved these changes Mar 11, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 11, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 12, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 13, 2025

openjdk bot closed this Mar 13, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 13, 2025

Conversation

GennadiyKrivoshein commented Feb 5, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 5, 2025

Uh oh!

openjdk bot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 5, 2025

Uh oh!

mlbridge bot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

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!

prsadhuk commented Feb 11, 2025

Uh oh!

openjdk bot commented Feb 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prrace left a comment

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!

GennadiyKrivoshein commented Mar 10, 2025

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!

GennadiyKrivoshein commented Mar 12, 2025

Uh oh!

openjdk bot commented Mar 12, 2025

Uh oh!

prrace commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

GennadiyKrivoshein commented Feb 5, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 5, 2025 •

edited

Loading

mlbridge bot commented Feb 5, 2025 •

edited

Loading