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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

kyliau
Copy link
Contributor

@kyliau kyliau commented May 14, 2019 •
edited
Loading

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 added target: patch This PR is targeted for the next patch release area: bazel Issues related to the published `@angular/bazel` build rules labels May 14, 2019
@kyliau kyliau requested a review from a team as a code owner May 14, 2019 20:17
@ngbot ngbot bot modified the milestone: needsTriage May 14, 2019
@kyliau kyliau force-pushed the disable_sandbox branch 2 times, most recently from f0cb21a to 8c18603 Compare May 14, 2019 21:22
gregmagolan
gregmagolan reviewed May 14, 2019
View reviewed changes
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
Copy link
Contributor

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

@gregmagolan gregmagolan self-requested a review May 14, 2019 21:48
gregmagolan
gregmagolan approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@gregmagolan gregmagolan left a 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?

@kyliau
Copy link
Contributor Author

kyliau 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 kyliau added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 14, 2019
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label May 14, 2019
@kyliau
Copy link
Contributor Author

kyliau commented May 14, 2019

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

IgorMinar
IgorMinar approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a 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.
Copy link
Contributor

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!

@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 15, 2019
@kyliau
fix(bazel): Disable sandbox on Mac OS
3272437 
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
@jasonaden jasonaden removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 15, 2019
@kyliau
Copy link
Contributor Author

kyliau commented May 15, 2019

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

@kyliau kyliau removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 15, 2019
@jasonaden jasonaden closed this in b6b1aec May 16, 2019
jasonaden pushed a commit that referenced this pull request May 16, 2019
@kyliau @jasonaden
fix(bazel): Disable sandbox on Mac OS (#30460)
3de26a8 
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 pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@kyliau @BioPhoton
fix(bazel): Disable sandbox on Mac OS (angular#30460)
4a7afaa 
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
@iget-master iget-master mentioned this pull request Jul 10, 2019
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
@kyliau kyliau deleted the disable_sandbox branch October 16, 2019 17:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IgorMinar IgorMinar IgorMinar approved these changes

@gregmagolan gregmagolan gregmagolan approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kyliau @IgorMinar @gregmagolan @jasonaden @googlebot