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

fix(bazel): Disable sandbox on Mac OS #30460

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@kyliau
Copy link
Member

commented May 14, 2019

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?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@kyliau kyliau requested a review from angular/tools-bazel as a code owner May 14, 2019

@ngbot ngbot bot modified the milestone: needsTriage May 14, 2019

@googlebot googlebot added the cla: yes label May 14, 2019

@kyliau kyliau force-pushed the kyliau:disable_sandbox branch 2 times, most recently from f0cb21a to 8c18603 May 14, 2019

const srcContent = readFileSync(source, 'utf-8');
const destContent = `${srcContent}
# Disable sandbox on Mac OS for performance reason.
build --spawn_strategy=standalone

This comment has been minimized.

Copy link
@gregmagolan

gregmagolan May 14, 2019

Contributor

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

@gregmagolan gregmagolan self-requested a review May 14, 2019

@gregmagolan
Copy link
Contributor

left a comment

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?

@kyliau kyliau force-pushed the kyliau:disable_sandbox branch from 8c18603 to 2d9fbac May 14, 2019

@kyliau

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

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 standalone to local.

The code was tested manually on Mac for a hello world (ng new) application:
ng build with sandbox: 22.0 seconds
ng build without sandbox: 13.3 seconds

@kyliau

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Caretaker: PR is good to go, but CodeFresh is stalled.

@IgorMinar
Copy link
Member

left a comment

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. :)

@@ -133,6 +134,18 @@ function replaceYarnWithNpm(source: string, dest: string) {
writeFileSync(dest, destContent);
}

// Disable sandbox on Mac OS by setting spawn_strategy in .bazelrc.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar May 15, 2019

Member

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!

fix(bazel): Disable sandbox on Mac OS
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

@kyliau kyliau force-pushed the kyliau:disable_sandbox branch from 2d9fbac to 3272437 May 15, 2019

@kyliau

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Updated commit message, comments in code, and PR description with reason for disabling sandbox.

@jasonaden jasonaden closed this in b6b1aec May 16, 2019

jasonaden added a commit that referenced this pull request May 16, 2019

fix(bazel): Disable sandbox on Mac OS (#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 #30460

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

fix(bazel): Disable sandbox on Mac OS (angular#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.