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

build: use bazel for snapshot builds #22543

Merged
merged 2 commits into from
Feb 2, 2022
Merged

build: use bazel for snapshot builds #22543

merged 2 commits into from
Feb 2, 2022

Conversation

kormide
Copy link
Contributor

@kormide kormide commented Jan 22, 2022

@gregmagolan @josephperrott

Here are the snapshot repo diffs caused by flipping to the bazel build.

Snapshot repo Diff
@angular/cli diff
@angular/pwa diff
@angular-devkit/architect diff
@angular-devkit/architect-cli diff
@angular-devkit/build-angular diff
@angular-devkit/build-webpack diff
@angular-devkit/core diff
@angular-devkit/schematics diff
@angular-devkit/schematics-cli diff
@ngtools/webpack diff
@schematics/angular diff

Note that some changes in these diffs may be caused by new things pushed to master of the "build" repos.

CI runs for the snapshot-test branch set up to test the ci. The build artifacts are published to the build repos under the same snapshot-test branch name.

@@ -8,7 +8,7 @@ load("//:constants.bzl", "SNAPSHOT_REPOS")
def _generate_snapshot_repo_filter():
filter = ""
for (i, pkg_name) in enumerate(SNAPSHOT_REPOS.keys()):
filter += "{sep}(..|objects|select(has(\"{pkg_name}\")))[\"{pkg_name}\"] |= \"github:{snapshot_repo}:BUILD_SCM_HASH-PLACEHOLDER\"\n".format(
filter += "{sep}(..|objects|select(has(\"{pkg_name}\")))[\"{pkg_name}\"] |= \"github:{snapshot_repo}#BUILD_SCM_HASH-PLACEHOLDER\"\n".format(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug in the github dependency format.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@kormide
Copy link
Contributor Author

kormide commented Jan 28, 2022

I'd like to run a few more tests before we mark this as ready to merge.

@kormide
Copy link
Contributor Author

kormide commented Jan 29, 2022

I ran into an issue trying to manually install the snapshots. The bazel build stamps snapshot versions of dependencies like:

github:angular/angular-devkit-architect-builds#bb8bc80581417898b10bdc23ef4d9ee4295da2a9

It's using the stamp variable {BUILD_SCM_COMMIT_SHA} provided by the ng-dev tool.

However, legacy build stamps using an abbreviated version of the SHA1, and the snapshot script tags the commit in the build repo using the abbreviated version, so running an npm install of the snapshots fails.

I tried changing the snapshot script to tag using the longer hash, however git complains that the name is too long.

Something I could do is to change the dev-infra repo to output another variable with the abbreviated hash (e.g., {BUILD_SCM_COMMIT_SHA_ABBREV}) so that we can stamp that instead. Wdyt @josephperrott?

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jan 31, 2022

Couple of questions.

  • why is the version 13.2.0-next and not 14.0.0-next?
    Screenshot 2022-01-31 at 13 06 35
  • can we also turn off source-map generation?

@kormide
Copy link
Contributor Author

kormide commented Jan 31, 2022

* why is the version `13.2.0-next` and not `14.0.0-next`?

I think that's just because master in the build repo has a more recent snapshot that the branch I'm working off of. That should resolve itself the next time a snapshot is pushed on snapshot-test.

* can we also turn off source-map generation?

Unfortunately I don't think we can with ts_library. @josephperrott previously ok'ed this, and it's consistent with what the angular bazel build produces.

For reference here's the CI: https://app.circleci.com/pipelines/github/angular/angular-cli?branch=snapshot-test&filter=all

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jan 31, 2022

Unfortunately I don't think we can with ts_library. @josephperrott previously ok'ed this, and it's consistent with what the angular bazel build produces.

This is not a big of deal, although sourcemaps are a bit redundant here and kinda of extra mbs over the wire and disk in this case. Unlike for angular/angular were they are useful for app developers.

But yeah, I don't think there is a away to disable sourcemaps in ts_library.

@kormide
Copy link
Contributor Author

kormide commented Jan 31, 2022

I'm now able to install the snapshot build locally. I'm getting a failure on the e2e windows tests, which I've seen in other ci runs so it appears unrelated to my changes. Is that test suite known to be flaky? https://app.circleci.com/pipelines/github/angular/angular-cli/20377/workflows/dc026fcf-1783-4d3d-9ac9-9ec9854c1eb3/jobs/286136

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jan 31, 2022

@kormide, looks like our Node.Js v12 CI is red.

Will investigate tomorrow, however it’s nothing related to this change.

@kormide
Copy link
Contributor Author

kormide commented Jan 31, 2022

@kormide, looks like our Node.Js v12 CI is red.

Will investigate tomorrow, however it’s nothing related to this change.

In that case, I think this is ready to go, unless you can think of anything else I should test first.

@alan-agius4
Copy link
Collaborator

Do you want to wait for @gregmagolan’s review?

@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Jan 31, 2022
@kormide
Copy link
Contributor Author

kormide commented Jan 31, 2022

Do you want to wait for @gregmagolan’s review?

Your and @josephperrott's review is sufficient. @gregmagolan reviewed previously and is in the loop, but hes out today.

@alan-agius4 alan-agius4 removed the request for review from gregmagolan January 31, 2022 21:54
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Jan 31, 2022
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

@dgp1130 dgp1130 added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Feb 2, 2022
@dgp1130 dgp1130 merged commit 82971c7 into angular:master Feb 2, 2022
@dgp1130
Copy link
Collaborator

dgp1130 commented Feb 2, 2022

There was a trivial conflict with package.json / yarn.lock in 13.2.x. So I merged this to master and did a manual cherry pick to 13.2.x in 491e210.

@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 Mar 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants