build: remove NPM from builder #27163
Labels
A-build-system
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
no-issue-activity
T-dev-inf
Comments
benesch
added a commit
to benesch/cockroach
that referenced
this issue
Jul 4, 2018
ProtobufJS's CLI attempts to install its own dependencies by invoking NPM. This breaks our dependency pinning and vendoring, so we pre-install its dependencies ourselves. Unfortunately, the CLI maintains two different dependency lists: the cliDependencies field in package.json, and the dependencies field in cli/package.standalone.json. We were looking at the latter when pre-installing the CLI's dependencies, but only the former is maintained. This patch adds a script to extract a package.json that encodes up-to-date dependency information. The Makefile then invokes `yarn install` on this synthesized package. This guarantees that the ProtobufJS CLI will see that the correct versions of its dependencies are pre-installed and will not invoke NPM. The resulting diff in pkg/ui/yarn.protobuf-cli.lock reveals that ProtobufJS was installing newer versions of its dependencies without us realizing it. Note that it is now safe to use new versions of jsdoc; the issue mentioned in the Makefile was fixed in jsdoc 3.5.4 (jsdoc/jsdoc#1408). To prevent accidents in the future, a forthcoming commit will remove NPM from the builder image so that CI will fail if ProtobufJS attempts to invoke NPM behind our back. See cockroachdb#27163. Release note: None
craig bot
pushed a commit
that referenced
this issue
Jul 4, 2018
27164: ui: get smarter about pinning protobufjs's dependency set r=knz a=benesch ProtobufJS's CLI attempts to install its own dependencies by invoking NPM. This breaks our dependency pinning and vendoring, so we pre-install its dependencies ourselves. Unfortunately, the CLI maintains two different dependency lists: the cliDependencies field in package.json, and the dependencies field in cli/package.standalone.json. We were looking at the latter when pre-installing the CLI's dependencies, but only the former is maintained. This patch adds a script to extract a package.json that encodes up-to-date dependency information. The Makefile then invokes `yarn install` on this synthesized package. This guarantees that the ProtobufJS CLI will see that the correct versions of its dependencies are pre-installed and will not invoke NPM. The resulting diff in pkg/ui/yarn.protobuf-cli.lock reveals that ProtobufJS was installing newer versions of its dependencies without us realizing it. Note that it is now safe to use new versions of jsdoc; the issue mentioned in the Makefile was fixed in jsdoc 3.5.4 (jsdoc/jsdoc#1408). To prevent accidents in the future, a forthcoming commit will remove NPM from the builder image so that CI will fail if ProtobufJS attempts to invoke NPM behind our back. See #27163. Release note: None Co-authored-by: Nikhil Benesch <[email protected]>
petermattis
added
the
C-enhancement
|
We have marked this issue as stale because it has been inactive for |
|
We have marked this issue as stale because it has been inactive for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
NPM is not a dependency, but PBJS sometimes makes it one: protobufjs/protobuf.js#716
Remove NPM from the builder so it's clear when PBJS is misbehaving and invoking NPM behind our backs.
Epic CRDB-10447
Jira issue: CRDB-4963
The text was updated successfully, but these errors were encountered: