Quoting for start arguments #10099

gregw · 2023-07-12T07:57:06Z

backport of #10090

probably should be merged first and then merged through 11 & 12 before applying #10090

backport of #10090 Signed-off-by: gregw <gregw@webtide.com>

Do not quote comma Signed-off-by: gregw <gregw@webtide.com>

sbordet

I like the idea of being minimal to quote, and only if necessary.

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

sbordet · 2023-07-12T09:10:30Z

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

-        if (arg != null)
+        if (commandLine.length() > 0)
+            commandLine.append(separator);
+        commandLine.append(option);


The logic to add option, name and value to commandLine and args seems different.
Should they not be paired and be equal?
E.g. above we always add option to commandLine, why not to args too?

We do. All three branches below add option to the args one way or another. The code is a bit convoluted because it is minimal. Let me make it more explicit even if it duplicates a few lines....

is that better?

sbordet · 2023-07-12T09:15:29Z

jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt

                     o  "java" - the JVM to run
                     o  "opts" - the JVM options (e.g. -D, -X and -XX flags)
                     o  "path" - the JVM class-path and/or the JPMS module-path
                     o  "main" - the main class to run
                     o  "args" - the arguments passed to the main class
+                     o  "multipart" - a fake part that causes the command to be


It's multiline! 😂

multiline is not necessary.

If you are working on a shell you can accomplish this output on any already released version of Jetty now.

$ java -jar /path/to/jetty-home/start.jar --dry-run | xargs printf "%s\n"

A useful technique when troubleshooting the behavior of our quoting on various shells too (as the string to arg conversion for that shell is performed through the pipe to xargs)

neat technique, but the multiline output is correctly slashed so it is still executable. Frequently in docker style processing it is necessary to modify the output of the dry-run in some way, and having it broken up into multiple lines will make scripts like sed simpler to apply

jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

Updates from review Signed-off-by: gregw <gregw@webtide.com>

joakime · 2023-07-12T12:59:14Z

This branch has broken jetty.sh handling of complex properties that was pointed out in Issue #9309

$ ~/code/jetty/jetty.project-10.0.x/jetty-home/target/jetty-home/bin/jetty.sh start
Starting Jetty: xargs: unmatched double quote; by default quotes are special to xargs unless you use the -0 option
2023-07-12 07:53:14.956:WARN :oejx.XmlConfiguration:main: Unable to execute XmlConfiguration
java.nio.file.NoSuchFileException: %s
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:218)
	at java.base/java.nio.file.Files.newByteChannel(Files.java:380)
	at java.base/java.nio.file.Files.newByteChannel(Files.java:432)
	at java.base/java.nio.file.spi.FileSystemProvider.newInputStream(FileSystemProvider.java:422)
	at java.base/java.nio.file.Files.newInputStream(Files.java:160)
	at org.eclipse.jetty.util.resource.PathResource.getInputStream(PathResource.java:456)
	at org.eclipse.jetty.xml.XmlConfiguration.<init>(XmlConfiguration.java:218)
	at org.eclipse.jetty.xml.XmlConfiguration.main(XmlConfiguration.java:1845)
Exception in thread "main" java.nio.file.NoSuchFileException: %s
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:218)
	at java.base/java.nio.file.Files.newByteChannel(Files.java:380)
	at java.base/java.nio.file.Files.newByteChannel(Files.java:432)
	at java.base/java.nio.file.spi.FileSystemProvider.newInputStream(FileSystemProvider.java:422)
	at java.base/java.nio.file.Files.newInputStream(Files.java:160)
	at org.eclipse.jetty.util.resource.PathResource.getInputStream(PathResource.java:456)
	at org.eclipse.jetty.xml.XmlConfiguration.<init>(XmlConfiguration.java:218)
	at org.eclipse.jetty.xml.XmlConfiguration.main(XmlConfiguration.java:1845)

Attached is the jetty.base to reproduce this.
requestlog-custom-base.tar.gz

