-
Notifications
You must be signed in to change notification settings - Fork 26.2k
fix(bazel): Disable sandbox on Mac OS #30460
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
Conversation
f0cb21a
to
8c18603
Compare
packages/bazel/src/builders/bazel.ts
Outdated
const srcContent = readFileSync(source, 'utf-8'); | ||
const destContent = `${srcContent} | ||
# Disable sandbox on Mac OS for performance reason. | ||
build --spawn_strategy=standalone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looks like standalone
is deprecated and the value to use is local
here: https://docs.bazel.build/versions/master/user-manual.html#strategy-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with comment regarding deprecated standalone
value; local
should be used. Have you tested manually that this has the desired effect on OSX for runfiles?
Changed The code was tested manually on Mac for a hello world (ng new) application: |
Caretaker: PR is good to go, but CodeFresh is stalled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but please update the commit message with info about "why" this change... and also expand the comment in the code itself. Always think about the future readers of this code - it might actually be the forgetful you. :)
packages/bazel/src/builders/bazel.ts
Outdated
@@ -133,6 +134,18 @@ function replaceYarnWithNpm(source: string, dest: string) { | |||
writeFileSync(dest, destContent); | |||
} | |||
|
|||
// Disable sandbox on Mac OS by setting spawn_strategy in .bazelrc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you capture why you want to disable this as well so that a future reader doesn't need guess or search for the reason. thanks!
Removing the sandbox improves build time by almost 40%. For a hello world (ng new) application: ng build with sandbox: 22.0 seconds ng build without sandbox: 13.3 seconds
Updated commit message, comments in code, and PR description with reason for disabling sandbox. |
Removing the sandbox improves build time by almost 40%. For a hello world (ng new) application: ng build with sandbox: 22.0 seconds ng build without sandbox: 13.3 seconds PR Close #30460
Removing the sandbox improves build time by almost 40%. For a hello world (ng new) application: ng build with sandbox: 22.0 seconds ng build without sandbox: 13.3 seconds PR Close angular#30460
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Removing the sandbox improves build time by almost 40%.
For a hello world (ng new) application:
ng build with sandbox: 22.0 seconds
ng build without sandbox: 13.3 seconds
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information