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

Introduce EMBROIDER_WORKING_DIRECTORY env var for concurrent builds #2270

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

simonihmig
Copy link
Collaborator

Currently Embroider will write rewritten packages and other build meta data into node_modules/.embroider. That location is hardcoded. Usually not a problem, but when running more than one build for the same app concurrently, builds will step on each others toes by trying to write into the same dir. The optional environment variable introduced here will allow to specify that directory for each build to isolate them.

Fixes #2086

Currently Embroider will write rewritten packages and other build meta data into `node_modules/.embroider`. That location is hardcoded. Usually not a problem, but when running more than one build for the same app concurrently, builds will step on each others toes by trying to write into the same dir. The optional environment variable introduced here will allow to specify that directory for each build to isolate them.

Fixes #2086
@simonihmig
Copy link
Collaborator Author

simonihmig commented Feb 14, 2025

Do we want to document this somewhere, or is this more like a hidden feature? t seems no one than us had a need for this before...

Also no tests. I could add some unit tests for the locateEmbroiderWorkingDir, which does not have any yet. E2E testing the actual "feature" of concurrent builds in CI seems like too much overhead I think? I tested this locally, and it was solving our problem.

@simonihmig simonihmig requested a review from a team February 14, 2025 14:40
@SergeAstapov
Copy link
Collaborator

+1 for documenting this.

as for question "where to document this", maybe "Options" section in readme.md would work

@mansona
Copy link
Member

mansona commented Feb 16, 2025

I feel like this probably should have a test for it 🙈 it seems like a sensible change if we can guarantee that everything is going through this function that relies on that location, but verifying would make me feel more comfortable about that. Also it means that we would have a regression test to avoid breaking this in future

@ef4
Copy link
Contributor

ef4 commented Feb 17, 2025

I agree that code not using locateEmbroiderWorkingDir is a risk, but I don't think it's particularly practical to guard against with a test. For the test to help you need to remember that your example needs to work under the env var, and if you remember that you could just have used locateEmbroiderWorkingDir anyway.

I'm thinking specifically of template-tag-codemod, which does in fact currently have this bug and would be unlikely to be caught by any general test here.

@ef4 ef4 merged commit 40daea1 into main Feb 17, 2025
218 checks passed
@ef4 ef4 deleted the allow-concurrent-builds branch February 17, 2025 15:02
@github-actions github-actions bot mentioned this pull request Feb 17, 2025
@mansona mansona added the enhancement New feature or request label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow concurrent builds
4 participants