joakime

Don't use Double Quotes if you want to support sh

Use "Strong Quoting" instead, which is supported across the board.
And then you have only 1 character to worry about to escape.

bash

[joakim@hyperion ~]$ java -jar ~/java/args-dump-1.1.jar 'Hello internal quotes' foo bar
args.length=3
args[0]: [Hello internal quotes]
args[1]: [foo]
args[2]: [bar]
[joakim@hyperion ~]$ java -jar ~/java/args-dump-1.1.jar 'Hello '\''internal'\'' quotes' foo bar
args.length=3
args[0]: [Hello 'internal' quotes]
args[1]: [foo]
args[2]: [bar]
[joakim@hyperion ~]$ java -jar ~/java/args-dump-1.1.jar 'Hello $USER env var' zed foo
args.length=3
args[0]: [Hello $USER env var]
args[1]: [zed]
args[2]: [foo]

sh

$ java -jar ~/java/args-dump-1.1.jar 'Hello internal quotes' foo bar
args.length=3
args[0]: [Hello internal quotes]
args[1]: [foo]
args[2]: [bar]
$ java -jar ~/java/args-dump-1.1.jar 'Hello '\''internal'\'' quotes' foo bar
args.length=3
args[0]: [Hello 'internal' quotes]
args[1]: [foo]
args[2]: [bar]
$ java -jar ~/java/args-dump-1.1.jar 'Hello $USER env var' zed foo
args.length=3
args[0]: [Hello $USER env var]
args[1]: [zed]
args[2]: [foo]

zsh

joakim@hyperion ~ % java -jar ~/java/args-dump-1.1.jar 'Hello internal quotes' foo bar
args.length=3
args[0]: [Hello internal quotes]
args[1]: [foo]
args[2]: [bar]
joakim@hyperion ~ % java -jar ~/java/args-dump-1.1.jar 'Hello '\''internal'\'' quotes' foo bar
args.length=3
args[0]: [Hello 'internal' quotes]
args[1]: [foo]
args[2]: [bar]
joakim@hyperion ~ % java -jar ~/java/args-dump-1.1.jar 'Hello $USER env var' zed foo
args.length=3
args[0]: [Hello $USER env var]
args[1]: [zed]
args[2]: [foo]

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

joakime · 2023-07-12T13:34:50Z

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

