Skip to content

Conversation

kormide
Copy link
Contributor

@kormide kormide commented Dec 6, 2021

@gregmagolan @josephperrott

This PR modifies the existing bazel rules to reproduce the packages that are currently being produced by the build.ts script. I also added version stamping which works for snapshot and release builds.

There are still some differences between the package.json files that bazel produces compared to build.ts, which I will work through in future pull requests. These differences include:

  • using tars as dependency versions for local builds
  • additional substitutions such as removing devDependencies, merging keywords from the base package.json, etc.

Once bazel is producing identical pacakges, I will switch the build.ts script to call bazel instead of tsc. Until then, none of this should affect the currently used build process.

@google-cla google-cla bot added the cla: yes label Dec 6, 2021
@@ -40,3 +43,73 @@ def ts_library(
# @external_end
**kwargs
)

def pkg_npm(name, use_prodmode_output = False, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this pkg_npm override over from the angular repo.

Comment on lines 52 to 60
substitutions = dict(common_substitutions, **{
# TODO: Current build script relies on 0.0.0 in package.json; uncomment after repalcing build script.
#"0.0.0-PLACEHOLDER": "0.0.0",
})
stamped_substitutions = dict(common_substitutions, **{
# TODO: Current build script relies on 0.0.0 in package.json; uncomment after repalcing build script.
#"0.0.0-PLACEHOLDER": "{BUILD_SCM_VERSION}",
"0.0.0": "{BUILD_SCM_VERSION}",
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented this out for now, but I thought it would be a good idea to use 0.0.0-PLACEHOLDER rather than the current 0.0.0 versions in each package's package.json file.` I can't change those package.json files yet as that would break the current build script that uses 0.0.0 for version substitution.

Comment on lines +310 to +315
genrule(
name = "license",
srcs = ["//:LICENSE"],
outs = ["LICENSE"],
cmd = "cp $(execpath //:LICENSE) $@",
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pkg_npm requires that included files be in the same directory, so this just copies the LICENSE file from the root.

@@ -61,7 +56,7 @@ ts_library(
exclude = [
# NB: we need to exclude the nested node_modules that is laid out by yarn workspaces
"node_modules/**",
"cli/lib/config/workspace-schema.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

@alan-agius4 This path was wrong so lib/config/workspace-schema.json was not actually being excluded. this dates back to your March commit 4b0223b. Is it correct to exclude it from this ts_library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this file gets converted to schema.json which is included.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah it should be excluded.

@gregmagolan gregmagolan self-requested a review December 8, 2021 18:57
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

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Dec 9, 2021
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 Dec 9, 2021

@alan-agius4 everything look okay to you?

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.

@kormide LGTM thanks

@alan-agius4 alan-agius4 merged commit 31801c1 into angular:master Dec 10, 2021
@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 Jan 10, 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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants