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

[rush] Fix an issue with rush install in large repos on Windows. #4701

Merged
merged 12 commits into from
May 14, 2024

Conversation

iclanton
Copy link
Member

Summary

This PR fixes an issue that was introduced in #4653.

Details

There is an issue where rush install and rush update will fail with an ENAMETOOLONG error on Windows in repos with a large number of projects. This is because Rush now calls pnpm install with --filter arguments for each project whose dependencies are being installed even if all projects are being installed.

This change updates the pnpm parameter generation algorithm to not generate any --filter arguments if all projects are being installed.
  

How it was tested

Tested in a repo that was failing with the error described above.

Impacted documentation

None.

@iclanton iclanton force-pushed the fix-rush-install branch 3 times, most recently from f1c8fae to 3bd7769 Compare May 14, 2024 07:03
@iclanton iclanton force-pushed the fix-rush-install branch 3 times, most recently from 4c2e66f to b7520b3 Compare May 14, 2024 08:40
@iclanton iclanton merged commit 236a0e5 into microsoft:main May 14, 2024
5 checks passed
@iclanton iclanton deleted the fix-rush-install branch May 14, 2024 09:24
shell: pwsh
run: |
mkdir -p $HOME/.rush-user
@{ buildCacheFolder = Join-Path ${{ github.workspace }} rush-cache } | ConvertTo-Json > $HOME/.rush-user/settings.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment explaining what this is doing? Not everybody is familiar with PowerShell particularly with the ${{ macro superimposed on its syntax.

Something like:

# Create a .rush-user/settings.json file containing a configuration such as this:
#
#   { "buildCacheFolder":  "/home/runner/work/rushstack/rushstack/rush-cache" }
#
# This is... ❓❓❓

Actually I have no idea what it is. .rush-user/settings.json is not documented anywhere on the website. Are users supposed to create it? Is this an internal implementation detail? What is this CI step doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did that never get documented? It's how users configured a machine-wide Rush cache location.


- name: Ensure repo README is up-to-date
run: node repo-scripts/repo-toolbox/lib/start.js readme --verify
working-directory: repo-a

- name: Clone another copy of the repo to test the build cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need another copy of the repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

During the second rush install run, the common/temp/<subspace>/pnpmfileSettings.json files have a property ("semverPath") that records where our generated pnpmfile can get the semver package. Running rush install on Rushstack with Rush in the current repo was causing an problem where the common/temp/default/node_modules folder is removed and recreated, so the "semverPath" points at a path that doesn't exist during the second subspace's installation.

Comment on lines 85 to 86
* Filters to be passed to PNPM during installation, if applicable.
* These restrict the scope of a workspace installation.
Copy link
Collaborator

@octogonz octogonz May 14, 2024

Choose a reason for hiding this comment

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

This description is too vague.

* An array of `--filter` arguments.  For example, if the array is ["a", "b"] then Rush would invoke
* `pnpm install --filter a --filter b` which restricts the install/update to dependencies of
* workspace projects "a" and "b".  If the array is empty or undefined, then an unfiltered install
* is performed.  Filtered installs have some limitations such as less comprehensive version analysis.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually just an array of arguments, including the --filter arguments.

iclanton added a commit to iclanton/rushstack that referenced this pull request May 14, 2024
*/
filteredProjects: RushConfigurationProject[];
selectedProjects: Set<RushConfigurationProject>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is selectedProjects actually needed any more? From what I can tell, it is only ever used by this code:

const selectedProjectNames: string[] = [];
for (const { packageName } of selectedProjects) {
selectedProjectNames.push(packageName);
}

...which just constructs a set of installed project names for last-install.flag to track. But isn't that exactly the same list as pnpmFilterArguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we aren't generating --filter args for every project anymore when doing a filtered install outside of subspace unless someone says rush install --only <something>. For the --to case, for example, we're generating args that use the pnpm filter syntax: https://pnpm.io/filtering

Comment on lines +158 to +159
for (const project of selectedProjects) {
const { subspace: projectSubspace } = project;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be using getSubspacesForProjects defined above to avoid duplicate processing of the metadata.

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

Successfully merging this pull request may close these issues.

None yet

4 participants