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.

Sign up for GitHub

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

Jump to bottom

npm_package: version placeholder sometimes not populated properly #1219

Closed
devversion opened this issue Oct 1, 2019 · 3 comments
Closed

npm_package: version placeholder sometimes not populated properly #1219

devversion opened this issue Oct 1, 2019 · 3 comments

Comments

@devversion
Copy link
Contributor

🐞 bug report

Affected Rule

npm_package

Is this a regression?

No

Description

Currently the version placeholder in the npm_package is populated by retrieving the version through a volatile stamp variable called: BUILD_SCM_VERSION.

This is not ideal because whenever the version changes, the npm_package targets are not re-built. Causing the old version to be part of the npm_package output.

Ideally the version placeholder would be a stable/non-volatile stamp variable that will cause the npm_package targets to be rebuilt if the version changes. Since this happens rarely and intentionally causes the version to be updated, it sounds like a reasonable solution.

Read more about stamp variables here: https://docs.bazel.build/versions/master/user-manual.html#flag--workspace_status_command
devversion added a commit to devversion/material2 that referenced this issue Oct 1, 2019
@devversion
build: ensure build-packages-dist script works in macOS
09346cf 
The `build-packages-dist` script does not seem to work on MacOS
because the `cp --no-preserve` flag is not supported by default.

This commit switches to a simple `chmod` call in order to make it
work in all Bash environments. Additionally the script has been updated
to clean previous `npm_package` outputs. This ensures that the version
placeholder is properly replaced in the release output.

This is necessary due to a bug in `rules_nodejs`. Read more here:
bazel-contrib/rules_nodejs#1219
@devversion devversion mentioned this issue Oct 1, 2019
Merged
devversion added a commit to devversion/material2 that referenced this issue Oct 1, 2019
@devversion
build: ensure build-packages-dist script works on macOS
6ef4985 
The `build-packages-dist` script does not seem to work on MacOS
because the `cp --no-preserve` flag is not supported by default.

This commit switches to a simple `chmod` call in order to make it
work in all Bash environments. Additionally the script has been updated
to clean previous `npm_package` outputs. This ensures that the version
placeholder is properly replaced in the release output.

This is necessary due to a bug in `rules_nodejs`. Read more here:
bazel-contrib/rules_nodejs#1219
jelbourn pushed a commit to angular/components that referenced this issue Oct 1, 2019
@devversion @jelbourn
build: ensure build-packages-dist script works on macOS (#17247)
c2562fb 
The `build-packages-dist` script does not seem to work on MacOS
because the `cp --no-preserve` flag is not supported by default.

This commit switches to a simple `chmod` call in order to make it
work in all Bash environments. Additionally the script has been updated
to clean previous `npm_package` outputs. This ensures that the version
placeholder is properly replaced in the release output.

This is necessary due to a bug in `rules_nodejs`. Read more here:
bazel-contrib/rules_nodejs#1219
@alexeagle
Copy link
Collaborator

It's subtle - see a good writeup bazelbuild/bazel#4842

I always write release scripts with --output_base=$(mktemp -d) - you really have to make a clean build when releasing to ensure all the artifacts picked up a new tag from version control. I don't think there's any way for Bazel to fix this.

Maybe you're proposing that we stop using Bazel's stamping mechanism altogether and do something like --define=BUILD_SCM_VERSION=1.2.3 and let that env var leak through to all node programs? Still won't work if you're cutting a full-stack release where the normal bazel stamping mechanism is used for one of your non-node binaries.

@devversion
Copy link
Contributor Author

@alexeagle Maybe I miss something, but my idea was that we just use the non-volatile stamping file and add it as an action input. That way, if one value of the stable keys changes, the npm_package will be re-assembled since the action has been invalidated.

If the contents of bazel-out/stable-status.txt change, Bazel invalidates the actions that depend on > them. In other words, if a stable key's value changes, Bazel will rerun stamped actions.

This can be achieved by using ctx.info_file instead of ctx.version_file and by renaming the key to STABLE_VERSION_PLACEHOLDER.

@alexeagle
Copy link
Collaborator

@devversion I should have listened to you! Yes the stable-version file ctx.info_file is better. #2157 fixes it, including using STABLE_* keys in the stamping documentation.
Next release Angular can stop doing --output_base=$(mktemp -d) which is bad because your whole build is non-incremental -only the terminal actions ought to be affected by stamping

devversion added a commit to devversion/dev-infra that referenced this issue Jan 12, 2023
@devversion
fix(ng-dev): always emit project version info as stable stamping keys
02f454f 
This fixes a long-standing issue where the project version is
emitted as a volatile key in stamping. This means that everytime a
new version is put into `package.json`, the NPM package will not
be rebuilt if it has been built previously.

This is the typical source of outdated NPM package artifacts where
the package version is accidentally still the old one. The Bazel
idiomatic fix is to use a `STABLE_` stamping key so that Bazel will
properly re-build dependent targets whenever the version changes.

Ideally as part of larger future Bazel refactorings we would also
make sure that only necessary target configurations depend on the
stable status key file. Currently `ng_package`/`pkg_npm` always
depends on it, even if there is no subsitution dependency on actual
stable keys.

bazel-contrib/rules_nodejs#1219 (comment)

Angular currently always wipes the cached artifact exactly to workaround
this issue. Previously it even built completely from scatch, dismissing
all of the previously cached artifacts.
devversion added a commit to devversion/dev-infra that referenced this issue Jan 12, 2023
@devversion
fix(ng-dev): always emit project version info as stable stamping keys
13ad93a 
This fixes a long-standing issue where the project version is
emitted as a volatile key in stamping. This means that everytime a
new version is put into `package.json`, the NPM package will not
be rebuilt if it has been built previously.

This is the typical source of outdated NPM package artifacts where
the package version is accidentally still the old one. The Bazel
idiomatic fix is to use a `STABLE_` stamping key so that Bazel will
properly re-build dependent targets whenever the version changes.

Ideally as part of larger future Bazel refactorings we would also
make sure that only necessary target configurations depend on the
stable status key file. Currently `ng_package`/`pkg_npm` always
depends on it, even if there is no subsitution dependency on actual
stable keys.

bazel-contrib/rules_nodejs#1219 (comment)

Angular currently always wipes the cached artifact exactly to workaround
this issue. Previously it even built completely from scatch, dismissing
all of the previously cached artifacts.
@devversion devversion mentioned this issue Jan 12, 2023
Merged
devversion added a commit to angular/dev-infra that referenced this issue Jan 12, 2023
@devversion
fix(ng-dev): always emit project version info as stable stamping keys
10e6f1b 
This fixes a long-standing issue where the project version is
emitted as a volatile key in stamping. This means that everytime a
new version is put into `package.json`, the NPM package will not
be rebuilt if it has been built previously.

This is the typical source of outdated NPM package artifacts where
the package version is accidentally still the old one. The Bazel
idiomatic fix is to use a `STABLE_` stamping key so that Bazel will
properly re-build dependent targets whenever the version changes.

Ideally as part of larger future Bazel refactorings we would also
make sure that only necessary target configurations depend on the
stable status key file. Currently `ng_package`/`pkg_npm` always
depends on it, even if there is no subsitution dependency on actual
stable keys.

bazel-contrib/rules_nodejs#1219 (comment)

Angular currently always wipes the cached artifact exactly to workaround
this issue. Previously it even built completely from scatch, dismissing
all of the previously cached artifacts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alexeagle @devversion