Skip to content

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

Closed
kyliau wants to merge 1 commit into
angular:masterfrom
kyliau:disable_sandbox
Closed

fix(bazel): Disable sandbox on Mac OS#30460
kyliau wants to merge 1 commit into
angular:masterfrom
kyliau:disable_sandbox

Conversation

@kyliau

@kyliau kyliau commented May 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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
Comment thread packages/bazel/src/builders/bazel.ts Outdated

@gregmagolan gregmagolan May 14, 2019

Copy link
Copy Markdown
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

@gregmagolan gregmagolan left a comment

Copy link
Copy Markdown
Contributor

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
kyliau force-pushed the disable_sandbox branch from 8c18603 to 2d9fbac Compare May 14, 2019 21:53
@kyliau

kyliau commented May 14, 2019

Copy link
Copy Markdown
Contributor Author

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

kyliau commented May 14, 2019

Copy link
Copy Markdown
Contributor Author

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

IgorMinar
IgorMinar approved these changes May 15, 2019
View reviewed changes

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread packages/bazel/src/builders/bazel.ts Outdated

@IgorMinar IgorMinar May 15, 2019

Copy link
Copy Markdown
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 Keen Yee Liau
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
@kyliau
kyliau force-pushed the disable_sandbox branch from 2d9fbac to 3272437 Compare May 15, 2019 21:22
@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

kyliau commented May 15, 2019

Copy link
Copy Markdown
Contributor Author

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

angular-automatic-lock-bot Bot commented Sep 15, 2019

Copy link
Copy Markdown

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

2 more reviewers
@IgorMinar IgorMinar IgorMinar approved these changes
@gregmagolan gregmagolan gregmagolan approved these changes
Reviewers whose approvals may not affect merge requirements

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.

5 participants

@kyliau @IgorMinar @gregmagolan @jasonaden @googlebot