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.

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

maven IDE hook for jj fix #2424

Open
victornoel opened this issue Feb 18, 2025 · 10 comments
Open

maven IDE hook for jj fix #2424

victornoel opened this issue Feb 18, 2025 · 10 comments

Comments

@victornoel
Copy link

Hi,

I am trying to setup jujutsu so that it runs spotless (configured using the maven plugin in my repository) when using jj fix, see https://jj-vcs.github.io/jj/latest/cli-reference/#jj-fix.

The problem is that the requirement from jujutsu is that the tool that does formatting should respect the following rule:

The external tools must accept the current file content on standard input, and return the updated file content on standard output. A tool's output will not be used unless it exits with a successful exit code. Output on standard error will be passed through to the terminal.

Would that be something that could be added to spotless?

Thanks!

@nedtwigg
Copy link
Member

Spotless Gradle supports this, but the Maven support is immature. @SapiensAnatis has made some progress here.

@nedtwigg nedtwigg changed the title Feature for piping files to spotless instead of in-place modification maven IDE hook for jj fix Feb 18, 2025
@SapiensAnatis
Copy link

I didn’t write the IDE hook to be clear, but it sounds broadly like what you are looking for.

If you use these arguments: https://github.com/SapiensAnatis/vscode-spotless-maven/blob/5cb5fa5a1cc444e47aa50e33547b2ffefdf56dec/src/maven/basicMavenExecutor.ts#L28

You will get a process that takes a document on stdin and then returns on stderr IS CLEAN / IS DIRTY and on stdout the formatted document.

The only problem is I can’t remember if you get the formatted document on stdout if Spotless returns IS CLEAN. In the vscode extension we just return ‘no changes’ if we get that back.

@victornoel
Copy link
Author

Hey @SapiensAnatis @nedtwigg thank you for the feedback, I will do a bit of experimentation with that to see how it behaves and come back here to share my findings!

@victornoel
Copy link
Author

I can already share this:

  • on "IS CLEAN", the file is not outputted indeed, that could be a problem for jj fix but I'm sure I will find a temporary workaround until we decide here if we should change the behaviour or not
  • on "IS DIRTY" it looks like it's doing exactly what I am expecting
  • in the long run there will also be the concern of performance
    • mvn is slow to startup, so for tens of file it's going to be long to run… well, it's always better than doing it by hand 😃
    • I'm not sure how this can be solved in practice: I mean, spotless already starts a separate npm process to be able to format multiple files faster, now will we need a separate mvn process to submit the files to format? 😅

@SapiensAnatis
Copy link

Performance wise I think the way forward is to use mvnd which runs as a daemon and is much faster for ad-hoc formatting. However it doesn’t seem to be compatible with the IDE hook at the minute, I’ve raised a few issues over there so hopefully those will be fixed soon!

@victornoel
Copy link
Author

victornoel commented Feb 20, 2025

I did a few tests with mvnd using both -DspotlessFiles and -DspotlessIdeHook (without stdin/out) and performances are still not great

It's taking something like 2.8sec with -DspotlessFiles and 3.8sec with -DspotlessIdeHook. The IDE Hook actually restarts the npm process every time ^^

running spotless:apply on ALL my files takes the same time btw, so the overhead is somewhere else, which means there is maybe some margin for improvement on the spotless side :)

@SapiensAnatis
Copy link

Oh the IDE hook is really only for a single file (hence IDE). On a single file it takes about 0.4s for me (assuming the daemon is warm) but if you are formatting your entire solution it will probably be slower.

@victornoel
Copy link
Author

@SapiensAnatis I meant a single file yes and it was clean and indexed so actually nothing was really done against the file and I let the daemon warm a bit indeed by calling the same command many times. In both cases (see below), most of the time is spent (from my subjective point of view) just after the line "[INFO] --- spotless:2.43.0:apply (default-cli) @ app ---".

Using the hook

JAVA_HOME=/usr/lib/jvm/java-23-openjdk mvnd -N spotless:apply -DspotlessIdeHook=$PWD/src/main/java/Test.java
[INFO] Processing build on daemon a98d8404
[INFO] Scanning for projects...
[INFO] BuildTimeEventSpy is registered.
[INFO] 
[INFO] Using the SmartBuilder implementation with a thread count of 11
[INFO] 
[INFO] ------------------------< my.app:app >------------------------
[INFO] Building Parent 1.0.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- spotless:2.43.0:apply (default-cli) @ app ---
[INFO] creating formatter function (starting server)
[INFO] [BEGIN] Starting npm based server in /home/victor/Codebases/app/target/spotless-prettier-node-modules-b900751a430598b577bf2148e1ea287e with StandardNpmProcessFactory.
[INFO] [END] Starting npm based server in /home/victor/Codebases/app/target/spotless-prettier-node-modules-b900751a430598b577bf2148e1ea287e with StandardNpmProcessFactory. (took 2ms)
[WARNING] [stderr] IS CLEAN
[INFO] Closing formatting function (ending server).
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.706 s (Wall Clock)
[INFO] Finished at: 2025-02-21T09:43:15+01:00
[INFO] ------------------------------------------------------------------------
java.lang.NullPointerException: Cannot invoke "org.mvndaemon.mvnd.logging.smart.LoggingOutputStream.forceFlush()" because "this.out" is null

and not the hook:

JAVA_HOME=/usr/lib/jvm/java-23-openjdk mvnd -N spotless:apply -DspotlessFiles=$PWD/src/main/java/Test.java
[INFO] Processing build on daemon a98d8404
[INFO] Scanning for projects...
[INFO] BuildTimeEventSpy is registered.
[INFO] 
[INFO] Using the SmartBuilder implementation with a thread count of 11
[INFO] 
[INFO] ------------------------< my.app:app >------------------------
[INFO] Building Parent 1.0.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- spotless:2.43.0:apply (default-cli) @ app ---
[INFO] Spotless.Java is keeping 1 files clean - 0 were changed to be clean, 0 were already clean, 1 were skipped because caching determined they were already clean
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.638 s (Wall Clock)
[INFO] Finished at: 2025-02-21T09:45:00+01:00
[INFO] ------------------------------------------------------------------------

@SapiensAnatis
Copy link

Ah, my numbers were from running on Java directly. I guess there may be some overhead if you need to invoke npm/prettier.

@victornoel
Copy link
Author

Ah, my numbers were from running on Java directly. I guess there may be some overhead if you need to invoke npm/prettier.

something to investigate, maybe in another ticket though ^^ I will try to get a feel of why it happens and create one if I feel like it's a good idea :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants