build: use cross platform workspace_status_command #27431

filipesilva · 2018-12-03T19:46:14Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

The workspace_status_command is a .sh script and thus does not properly run on windows.

What is the new behavior?

The command is a node script instead. This is the workaround suggested in bazelbuild/bazel#4802.

Does this PR introduce a breaking change?

Yes
No

Other information

cc @alexeagle

alexeagle · 2018-12-04T20:08:04Z

tools/bazel_stamp_vars.js

+// See https://github.com/bazelbuild/bazel/issues/5958
+// Note: git operations, especially git status, take a long time inside mounted docker volumes
+// in Windows or OSX hosts (https://github.com/docker/for-win/issues/188).
+// To bypass this script use `--workspace_status_command=`, in the command line or in a rc file.


can remove this line, the workspace_status_command is opt-in now

alexeagle · 2018-12-04T20:10:10Z

tools/bazel_stamp_vars.js

+// in Windows or OSX hosts (https://github.com/docker/for-win/issues/188).
+// To bypass this script use `--workspace_status_command=`, in the command line or in a rc file.
+const execSync = require('child_process').execSync;
+const _exec = (str => execSync(str).toString().trim());


nit: style guide in google3 is to prefer
function _exec() {}
over
const _exec = () => {}
since they are equivalent and the former was the previous idiom

Wow, sounds like a bad guideline to me 😁

It is more verbose and not 100% equivalent 😞
Is the preference to also use functions and protypes instead of classes? 😛

alexeagle · 2018-12-04T20:11:22Z

tools/bazel_stamp_vars.js

+console.error('Running', process.argv.join(' '));
+
+function onError() {
+  console.log('Failed to execute:,', process.argv.join(' '));


should these be printed to stderr instead?

alexeagle · 2018-12-04T20:11:59Z

tools/bazel_stamp_vars.js

+  console.log(`No git tags found, can't stamp the build.`);
+  console.log('Either fetch the tags:');
+  console.log('       git fetch git@github.com:angular/angular.git --tags');
+  console.log('or build without stamping by giving an empty workspace_status_command:');


I think this needs an update since workspace_status_command is now opt-in

Updated to just ask the user to fetch the tags.

ngbot · 2019-01-09T13:24:18Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "continuous-integration/travis-ci/pr" is failing
     missing required labels: PR target: *
     status "code-review/pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

ngbot · 2019-01-09T16:58:05Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "ci/circleci: legacy-unit-tests-saucelabs" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

PR Close angular#27431

angular-automatic-lock-bot · 2019-09-14T12:51:24Z

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.}

googlebot added the cla: yes label Dec 3, 2018

filipesilva requested a review from alexeagle December 3, 2018 19:46

alexeagle approved these changes Dec 4, 2018

View reviewed changes

alexeagle 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 Dec 4, 2018

filipesilva force-pushed the windows-workspace-command branch 3 times, most recently from f918cac to 76a1b92 Compare December 5, 2018 11:35

filipesilva added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 9, 2019

filipesilva 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 Jan 9, 2019

build: use cross platform workspace_status_command

0812d36

filipesilva force-pushed the windows-workspace-command branch from 76a1b92 to 0812d36 Compare January 9, 2019 13:26

filipesilva requested a review from a team as a code owner January 9, 2019 13:26

filipesilva added target: major This PR is targeted for the next major release and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jan 9, 2019

alexeagle approved these changes Jan 9, 2019

View reviewed changes

filipesilva added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note area: build & ci Related the build and CI infrastructure of the project labels Jan 9, 2019

ngbot bot added this to the needsTriage milestone Jan 9, 2019

AndrewKushnir closed this in d6cfe2e Jan 9, 2019

ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

build: use cross platform workspace_status_command (angular#27431)

627b9c8

PR Close angular#27431

devversion mentioned this pull request Mar 27, 2019

build: add version name pre release check [patch] #29552

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: use cross platform workspace_status_command #27431

build: use cross platform workspace_status_command #27431

filipesilva commented Dec 3, 2018

alexeagle Dec 4, 2018

filipesilva Dec 5, 2018

alexeagle Dec 4, 2018

filipesilva Dec 5, 2018

gkalpak Dec 5, 2018

alexeagle Dec 4, 2018

filipesilva Dec 5, 2018

alexeagle Dec 4, 2018

filipesilva Dec 5, 2018

ngbot bot commented Jan 9, 2019

ngbot bot commented Jan 9, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

build: use cross platform workspace_status_command #27431

build: use cross platform workspace_status_command #27431

Conversation

filipesilva commented Dec 3, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Jan 9, 2019

ngbot bot commented Jan 9, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019