Skip to content

Conversation

@dwalluck
Copy link
Contributor

@dwalluck dwalluck commented Apr 2, 2025

This makes the build cross-platform and fixes execution on, e.g., Windows.

This makes the build cross-platform and fixes execution on, e.g.,
Windows.
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

Cool! 💯

Does it work even if the classpath contains spaces (e.g. `-Dmaven.local.repo=?

@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 3, 2025

Does it work even if the classpath contains spaces (e.g. `-Dmaven.local.repo=?

It does if I add double quotes to the -cp argument. I assume that this works in both Windows and Bash.

For example, I tried: mvn -Pbenchmark "-Dmaven.repo.local=My Temp" and it worked.

Note that I think Windows can handle some mix of file separators (both \ and /), but it's the java command itself which expects ; rather than : for the classpath, I think.

@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 3, 2025

Also, I tried to see if using exec:java rather than exec:exec would do some of this automatically, but I couldn't get that to work. We could probably get rid of the need to set the classpath if we created a shaded benchmarks.jar like they show in the JMH examples, but I guess this way is working.

@ppkarwasz
Copy link
Contributor

Also, I tried to see if using exec:java rather than exec:exec would do some of this automatically, but I couldn't get that to work.

I tried that too and there are some classpath problems in the JVMs forked by JMH. This might be due to the way JMH forks the JVM.
Probably this could be solved with some JVM-specific arguments (e.g. -jvmArgs).

We could probably get rid of the need to set the classpath if we created a shaded benchmarks.jar like they show in the JMH examples, but I guess this way is working.

I tried that too before. The Maven Shade Plugin always shades the main artifact and has an option to also shade the test jar. This is probably not what we want to do.

pom.xml Outdated
<executable>${java.home}/bin/java</executable>
<commandlineArgs>-cp target/classes:target/test-classes:${test.classpath} org.openjdk.jmh.Main ${jmh.args}</commandlineArgs>
<executable>${java.home}${file.separator}bin${file.separator}java</executable>
<commandlineArgs>-cp "target${file.separator}classes${path.separator}target${file.separator}test-classes${path.separator}${test.classpath}" org.openjdk.jmh.Main ${jmh.args}</commandlineArgs>
Copy link
Contributor

@ppkarwasz ppkarwasz Apr 4, 2025

Choose a reason for hiding this comment

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

This command line does not look very elegant, due to the fact that:

  • we want ${jmh.args} to be split into words.
  • we do not want the classpath to be split into words.

What would you say about passing the classpath in the CLASSPATH environment variable instead:

<environmentVariables>
  <CLASSPATH>target${file.separator}classes${path.separator}target${file.separator}test-classes${path.separator}${test.classpath}</CLASSPATH>
</environmentVariables>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this seems to work as well. I changed it.

But, I thought that the quotes were already preventing the splitting of the classpath. The JMH args were left unquoted, same as before.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, I thought that the quotes were already preventing the splitting of the classpath. The JMH args were left unquoted, same as before.

The quotes should work in most normal cases, although I suspect that using " in the path of the Maven local repository on UNIX might break the configuration (the plugin uses plexus-utils to split the command line).
Using the environment variable is probably just safer.

Copy link
Collaborator

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremylong jeremylong merged commit 1ca3d8f into package-url:master May 6, 2025
3 checks passed
@jeremylong
Copy link
Collaborator

@dwalluck and @ppkarwasz - sorry about the delay in getting back to these PRs. I'm going to continue working through them over the next few days.

@dwalluck dwalluck deleted the jmh-separators branch May 6, 2025 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants