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

Use separate nodeProjectDir for each subproject #2680

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

tylerbertrand
Copy link
Contributor

@tylerbertrand tylerbertrand commented Aug 29, 2024

Description

Sometimes, NpmTasks are failing with being unable to find files/commands under npmProjectDir. Here's an example of such a failure. The failing tasks always execute after the nodeSetup task for that project, so in theory, everything should be in place.

However, in this case, multiple subprojects share the same npmProjectDir. The way the gradle-node-plugin works, having multiple projects use the same npmProjectDir doesn't save work, and can actually cause timing issues like the failures we are seeing. This is because each nodeSetup task clears out the old directory before it unpacks it again. In the Timeline view of the same Build Scan linked above, we can see that the :solr:solr-ref-guide:downloadAntoraCli task (an NpmTask), is executing at the same time as :solr:packaging:nodeSetup, and downloadAntoraCli can't find node because it's been cleared out by another suproject's nodeSetup task.

Solution

This PR updates each subproject to use its own individual nodeProjectDir to avoid these timing issues. Separating these out doesn't result in node being downladed twice - despite unpacking the same version multiple times, it's only ever downloaded once. So no extra work is being done, since sharing the same nodeProjectDir doesn't save work in the first place, as explained above.

Tests

No new tests were added, and no existing tests were modified, but the check and integrationTests tasks consistently pass when run locally with these changes.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

It seems to make sense, I wish I knew more to weigh in one way or the other, hopefully someone else gives a +1!

Copy link

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

@github-actions github-actions bot added the stale PR not updated in 60 days label Oct 30, 2024
Copy link

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.

@github-actions github-actions bot added the closed-stale Closed after being stale for 60 days label Dec 29, 2024
@github-actions github-actions bot closed this Dec 29, 2024
Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

I see no reason for not doing this? Fixes a bug..

@janhoy janhoy reopened this Jan 3, 2025
@janhoy janhoy removed stale PR not updated in 60 days closed-stale Closed after being stale for 60 days labels Jan 3, 2025
@janhoy
Copy link
Contributor

janhoy commented Jan 3, 2025

@tylerbertrand Will you resolve the conflicts, then me or @epugh will get it merged?

@epugh
Copy link
Contributor

epugh commented Jan 3, 2025

thanks for reminding us @janhoy

@tylerbertrand
Copy link
Contributor Author

@janhoy Sure, sounds good

Using a shared nodeProjectDir caused multiple nodeSetup tasks to step on each others' toes. The com.github.node-gradle.node plugin doesn't fully support reusing nodeProjectDir, and each nodeSetup task will clean up the existing nodeProjectDir before unpacking the node installation into it. This can result in tasks seeing issues when trying to use npm/node while a nodeSetup task from another project is cleaning it up.

use nodeProjectDir instead of rootNodeDir
@tylerbertrand tylerbertrand force-pushed the tylerbertrand/node-fixes branch from 3043002 to 760348e Compare January 3, 2025 22:10
@tylerbertrand
Copy link
Contributor Author

Hi @janhoy and @epugh, the conflicts have been resolved

@epugh epugh merged commit 388101f into apache:main Jan 8, 2025
2 of 3 checks passed
epugh pushed a commit that referenced this pull request Jan 8, 2025
Using a shared nodeProjectDir caused multiple nodeSetup tasks to step on each others' toes. The com.github.node-gradle.node plugin doesn't fully support reusing nodeProjectDir, and each nodeSetup task will clean up the existing nodeProjectDir before unpacking the node installation into it. This can result in tasks seeing issues when trying to use npm/node while a nodeSetup task from another project is cleaning it up.

use nodeProjectDir instead of rootNodeDir

---------

Co-authored-by: Eric Pugh <[email protected]>
(cherry picked from commit 388101f)
@epugh
Copy link
Contributor

epugh commented Jan 8, 2025

Thanks for the contribution @tylerbertrand!

@dsmiley
Copy link
Contributor

dsmiley commented Jan 9, 2025

I'm going to move the CHANGES.txt entry from "Bug fix" into the "Other" category because Solr itself does not have a bug.

Also note the Lucene project uses "GITHUB#1234" pattern; we might want to do similarly. It's more future-proof as some references are to issues not PRs specifically. I'm making this change as well.

@epugh
Copy link
Contributor

epugh commented Jan 9, 2025

Thanks @dsmiley ... I guess I was thinking as developer of Solr versus a user of Solr! And Changes.txt is meant for the user primarily!

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

Successfully merging this pull request may close these issues.

4 participants