Sitelet https://web.archive.org/web/20260605025928/https://github.com/angular/angular/pull/39636
Skip to content

build: update bazel rules_nodejs to 2.3.1 to use the fixed npm_package.pack rule on Windows OS#39636

Closed
JiaLiPassion wants to merge 3 commits into
angular:masterfrom
JiaLiPassion:npm-pack-bazel
Closed

build: update bazel rules_nodejs to 2.3.1 to use the fixed npm_package.pack rule on Windows OS#39636
JiaLiPassion wants to merge 3 commits into
angular:masterfrom
JiaLiPassion:npm-pack-bazel

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion commented Nov 11, 2020 •
edited
Loading

Update rules_nodejs to 2.3.1 to use the new npm_package.pack rule which is also working on Windows OS now by this fix bazel-contrib/rules_nodejs@bc36519

@google-cla google-cla Bot added the cla: yes label Nov 11, 2020
@pullapprove pullapprove Bot requested a review from IgorMinar November 11, 2020 17:48
IgorMinar
IgorMinar approved these changes Nov 11, 2020
View reviewed changes
Copy link
Copy Markdown
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.

// @petebacondarwin fyi since you mentioned rules_nodejs today.

@IgorMinar IgorMinar added target: patch This PR is targeted for the next patch release area: build & ci Related the build and CI infrastructure of the project labels Nov 11, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Nov 11, 2020
@petebacondarwin
Copy link
Copy Markdown
Contributor

petebacondarwin commented Nov 11, 2020

LGTM.

// @petebacondarwin fyi since you mentioned rules_nodejs today.

Thanks @IgorMinar - I was told v3 but perhaps they meant v2.3? I'll give it a try

gkalpak
gkalpak approved these changes Nov 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

gkalpak
gkalpak suggested changes Nov 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just noticed that running node scripts\build\build-packages-dist.js leaves behind an undefined-0.1.0.tgz file in my working directory 😕

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Nov 12, 2020 •
edited
Loading

@gkalpak , thanks, I fixed it in this PR bazel-contrib/rules_nodejs#2282 (comment) to fix this undefined-0.1.0.tgz, this should not impact the build and test, but it is annoying to generate an unused file locally when we build .pack on Windows OS, do we need to wait for the next release?

@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Nov 12, 2020

Nice! I would say let's wait for the next release, because people will see this bogus .tgz there and won't know what to do with it or where it came from.

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Nov 12, 2020

@gkalpak , got it, thanks!

@JiaLiPassion JiaLiPassion marked this pull request as draft November 12, 2020 17:11
@JiaLiPassion JiaLiPassion marked this pull request as ready for review December 3, 2020 20:52
@JiaLiPassion JiaLiPassion requested a review from gkalpak December 3, 2020 21:25
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Dec 3, 2020 •
edited
Loading

@gkalpak , the rule_nodejs has been updated to 2.3.1, and the issue earlier undefined-0.1.0.tgz is gone, could you take a look again? Thanks.

@JiaLiPassion JiaLiPassion changed the title build: update bazel rules_nodejs to 2.3.0 to use the fixed npm_package.pack rule on Windows OS build: update bazel rules_nodejs to 2.3.1 to use the fixed npm_package.pack rule on Windows OS Dec 3, 2020
gkalpak
gkalpak approved these changes Dec 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread integration/bazel/WORKSPACE Outdated
@gkalpak gkalpak 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, 2020
@ngbot ngbot Bot modified the milestones: needsTriage, Backlog Dec 4, 2020
@JiaLiPassion JiaLiPassion force-pushed the npm-pack-bazel branch 2 times, most recently from 7918893 to bcf1a9b Compare December 4, 2020 17:36
@JiaLiPassion JiaLiPassion 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 Dec 4, 2020
mhevery added a commit that referenced this pull request Dec 8, 2020
@mhevery
Revert "build: update to use npm_package.pack rule (#39636)"
1812479 
This reverts commit 545ce2b.
mhevery added a commit that referenced this pull request Dec 8, 2020
@mhevery
Revert "build: update bazel rules_nodejs to version 2.3.1 (#39636)"
19309b5 
This reverts commit b82c88a.
@google-cla

This comment has been minimized.

Sign in to view
@google-cla google-cla Bot added cla: no and removed cla: yes labels Dec 8, 2020
@pullapprove pullapprove Bot removed the area: build & ci Related the build and CI infrastructure of the project label Dec 9, 2020
@ngbot ngbot Bot removed this from the Backlog milestone Dec 9, 2020
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Dec 10, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla Bot added cla: no and removed cla: yes labels Dec 10, 2020
@pullapprove pullapprove Bot added area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) area: testing Issues related to Angular testing features, such as TestBed area: zones Issues related to zone.js labels Dec 10, 2020
@ngbot ngbot Bot modified the milestone: Backlog Dec 10, 2020
@alxhub alxhub added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Dec 14, 2020
@alxhub
Copy link
Copy Markdown
Member

alxhub commented Dec 14, 2020

@JiaLiPassion this PR doesn't merge cleanly to 11.0.x. Please open a separate PR for that branch if needed.

@alxhub alxhub closed this in 68cf012 Dec 14, 2020
alxhub pushed a commit that referenced this pull request Dec 14, 2020
@JiaLiPassion @alxhub
build: update to use npm_package.pack rule (#39636)
35a1975 
Remove the work around solution for the `npm pack`, we can now
use `npm_package.pack` rule of bazel, since the windows os issue
has been fixed here bazel-contrib/rules_nodejs@bc36519

PR Close #39636
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 17, 2020
@josephperrott
Revert "build: update bazel rules_nodejs to version 2.3.1 (angular#39636
7949d8a 
)"

This reverts commit 68cf012.
josephperrott added a commit that referenced this pull request Dec 17, 2020
@josephperrott
Revert "build: update bazel rules_nodejs to version 2.3.1 (#39636)" (#…
ee6b8a7 
…40174)

This reverts commit 68cf012.

PR Close #40174
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Jan 14, 2021

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 Jan 14, 2021
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

@gkalpak gkalpak gkalpak 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: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) area: testing Issues related to Angular testing features, such as TestBed area: zones Issues related to zone.js cla: yes target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

7 participants

@JiaLiPassion @petebacondarwin @gkalpak @alxhub @IgorMinar @mhevery @josephperrott