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.

Sign up for GitHub

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

Jump to bottom

[rush] Feature request for RUSH_TEMP_FOLDER support when pnpm useWorkspaces is true #4974

Open
timmydo opened this issue Oct 15, 2024 · 6 comments · May be fixed by #4987
Open

[rush] Feature request for RUSH_TEMP_FOLDER support when pnpm useWorkspaces is true #4974

timmydo opened this issue Oct 15, 2024 · 6 comments · May be fixed by #4987

Comments

@timmydo
Copy link

timmydo commented Oct 15, 2024

Summary

We're trying to reduce git clean time for our developers. We have a C# repo used by hundreds of developers that invokes rush build to build 80+ rush projects. Long story short, git clean is part of their workflow due to a full build with caching being faster than incremental rebuilds when switching branches.

Currently, git clean can take 5 to 10 minutes to run. From what I can tell, git spends most of the time enumerating files in common/temp.

rushstack/libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts

Line 86 in 681f718

if (EnvironmentConfiguration.rushTempFolderOverride !== undefined) {

Repro steps

The symlinks created point to the virtual store in common/temp, which is indistinguishable from regular files to git. Just enumerating the files takes a while.

$ time find . | wc -l
263979

real    0m19.698s
user    0m1.531s
sys     0m11.717s

Expected result:

I'd like to specify an environment variable when running rush commands or installing rush that basically has the effect of RUSH_TEMP_FOLDER. Then I would set this environment variable to a location outside of the git repository.
@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Oct 15, 2024
@iclanton
Copy link
Member

Do you want to just move the virtual store folder? We could probably include something similar to RUSH_PNPM_STORE_PATH for the virtual store. @D4N14L had some thoughts about this.

It'd require a fair bit of testing, though.

@iclanton iclanton moved this from Needs triage to Waiting for Author in Bug Triage Oct 21, 2024
@timmydo
Copy link
Author

timmydo commented Oct 21, 2024

I'm trying to get all written files by the build out of the git source directory. I'm using RUSH_PNPM_STORE_PATH successfully, so something similar to that would work.

@SquGus
Copy link

SquGus commented Oct 28, 2024

Do you want to just move the virtual store folder? We could probably include something similar to RUSH_PNPM_STORE_PATH for the virtual store. @D4N14L had some thoughts about this.

@D4N14L would you be able to share your thoughts here?

@D4N14L
Copy link
Member

D4N14L commented Oct 31, 2024 •
edited
Loading

@SquGus I mean I think this is a quick fix from my perspective. Should just be allowing us to modify the default virtual store directory that we use. We don't manually touch anything in the virtual store I don't believe, so it should just be specifying the override via env var and passing it through. We could possibly do a pre-release of the change to see if it works for what you need? @iclanton thoughts?

@D4N14L D4N14L linked a pull request Nov 1, 2024 that will close this issue
Draft
@SquGus
Copy link

SquGus commented Nov 6, 2024 •
edited
Loading

@D4N14L why is it that RUSH_TEMP_FOLDER is not compatible with workspace installs (as mentioned here)?

What @timmydo and I were hoping to do is to set an alternative location for the files placed in common/temp/, while still using pnpm workspaces.

While I wait for your response, I will experiment by removing the error thrown at WorkspaceInstallManager.prepareCommonTempAsync() to figure out if we can actually do this.

@D4N14L
Copy link
Member

D4N14L commented Nov 6, 2024

@SquGus The reason for this limitation is because the pnpm-lock.yaml file uses relative paths from its location on-disk in order to perform the install. Changing the location of the common/temp/ folder modifies the path it exists in during the install, and therefore would make the produced install fail. We can move the other paths (the store and virtual store directories) because these are just paths that get linked where they need to be linked by PNPM. But the pnpm-lock.yaml file would need to remain in-place.

@D4N14L D4N14L mentioned this issue Nov 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Bug Triage
Status: Waiting for Author
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[rush-lib] Add override support for the PNPM virtual store directory D4N14L/rushstack
4 participants
@timmydo @D4N14L @iclanton @SquGus