Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix args parsing on Windows #609

Merged
merged 11 commits into from
Sep 4, 2024
Merged

Fix args parsing on Windows #609

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
90abd0b
fix buildArgs with whitespaces in path
houcine7 Jul 9, 2024
311c8ad
revert to use tempdir for testing
houcine7 Jul 9, 2024
19d8fde
fix failing tests
houcine7 Jul 9, 2024
4b8987b
refactor
houcine7 Jul 9, 2024
49b26ad
fix add missing build args
houcine7 Jul 10, 2024
988cfad
add workflow for whitespace home folders
houcine7 Jul 23, 2024
c87de63
change testing dir and use repo with whitespaces
houcine7 Jul 28, 2024
5a86516
split build arguments correcltly
houcine7 Aug 6, 2024
8fcddbf
Merge branch 'master' into fix-path-whitespaces-args
alvarosanchez Sep 4, 2024
2798688
Cleanup tests
alvarosanchez Sep 4, 2024
cdfcfa2
Unit test code
alvarosanchez Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion native-maven-plugin/src/main/java/org/graalvm/buildtools/maven/AbstractNativeImageMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,29 @@ protected List<String> getBuildArgs() throws MojoExecutionException {

if (buildArgs != null && !buildArgs.isEmpty()) {
for (String buildArg : buildArgs) {
cliArgs.addAll(Arrays.asList(buildArg.split("\\s+")));
if(buildArg.startsWith("\\Q") ||
buildArg.startsWith("-H:ConfigurationFileDirectories")) {
cliArgs.add(buildArg);
continue;
}
String[] args = buildArg.split("\\s+");
int i=0;
while(i < args.length) {
String a =args[i];
if (a.charAt(0) == System.getProperty("user.home").charAt(0)) {
houcine7 marked this conversation as resolved.
Show resolved Hide resolved
StringBuilder path = new StringBuilder(a);
i++;
while( i< args.length && args[i].toLowerCase().charAt(0) <= 'z' &&
houcine7 marked this conversation as resolved.
Show resolved Hide resolved
args[i].toLowerCase().charAt(0) >= 'a') {
path.append(" ").append(args[i]);
i++;
}
cliArgs.add(path.toString());
} else {
cliArgs.add(a);
i++;
}
}
}
}

Expand Down
29 changes: 27 additions & 2 deletions ...estFixtures/groovy/org/graalvm/buildtools/maven/AbstractGraalVMMavenFunctionalTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,15 @@ import org.eclipse.jetty.server.handler.ResourceHandler
import spock.lang.Specification
import spock.lang.TempDir

import java.nio.file.FileVisitResult
import java.nio.file.Files
import java.nio.file.Path
import java.nio.file.SimpleFileVisitor
import java.nio.file.StandardCopyOption
import java.nio.file.attribute.BasicFileAttributes

abstract class AbstractGraalVMMavenFunctionalTest extends Specification {
@TempDir
Copy link
Collaborator

@dnestoro dnestoro Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you have removed @TempDir annotation and then proceeded with deleting files manually in the cleanup function?

Copy link
Member Author

@houcine7 houcine7 Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my local when I work with Tempdir all tests are passing, because windows avoids tempDir path whitespace with a certain encoding (C:\Users\Lahoucine~\AppData\Temp) so no parsing errors will result if I have withespaces in my path.


Path testDirectory

Path testOrigin;
Expand All @@ -71,6 +74,13 @@ abstract class AbstractGraalVMMavenFunctionalTest extends Specification {
boolean IS_MAC = System.getProperty("os.name", "unknown").contains("Mac");

def setup() {
var home_dir = Path.of(System.getProperty("user.home"))
testDirectory = home_dir.resolve("tests")

if (Files.notExists(testDirectory)) {
Files.createDirectory(testDirectory)
}

executor = new IsolatedMavenExecutor(
new File(System.getProperty("java.executable")),
testDirectory.resolve("m2-home").toFile(),
Expand All @@ -79,6 +89,21 @@ abstract class AbstractGraalVMMavenFunctionalTest extends Specification {
}

def cleanup() {

//cleanup test directory and all it's sub directories
Files.walkFileTree(testDirectory, new SimpleFileVisitor<Path>() {
@Override
FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
Files.delete(file);
return FileVisitResult.CONTINUE;
}
@Override
FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
Files.delete(dir);
return FileVisitResult.CONTINUE;
}
});

if (server != null) {
server.stop()
server.destroy()
Expand Down Expand Up @@ -158,7 +183,7 @@ abstract class AbstractGraalVMMavenFunctionalTest extends Specification {
var resultingSystemProperties = [
"common.repo.uri": System.getProperty("common.repo.uri"),
"seed.repo.uri": System.getProperty("seed.repo.uri"),
"maven.repo.local": testDirectory.resolve("local-repo").toFile().absolutePath
"maven.repo.local": testDirectory.resolve("local repo").toFile().absolutePath
alvarosanchez marked this conversation as resolved.
Show resolved Hide resolved
]
resultingSystemProperties.putAll(systemProperties)

Expand Down
Loading