Skip to content

fix: use separator properties when running JMH #228

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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.

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.

2 participants