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

npmInstall up-to-date check broken with fastNpmInstall #310

Open
SebastianS90 opened this issue Apr 23, 2024 · 6 comments
Open

npmInstall up-to-date check broken with fastNpmInstall #310

SebastianS90 opened this issue Apr 23, 2024 · 6 comments
Milestone

Comments

@SebastianS90
Copy link

I stumbled on a weird issue with the following steps:

  1. Fresh git checkout, i.e. no node_modules directory present
  2. Run task npmInstall with the fastNpmInstall option enabled
  3. Run the clean task, i.e. remove the whole node_modules directory
  4. Run npmInstall again, it reports as up-to-date
  5. Other tasks fail because node_modules is missing

The problem is that in step 2 the file node_modules/.package-lock.json was not recorded as output because it did not yet exist at configuration time, only after task execution. So the task has no configured outputs and won't become out of date by removing the node_modules directory.

Proposed fix: When using fastNpmInstall, do not check for existance of the file at configuration time here, instead always return the file.

@deepy
Copy link
Member

deepy commented Apr 28, 2024

Weirdly enough I see the opposite behaviour:

// because of package-lock.json that was generated during the previous npm install execution
result.task(":npmInstall").outcome == TaskOutcome.SUCCESS

What npm version are you using?

@SebastianS90
Copy link
Author

SebastianS90 commented Apr 28, 2024

I use npm 10.5.2, installed from the Arch Linux repo.

The opposite behavior is also easy to explain:

  1. Fresh git checkout, i.e. no node_modules directory present
  2. Run task npmInstall. At configuration time the node_modules/.package-lock.json did not exist, so it is tracked as "null" in the task outputs, regardless of its contents after task execution.
  3. Run npmInstall again. At configuration time the node_modules/.package-lock.json file exists, so the up to date check compares it's contents to the previously tracked output "null" and decides that the task it not up to date because the output has changed. After the task completes, the file content is tracked as output.
  4. Run npmInstall again. At configuration time the node_modules/.package-lock.json file exists, so the up to date check compares it's contents to the contents tracked in step 3. It is the same, so the task is up to date (provided that no input has changed).

This scenario is not that severe, it just executes the task when it was not necessary. My scenario in the issue opening post however is rather annoying since it is not easy to convince Gradle to execute the task again when it thinks it is still up to date and it leads to a failing build.

The proposed fix (to not check for file existence at configuration time) should solve both scenarios.

@deepy
Copy link
Member

deepy commented Apr 28, 2024

The test itself turns out to be broken, which is a little easier to confirm outside a plane 😅
But this seems like it could lead to cache poisoning and that makes it a bit more severe, I'll need to fix the test before I can push a fix though. But I agree on the fix, it looks like I added that on accident

@deepy
Copy link
Member

deepy commented May 4, 2024

Do the tasks that use packages provided by npm install have a dependency on npmInstall?

I can only reproduce this with tasks that don't have the dependency and I can't really fix the output properly without dropping support for npm < 7 which I don't want to do just quite yet

@SebastianS90
Copy link
Author

Thank you so much for looking into it and composing an integration test. In my actual project I also have npmInstallCommand = 'ci' and if you add that to your test then it will fail.

The reason for your test to pass is that with the default npmInstallCommand = 'install' (and adding --info) we have in the second run:

Task ':npmInstall' is not up-to-date because:
  Output property 'packageLockFileAsOutput' has been added for task ':npmInstall'

Again, the if-exists semantics is kind of weird here. At configuration time for the first execution the file does not exist, so "null" is tracked as output after task execution even though the file has just been created by that task. At configuration time for the second execution the file exists, so it is part of the outputs, and when the second run reaches the up-to-date check it sees that this output property has somehow changed from "null" to the existing file, so the task is not up-to-date.

With npmInstallCommand = 'ci' that output is not tracked (which is correct since the file is not written when using ci).

Another way to make your test fail even with npmInstallCommand = 'install' is install - delete - install - delete - taskWithDependency:

--- a/src/test/groovy/com/github/gradle/node/npm/task/NpmInstall_integTest.groovy
+++ b/src/test/groovy/com/github/gradle/node/npm/task/NpmInstall_integTest.groovy
@@ -148,6 +148,19 @@ class NpmInstall_integTest extends AbstractIntegTest {
         then:
         result.task(':deleteNodeModules').outcome == TaskOutcome.SUCCESS
 
+        when:
+        result = build('npmInstall')
+
+        then:
+        result.task(":npmInstall").outcome == TaskOutcome.SUCCESS
+
+        when:
+        // when the node_modules is removed
+        result = build('deleteNodeModules')
+
+        then:
+        result.task(':deleteNodeModules').outcome == TaskOutcome.SUCCESS
+
         when:
         result = build('taskWithDependency')

@deepy
Copy link
Member

deepy commented Sep 27, 2024

I'm having some difficulties modeling this in a way that both install and ci work :-/
So next major is going to drop support for npm versions older than npm 7, which makes this trivial to implement

@deepy deepy added this to the 8.x milestone Sep 27, 2024
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

No branches or pull requests

2 participants