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

fix(builtin): only stamp artifacts when --stamp is passed to bazel #1441

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

alexeagle
Copy link
Collaborator

Fixes remote cache misses of rollup_bundle and npm_package targets observed by Angular team

Copy link
Collaborator

@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, couple nits

docs/index.md Outdated

> Previous releases of rules_nodejs stamped builds always.
> However this caused stamp-aware actions to never be remotely cached, since the volatile
> status file is passed as an input and its checksum cannot be known.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: technically its that its checksum always changes

@@ -129,6 +130,11 @@ NPM_PACKAGE_ATTRS = {
doc = """Files inside this directory which are simply copied into the package.""",
allow_files = True,
),
"node_context_data": attr.label(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is internal should it be _ prefixed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the rollup case, I override from a test. I guess I have no such test right now for npm_package doing the stamp thing but I like them being the same, and we ought to have such a test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, the test for npm_package stamping was just busted. when I fixed the test it required passing this data to override --stamp. Fixed

Fixes remote cache misses of rollup_bundle and npm_package targets observed by Angular team
@alexeagle alexeagle merged commit cbaab60 into bazel-contrib:master Dec 10, 2019
josephperrott added a commit to josephperrott/components that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
mmalerba pushed a commit to angular/components that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 16, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 19, 2019
Updating to latest rules_nodejs which properly handles stamping for
releases.  Previously rollup_bundle and npm_package were unable to
be cached remotely.
See bazel-contrib/rules_nodejs#1441 for changes.

With new requirements from changes in rules_nodejs, we now must add
--stamp to bazel builds we wish to include stamping.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants