Add ability to use npm and node executables installed by node-gradle-plugin

@jnels124

When using the node-gradle-plugin to install the node/npm binaries, we have a timing issue:

The node-gradle-plugin installs missing node/npm binaries in the execution phase.
Spotless tries to run npm install (for preparing formatting servers) in the configuration phase.

Given that scenario, spotless will always fail in a clean setup, when a user tries to configure spotless to use the binaries installed by node-gradle-plugin (though it might succeed in a dirty state, when installation of the binaries has already happened in a previous gradle run).

Snippet that demonstrates the problem:

plugins {
    id 'com.diffplug.spotless'
    id 'com.github.node-gradle.node' version '3.5.1'
}
repositories { mavenCentral() }
node {
    download = true
    version = '18.13.0'
    workDir = file("${buildDir}/nodejs")
}
spotless {
    typescript {
        target 'app/**/*.ts'
        prettier()
            .nodeExecutable("${tasks.named('npmSetup').get().npmDir.get()}/bin/npm")
    }
}
tasks.named('spotlessTypescript').configure {
    it.dependsOn('nodeSetup', 'npmSetup')
}

Calling gradlew spotlessTypesriptApply will fail with something along the lines:

* Exception is:
org.gradle.api.internal.tasks.TaskDependencyResolveException: Could not determine the dependencies of task ':spotlessApply'.
[...]
Caused by: org.gradle.api.internal.tasks.DefaultTaskContainer$TaskCreationException: Could not create task ':spotlessMytypescriptApply'.
	at org.gradle.api.internal.tasks.DefaultTaskContainer.taskCreationException(DefaultTaskContainer.java:720)
[...]
Caused by: com.diffplug.spotless.npm.NpmProcess$NpmProcessException: Failed to launch npm command 'npm install --no-audit --no-package-lock --no-fund --prefer-offline'.
[...]
Caused by: java.io.IOException: Cannot run program "/private/var/folders/mb/xcsw5ld95cxbm8m310v0lpn0000141/T/junit7221955071881174664/build/npm/npm-v8.19.2/bin/npm" (in directory "/private/var/folders/mb/xcsw5ld95cxbm8m310v0lpn0000141/T/junit7221955071881174664/build/spotless-node-modules-prettier-format"): error=2, No such file or directory

This issue is to discuss the situation and possible solutions.

