Quote filenames written in shell scripts #997
Conversation
…d by `JavaPathType` (because filenames specified in `java.util.spi.ToolProvider` API shall not be quoted), quotes are added by the plugin instead, and only when the target is a shell script. Opportunistically add a safety when path using only for debugging purposes cannot be relativized.
| } | ||
| final var commandLine = new StringBuilder("For trying to compile from the command-line, use:"); | ||
| Path dir = basedir; | ||
| if (dir != null) { // Should never be null, but it has been observed with some Maven versions. |
There was a problem hiding this comment.
I don't get this comment in together with the if statement.
The comment says that dir should never be null, but it was observed, so I would assume the if handles the one path and an else path the rare, but observed, path if it is. But I don't see an else to handle this?
There was a problem hiding this comment.
basedir is a field declared in this class as below:
@Parameter(defaultValue = "${project.basedir}", required = true, readonly = true)
protected Path basedir;A value should always be injected by Maven core. But during the development of this plugin, sometime when upgrading from a Maven version to the next one, or when using a snapshot version compiled from master, the value was not supplied anymore by Maven core. For example, a similar issue can be reproduced with apache/maven#11441 (I don't know if it was the same cause).
Even if a null basedir may be an indication of a serious problem that we want to be aware of, in this particular context, basedir is used only for making a path relative when creating a debug file. This code is executed only if the build failed (or was executed with --verbose), so we probably don't want the original cause to be hidden by a NullPointerException while writing debug information.
| @Override | ||
| public String[] option(Iterable<? extends Path> paths) { | ||
| var joiner = new StringJoiner(File.pathSeparator, (moduleName != null) ? moduleName + "=\"" : "\"", "\""); | ||
| var joiner = new StringJoiner(File.pathSeparator, (moduleName != null) ? moduleName + '=' : "", ""); |
There was a problem hiding this comment.
Here the quotes only got removed, but not replaced. Is this correct?
There was a problem hiding this comment.
Yes, it needs to be consistent with apache/maven#11435.
|
Thanks all for the reviews. |
3 participants
Following apache/maven#11435 (a change in Maven core for not quoting the files formatted by
JavaPathTypebecause filenames specified injava.util.spi.ToolProviderAPI shall not be quoted), quotes are added by the plugin instead, and only when the target is a shell script.Opportunistically add a safety when path using only for debugging purposes cannot be relativized.