+     * Quote a string suitable for use with a command line shell using double quotes.
+     * <p>This method applies doubles quoting as described for the unix {@code sh} commands:
+     * Enclosing characters within double quotes preserves the literal meaning of all characters except
+     * dollarsign ($), backquote (`), and backslash (\).
+     * The backslash inside double quotes is historically weird, and serves
+     * to quote only the following characters: {@code $ ` " \ newline}.
+     * Otherwise, it remains literal.


Suggested change

* Quote a string suitable for use with a command line shell using double quotes.

* <p>This method applies doubles quoting as described for the unix {@code sh} commands:

* Enclosing characters within double quotes preserves the literal meaning of all characters except

* dollarsign ($), backquote (`), and backslash (\).

* The backslash inside double quotes is historically weird, and serves

* to quote only the following characters: {@code $ ` " \ newline}.

* Otherwise, it remains literal.

* Quote a string suitable for use with a command line shell using strong quotes {@code '}.

* <p>This method applies strong quoting as described for the unix {@code sh} commands:

* Enclosing characters within strong quotes preserves the literal meaning of all characters.

We don't want to use double quotes with sh / bash / zsh, we want to use strong quotes ', as that will not interpret the contents between the quotes.

Examples:

sh on ubuntu

[joakim@hyperion ~]$ which sh /usr/bin/sh [joakim@hyperion ~]$ ls -la /usr/bin/sh lrwxrwxrwx 1 root root 4 Mar 23 2022 /usr/bin/sh -> dash [joakim@hyperion ~]$ sh -i -l $ echo "Hello $USER" Hello joakim $ echo 'Hello $USER' Hello $USER

bash on ubuntu

[joakim@hyperion ~]$ echo "Hello $USER" Hello joakim [joakim@hyperion ~]$ echo 'Hello $USER' Hello $USER

zsh on ubuntu

[joakim@hyperion ~]$ zsh -i -l joakim@hyperion ~ % echo "Hello $USER" Hello joakim joakim@hyperion ~ % echo 'Hello $USER' Hello $USER

Using single quotes is ugly/complex because we have to escape literal single quotes outside of the scope of the quotes, so quoting something like my.property='foo bar' (where the single quotes are literal) will end up with the unreadable my.property=\''foo bar'\'

Double quotes a simpler and with 4 characters that need escaping are well enough defined and simple to support.

The example property my.property='foo bar' would, as if it appeared in start.d/my.ini would mean that the end user wants the single quotes in the value as the end result (meaning if they do properties.get("my.property") they would expect a String with those single-tick characters: "'foo bar'")

The end result would be for strong quotes ...

'-Dmy.property='\''foo bar'\'''

The entire argument is strong quoted, not just a section.

Example:

[joakim@hyperion ~]$ java -Dmy.property='foo bar' -XshowSettings:properties --version 2>&1 | grep my.prop my.property = foo bar [joakim@hyperion ~]$ java '-Dmy.property='\''foo bar'\''' -XshowSettings:properties --version 2>&1 | grep my.prop my.property = 'foo bar'

This Strong Quoting also eliminates ALL interpretation of the contents of the quoted string by the shell (on all shells)

From the GNU dash man page (what the standard sh is on Ubuntu)

Single Quotes
Enclosing characters in single quotes preserves the literal meaning
of all the characters (except single quotes, making it impossible
to put single-quotes in a single-quoted string).

Something that Double Quotes does not do, and escaping will need to be SUPER robust (things you are ignoring in this PR will fail on various shells) if we go that route.

Yes, I'm aware that dash only mentions a handful of characters for Double Quotes, the other shells have far more special characters when encountered within Double Quotes (a behavior that Strong Quoting doesn't have)

@joakime I think what you have been trying to say is: "xargs doesn't grok double quotes like y'd think". Once I worked that out, then OK let's go with single quotes. updated.

@joakime I think what you have been trying to say is: "xargs doesn't grok double quotes like y'd think". Once I worked that out, then OK let's go with single quotes. updated.

@joakime what I meant to say was... thanks for persisting and I understood the issue eventually.

@joakime I think what you have been trying to say is: "xargs doesn't grok double quotes like y'd think". Once I worked that out, then OK let's go with single quotes. updated.

It's not xargs doing that.
It is bash doing that, bash (and zsh) will evaluate on any redirection/pipe before redirection/pipe (unlike dash).
This was a design decision by bash to get around deficiencies in sh not allowing variables to be set on redirection/pipe.

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt

jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java

gregw · 2023-07-12T14:15:47Z

@joakime your custom requestlog base is broken. It contains the line:

jetty.requestlog.formatString=%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t "%r $JETTY_HOME`' %s %O "%{Referer}i" "%{User-Agent}i"

So the unmatched quotes are from your ini file!

If I take the OPs reproducer from #9309 and put it in my ini file as:

jetty.requestlog.formatString=%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t "%r" %s %O "%{Referer}i" "%{User-Agent}i"

then it starts fine and --dry-run=multiline reports:

/usr/lib/jvm/java-17-openjdk-amd64/bin/java \
  -Djava.io.tmpdir=/tmp \
  -Djetty.home=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home \
  -Djetty.base=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/base \
  --class-path \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/base/resources:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/slf4j-api-2.0.5.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/jetty-slf4j-impl-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-servlet-api-4.0.6.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-http-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-server-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-xml-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-util-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-io-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-security-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-servlet-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-webapp-10.0.16-SNAPSHOT.jar:/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/jetty-deploy-10.0.16-SNAPSHOT.jar \
  org.eclipse.jetty.xml.XmlConfiguration \
  jetty.base=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/base \
  jetty.base.uri=file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/base \
  jetty.home=/home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home \
  jetty.home.uri=file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home \
  jetty.requestlog.formatString="%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t \"%r\" %s %O \"%{Referer}i\" \"%{User-Agent}i\"" \
  jetty.webapp.addServerClasses=org.eclipse.jetty.logging.,file:///home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/lib/logging/,org.slf4j. \
  runtime.feature.alpn=true \
  slf4j.version=2.0.5 \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-bytebufferpool.xml \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-threadpool.xml \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty.xml \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-webapp.xml \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-deploy.xml \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-http.xml \
  /home/gregw/src/jetty-10.0.x/jetty-home/target/jetty-home/etc/jetty-requestlog.xml

This looks perfectly correct

gregw · 2023-07-12T14:40:52Z

I think we are posix shell compliant:

2.2.2 Single-Quotes
Enclosing characters in single-quotes ( '' ) shall preserve the literal value of each character within the single-quotes. A single-quote cannot occur within single-quotes.

2.2.3 Double-Quotes
Enclosing characters in double-quotes ( "" ) shall preserve the literal value of all characters within the double-quotes, with the exception of the characters backquote, <dollar-sign>, and <backslash>, as follows:

$
The <dollar-sign> shall retain its special meaning introducing parameter expansion (see [Parameter Expansion](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02)), a form of command substitution (see [Command Substitution](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03)), and arithmetic expansion (see [Arithmetic Expansion](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_04)).
The input characters within the quoted string that are also enclosed between "$(" and the matching ')' shall not be affected by the double-quotes, but rather shall define that command whose output replaces the "$(...)" when the word is expanded. The tokenizing rules in [Token Recognition](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_03), not including the alias substitutions in [Alias Substitution](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_03_01), shall be applied recursively to find the matching ')'.

Within the string of characters from an enclosed "${" to the matching '}', an even number of unescaped double-quotes or single-quotes, if any, shall occur. A preceding <backslash> character shall be used to escape a literal '{' or '}'. The rule in [Parameter Expansion](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02) shall be used to determine the matching '}'.

`
The backquote shall retain its special meaning introducing the other form of command substitution (see [Command Substitution](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03)). The portion of the quoted string from the initial backquote and the characters up to the next backquote that is not preceded by a <backslash>, having escape characters removed, defines that command whose output replaces "`...`" when the word is expanded. Either of the following cases produces undefined results:
A single-quoted or double-quoted string that begins, but does not end, within the "`...`" sequence

A "`...`" sequence that begins, but does not end, within the same double-quoted string

\
The <backslash> shall retain its special meaning as an escape character (see [Escape Character (Backslash)](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_01)) only when followed by one of the following characters when considered special:
$   `   "   \   <newline>

The application shall ensure that a double-quote is preceded by a <backslash> to be included within double-quotes. The parameter '@' has special meaning inside double-quotes and is described in [Special Parameters](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_05_02) .

gregw · 2023-07-12T14:42:22Z

I'll change our doco to say posix shell...

Updates from review Signed-off-by: gregw <gregw@webtide.com>

joakime · 2023-07-12T15:33:55Z

I'll change our doco to say posix shell...

The current released versions of dash are not POSIX shell compliant btw (but it's being worked on).

gregw · 2023-07-12T15:39:12Z

@joakime ultimately, both double quotes and single quotes are well documented in sh, bash, posix etc. We can use either, so long as we fully implement either spec.

I chose double quotes because I prefer the simplicity of just 5 characters needing escape within the quotes, whilst single quotes needs to have two mechanism: single quotes; escaped chars outside of quotes; and sometimes a combination. Ultimately for the few difficult cases we might have, double quotes result in a lot more readable and understandable output.

Ultimately, for our purposes single and double quotes are equivalent, as we can convert one to the other, since we never include any unexpanded parameters. Thus there is no compelling reason to change.

gregw · 2023-07-12T15:57:42Z

I'll change our doco to say posix shell...

The current released versions of dash are not POSIX shell compliant btw (but it's being worked on).

Then who knows how it implements single quotes? If they are not POSIX compliant, then neither quoting style is ok.

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

joakime · 2023-07-13T09:22:18Z

@joakime your custom requestlog base is broken. It contains the line:
jetty.requestlog.formatString=%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t "%r $JETTY_HOME`' %s %O "%{Referer}i" "%{User-Agent}i"

Unmatched quotes are a perfectly legitimate thing to appear in one of our INIs.
Almost anything can appear as a property value in our INI. (the only exception being control characters, as we don't have the same escaping rules as a Java properties file).

BTW, this example base is supported properly on PR #10100 now.

Revert to single quotes for the sake of xargs in jetty.sh Signed-off-by: gregw <gregw@webtide.com>

restored deprecated APIs Signed-off-by: gregw <gregw@webtide.com>

fixed JPMS args Signed-off-by: gregw <gregw@webtide.com>

options cleaned up Signed-off-by: gregw <gregw@webtide.com>

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java

Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

updates from review Signed-off-by: gregw <gregw@webtide.com>

sbordet

Small javadocs nits if you want to fix them, but LGTM.

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

joakime

Is it intentional that all --<value> arguments are passed to the JVM side of the --dry-run ?

Example:

start.d/foo.ini

--long-arg
--zed=zora

Those two will be present in --dry-run before the classpath.

If we have a directory with an equals sign, this is interpreted as a property, not an XML.

start.d/mode.ini

etc/mode=dev/example.xml

I think we check for equals before we check for endsWith(".xml")?

It's not possible to have an xml or lib with a directory with a backslash defined in an start.d/<name>.ini

Example:

etc/oh\no/example.xml
--lib=lib/rut\roh/main.jar

An argument that starts with an equals sign produces some odd output on --dry-run

start.d/zed.ini

=1234

results in the output

''=1234

If there is no key with the use of the = sign, that quoting is not necessary.

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

joakime · 2023-07-13T17:28:55Z

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java

@@ -208,6 +208,7 @@ public class StartArgs
    private boolean listConfig = false;
    private boolean version = false;
    private boolean dryRun = false;
+    private boolean multiLine = false;


This should be a separate concept, not as a "part" or "section" on --dry-run=<parts>

Consider instead a --dry-run-format=<type> where we have a few options.

sh - what we do now (and is the default format)

windows - an alternate mode for windows

newline - what this multiLine is doing

And in the future we might even be able to support the various Daemon tooling and produce JSON or XML of the args that those tools consume.

But how do you select which parts to output with this new option?

The existing --dry-run=<parts> still exists.
Select what you want from that.

How the parts are presented are impacted by the new arg.

default behavior: (as --dry-run-format=sh is default, that's what is used)

$ java -jar start.jar --dry-run=opts,path,main,args

or on windows

> java -jar start.jar --dry-run=opts,args --dry-run-format=windows

or for new lines

$ java -jar start.jar --dry-run=opts,path,main,args --dry-run-format=newline

and in the future

$ java -jar start.jar --dry-run=opts,path,main,args --dry-run-format=json

This is too complex and verbose for a minor feature

updates from review Signed-off-by: gregw <gregw@webtide.com>

gregw · 2023-07-13T21:14:43Z

These may well be issues, but they have nothing to do with quoting or --dry-run

Is it intentional that all --<value> arguments are passed to the JVM side of the --dry-run ?

Any unknown options are considered JVM options and are thus ordered as they are.

If we have a directory with an equals sign, this is interpreted as a property, not an XML.

Indeed, any arg with an = will be recognised as a property and it has been like this since the beginning.
If it every becomes an issue, then we can add an optional option called '--file" which makes the next arg always be handled as a file.

It's not possible to have an xml or lib with a directory with a backslash defined in an start.d/<name>.ini

Ditto. If this ever becomes an issue, then we can add an optional --xml option.
But I think having a backslash in a filename is asking for problems.

An argument that starts with an equals sign produces some odd output on --dry-run

Well perhaps we should make this an error, but it has not been a problem because there is no need for an arg starting with =

gregw · 2023-07-13T21:21:43Z

@sbordet I've run out of time to deal with this PR. Can you take over this PR to get it approved and merged 10,11, and to 12. The PR #10090 is out of date and I've not updated whilst waiting for this to be approved. It might be simpler to abandon that and just merge this to 12 and fix up the env handling differences.

With regards to the multiline feature, I do not think this should be turned into anything as complex as has been suggested. It is just a simple little tool and we have no need to support json or non posix shells. If there is no agreement on it, then just remove it. We don't want to hold up progress over such a simple thing.

joakime · 2023-07-14T12:25:50Z

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java

+        // Single quotes are used because double quotes are processed differently by some shells and the xarg
+        // command used by jetty.sh


Suggested change

// Single quotes are used because double quotes are processed differently by some shells and the xarg

// command used by jetty.sh

// Single quotes are used because double quotes are evaluated differently by some shells such as bash

// which is used by jetty.sh and the Jetty docker image

* A couple more test cases. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

joakime

LGTM, lets merge this to jetty-12.0.x asap

Quoting for start arguments

68209de

backport of #10090 Signed-off-by: gregw <gregw@webtide.com>

gregw requested review from joakime and sbordet July 12, 2023 07:57

gregw mentioned this pull request Jul 12, 2023

Improved start.jar dry-run command line quoting #10090

Closed

Quoting for start arguments

a85ccf5

Do not quote comma Signed-off-by: gregw <gregw@webtide.com>

sbordet requested changes Jul 12, 2023

View reviewed changes

Quoting for start arguments

26af0c8

Updates from review Signed-off-by: gregw <gregw@webtide.com>

gregw requested a review from sbordet July 12, 2023 10:32

joakime requested changes Jul 12, 2023

View reviewed changes

Quoting for start arguments

d65ca77

Updates from review Signed-off-by: gregw <gregw@webtide.com>

gregw requested a review from joakime July 12, 2023 14:48

gregw mentioned this pull request Jul 13, 2023

Fix Quoting of arguments from Jetty start.jar --dry-run #10100

Closed

joakime reviewed Jul 13, 2023

View reviewed changes

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java Outdated Show resolved Hide resolved

gregw added 4 commits July 13, 2023 12:00

Quoting for start arguments

df91bfd

Revert to single quotes for the sake of xargs in jetty.sh Signed-off-by: gregw <gregw@webtide.com>

Quoting for start arguments

f4a2dea

restored deprecated APIs Signed-off-by: gregw <gregw@webtide.com>

Quoting for start arguments

ff67ade

fixed JPMS args Signed-off-by: gregw <gregw@webtide.com>

Quoting for start arguments

44c1fe2

options cleaned up Signed-off-by: gregw <gregw@webtide.com>

gregw commented Jul 13, 2023

View reviewed changes

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java Outdated Show resolved Hide resolved

gregw commented Jul 13, 2023

View reviewed changes

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java Outdated Show resolved Hide resolved

gregw commented Jul 13, 2023

View reviewed changes

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java Outdated Show resolved Hide resolved

sbordet requested changes Jul 13, 2023

View reviewed changes

gregw commented Jul 13, 2023

View reviewed changes

jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java Outdated Show resolved Hide resolved

gregw commented Jul 13, 2023

View reviewed changes

jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java Outdated Show resolved Hide resolved

gregw and others added 2 commits July 13, 2023 17:09

Apply suggestions from code review

845fbb1

Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

Quoting for start arguments

61b51ba

updates from review Signed-off-by: gregw <gregw@webtide.com>

gregw requested review from joakime and sbordet July 13, 2023 15:15

sbordet previously approved these changes Jul 13, 2023

View reviewed changes

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java Outdated Show resolved Hide resolved

jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java Outdated Show resolved Hide resolved

Apply suggestions from code review

ab25cd9

Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

gregw dismissed sbordet’s stale review via ab25cd9 July 13, 2023 15:40

gregw requested a review from sbordet July 13, 2023 15:55

sbordet previously approved these changes Jul 13, 2023

View reviewed changes

joakime requested changes Jul 13, 2023

View reviewed changes

Quoting for start arguments

6871906

updates from review Signed-off-by: gregw <gregw@webtide.com>

gregw dismissed sbordet’s stale review via 6871906 July 13, 2023 20:52

gregw requested review from joakime and sbordet July 13, 2023 21:16

gregw assigned sbordet Jul 13, 2023

joakime reviewed Jul 14, 2023

View reviewed changes

* Made "dry-run=multiline" hidden.

4ecf521

* A couple more test cases. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet approved these changes Jul 14, 2023

View reviewed changes

joakime approved these changes Jul 14, 2023

View reviewed changes

sbordet merged commit 11d2a32 into jetty-10.0.x Jul 14, 2023
2 of 3 checks passed

sbordet deleted the fix/jetty-10-start-quoting branch July 14, 2023 15:05

sbordet mentioned this pull request Jul 14, 2023

Deployment of static files does not work with --dry-run Jetty-12 #10055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quoting for start arguments #10099

Quoting for start arguments #10099

gregw commented Jul 12, 2023

sbordet left a comment

sbordet Jul 12, 2023

gregw Jul 12, 2023

gregw Jul 12, 2023

sbordet Jul 12, 2023

joakime Jul 13, 2023

gregw Jul 13, 2023 •

edited

joakime commented Jul 12, 2023 •

edited

joakime left a comment •

edited

joakime Jul 12, 2023

gregw Jul 12, 2023

joakime Jul 12, 2023 •

edited

gregw Jul 13, 2023

gregw Jul 13, 2023

joakime Jul 13, 2023 •

edited

gregw commented Jul 12, 2023

gregw commented Jul 12, 2023 •

edited

gregw commented Jul 12, 2023

joakime commented Jul 12, 2023

gregw commented Jul 12, 2023

gregw commented Jul 12, 2023

joakime commented Jul 13, 2023

sbordet left a comment

joakime left a comment

joakime Jul 13, 2023 •

edited

gregw Jul 13, 2023 •

edited

joakime Jul 13, 2023

gregw Jul 13, 2023

gregw commented Jul 13, 2023

gregw commented Jul 13, 2023

joakime Jul 14, 2023

joakime left a comment

		// Single quotes are used because double quotes are processed differently by some shells and the xarg
		// command used by jetty.sh

Quoting for start arguments #10099

Quoting for start arguments #10099

Conversation

gregw commented Jul 12, 2023

sbordet left a 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

gregw Jul 13, 2023 • edited

Choose a reason for hiding this comment

joakime commented Jul 12, 2023 • edited

joakime left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime Jul 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime Jul 13, 2023 • edited

Choose a reason for hiding this comment

gregw commented Jul 12, 2023

gregw commented Jul 12, 2023 • edited

gregw commented Jul 12, 2023

joakime commented Jul 12, 2023

gregw commented Jul 12, 2023

gregw commented Jul 12, 2023

joakime commented Jul 13, 2023

sbordet left a comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

joakime Jul 13, 2023 • edited

Choose a reason for hiding this comment

gregw Jul 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Jul 13, 2023

gregw commented Jul 13, 2023

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

gregw Jul 13, 2023 •

edited

joakime commented Jul 12, 2023 •

edited

joakime left a comment •

edited

joakime Jul 12, 2023 •

edited

joakime Jul 13, 2023 •

edited

gregw commented Jul 12, 2023 •

edited

joakime Jul 13, 2023 •

edited

gregw Jul 13, 2023 •

edited