(Thanks @jnels124 for bringing this up in #1381 )

add tests that verifies issue diffplug#1499

@nedtwigg

To move npm usage from configuration-phase to execution phase, we would need to move the runNpmInstall() call from the prepareNodeServer() function into the createFormatterFunc() or the npmRunServer() steps.

Current prepareNodeServer impl:

spotless/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java

Lines 72 to 88 in 9df1809

    
           private NodeServerLayout prepareNodeServer(File buildDir) throws IOException { 
        
           	NodeServerLayout layout = new NodeServerLayout(buildDir, stepName); 
        
           	NpmResourceHelper.assertDirectoryExists(layout.nodeModulesDir()); 
        
           	NpmResourceHelper.writeUtf8StringToFile(layout.packageJsonFile(), 
        
           			this.npmConfig.getPackageJsonContent()); 
        
           	NpmResourceHelper 
        
           			.writeUtf8StringToFile(layout.serveJsFile(), this.npmConfig.getServeScriptContent()); 
        
           	if (this.npmConfig.getNpmrcContent() != null) { 
        
           		NpmResourceHelper.writeUtf8StringToFile(layout.npmrcFile(), this.npmConfig.getNpmrcContent()); 
        
           	} else { 
        
           		NpmResourceHelper.deleteFileIfExists(layout.npmrcFile()); 
        
           	} 
        
           	FormattedPrinter.SYSOUT.print("running npm install"); 
        
           	runNpmInstall(layout.nodeModulesDir()); 
        
           	FormattedPrinter.SYSOUT.print("npm install finished"); 
        
           	return layout; 
        
           }

Moving the runNpmInstall()-call into the npmRunServer() would be the best option IMHO. Maybe that would go together with #1480 ?

We would need to find a solution that does integrate nicely with the current state caching mechanisms. Any hints or pointers for that @nedtwigg ?

add tests that verifies issue diffplug#1499

The function of State is up-to-date checks and buildcache (local and remote). Right now, that State is based on the content of the package.json:

spotless/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java

Line 46 in bba3ea1

private final FileSignature packageJsonSignature;

So moving runNpmInstall() from State calculation to createFormatterFunc should work and doesn't affect the contract we're offering right now. So I'm fine with the change you have proposed.

Comparing this to the rest of Gradle, if you want a jar file you put it into a "configuration", and those get resolved at configuration time, not task execution time. I think the node-gradle plugin ought to provide a way to get a FileCollection which, when resolved, triggers npm install. That would be the the Gradle-y way to do it, and if it were implemented, we wouldn't need to make the change you're proposing.

If the goal is integration with the node-gradle-plugin as it exists, I think the change you have proposed is a good idea. If the goal is integration with the npm ecosystem, I think it would be better to improve node-gradle-plugin and leave the Spotless integration as-is.

Comparing this to the rest of Gradle, if you want a jar file you put it into a "configuration", and those get resolved at configuration time, not task execution time. I think the node-gradle plugin ought to provide a way to get a FileCollection which, when resolved, triggers npm install. That would be the the Gradle-y way to do it, and if it were implemented, we wouldn't need to make the change you're proposing.

You are probably right there. This is the way it should be set up. We might as well try and raise an issue on the project.

If the goal is integration with the node-gradle-plugin as it exists, I think the change you have proposed is a good idea. If the goal is integration with the npm ecosystem, I think it would be better to improve node-gradle-plugin and leave the Spotless integration as-is.

I think the goal here (as I understood the previous issues raised e.g. #1381, #1185, #1486) is a smother integration between the two plugins. I think we might even ease the way for #1480 (when we delay the npm install call, we can more easily check if it is needed at all on a second/third run)

The slow operation is f(package.json) -> list of files. That operation ought to happen in some gradle-npm plugin, and the caching of it ought to happen there too. If it did, all sorts of stuff could build on top of it and be fast.

We could build such caching inside of Spotless, I'm happy to merge and release such a feature. As a maintainer, I'll merge and release anything that makes a project better for end-users. But when it comes to my time as a coder, I try to only spend time on systems that have the best design I can see. If I can see a better design, then I would rather spend my time persuading the upstream component or even forking it so that the design can be good. Spotless is actually such a case - it was originally an Eclipse formatter maintained by somebody else, I submitted a PR that refactored the plugin into apply + check so that I could run it on CI, but the original author didn't want that feature so I forked into Spotless.

As far as the npm stuff goes, I'm a maintainer not a coder, so I'm happy either way :) But imo the right thing is a better node plugin. If the existing one doesn't want to improve, Spotless can recommend integration with a fork.

Released in plugin-gradle 6.14.0 and plugin-maven 2.31.0.

Navigation Menu

Repository navigation

Add ability to use npm and node executables installed by node-gradle-plugin #1499

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	private NodeServerLayout prepareNodeServer(File buildDir) throws IOException {
	NodeServerLayout layout = new NodeServerLayout(buildDir, stepName);
	NpmResourceHelper.assertDirectoryExists(layout.nodeModulesDir());
	NpmResourceHelper.writeUtf8StringToFile(layout.packageJsonFile(),
	this.npmConfig.getPackageJsonContent());
	NpmResourceHelper
	.writeUtf8StringToFile(layout.serveJsFile(), this.npmConfig.getServeScriptContent());
	if (this.npmConfig.getNpmrcContent() != null) {
	NpmResourceHelper.writeUtf8StringToFile(layout.npmrcFile(), this.npmConfig.getNpmrcContent());
	} else {
	NpmResourceHelper.deleteFileIfExists(layout.npmrcFile());
	}
	FormattedPrinter.SYSOUT.print("running npm install");
	runNpmInstall(layout.nodeModulesDir());
	FormattedPrinter.SYSOUT.print("npm install finished");
	return layout;
	}

Add ability to use npm and node executables installed by node-gradle-plugin #1499

Description

Activity

simschla commented on Jan 18, 2023

nedtwigg commented on Jan 19, 2023

simschla commented on Jan 19, 2023

nedtwigg commented on Jan 19, 2023

nedtwigg commented on Jan 26, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions