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 pnpm installs by moving binary download location #1927

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

jonathanrainer
Copy link
Contributor

@jonathanrainer jonathanrainer commented Jun 14, 2024

Fixes #1881

The Problem

This bug requires a little explanation. So, in the release of v0.23.0 of Rover we inlined the binary-install dependency to resolve a security vulnerability with axios (#1819). The inlining of this dependency caused the place where rover binaries were downloaded to change, but in such a way that it only affected installs via pnpm.

The reason for this is that the code that was inlined was using the __dirname variable to site the binary installation, and this variable always contains the path of the folder where the currently executing script is located. Normally this would not be a problem because the script ran inside the binary-install packages folder. As evidenced by running console.log against the output of trying to install the binary under Rover v0.22.0

root@3479114cabfc:/node_modules/.bin# ./rover
Binary {
  url: 'https://rover.apollo.dev/tar/rover/x86_64-unknown-linux-gnu/v0.22.0',
  name: 'rover',
  installDirectory: '/node_modules/.pnpm/binary-install@1.1.0/node_modules/binary-install/node_modules/.bin',
  binaryPath: '/node_modules/.pnpm/binary-install@1.1.0/node_modules/binary-install/node_modules/.bin/rover'
}
....

However, when the package was inlined and moved the location of the script itself changed and so therefore did the value of __dirname and so it tried to install the binary somewhere else, as evidenced by running the same commands as above but this time using Rover v0.23.0

root@44b617aea38c:/usr/local/lib/node_modules/@apollo/rover# ./run.js
Binary {
  url: 'https://rover.apollo.dev/tar/rover/aarch64-unknown-linux-gnu/v0.23.0',
  name: 'rover',
  installDirectory: '/usr/local/lib/node_modules/@apollo/rover/node_modules/.bin',
  binaryPath: '/usr/local/lib/node_modules/@apollo/rover/node_modules/.bin/rover'
}

We see here that it's now trying to install into the node_modules/.bin inside the rover package itself. This is what leads to problems because this location is already utilised by pnpm to store a shell script (also called rover) that calls out to the run.js. So under Rover v0.23.0 we end up with an infinite loop where binary.js looks for the binary, finds the one put in by pnpm, invokes it, which then looks for the binary, finds the one put there by pnpm, invokes it and so on.

Why was this not a problem when using `npm` to install?
Under `npm` the situation is different, if you install `rover` globally then the shell script that invokes run.js gets installed to `usr/local/bin/rover` which then will not conflict or form the infinite loop (as per https://docs.npmjs.com/cli/v6/configuring-npm/package-json#bin)

If you install rover as part of another npm project then the binary gets installed to the node_modules inside the rover package as it's installed as can be seen below

Binary {
  url: 'https://rover.apollo.dev/tar/rover/aarch64-unknown-linux-gnu/v0.23.0',
  name: 'rover',
  installDirectory: '/node_modules/@apollo/rover/node_modules/.bin',
  binaryPath: '/node_modules/@apollo/rover/node_modules/.bin/rover'
}

So this will not conflict either because what's specified in the bin stanza in package.json gets installed to the top level node_modules so again no conflict.

The Solution

If we stop using the __dirname variable and instead use something relative to the package.json of Rover itself all these problems disappear. This is not a known path so will not get clobbered by another package manager (pnpm or otherwise) and will allow consistency across platforms. That is what this PR proposes

Testing

Added a few more CI steps to ensure we can install in all JS domains (npm, npm (global), pnpm), as can be seen here, when run with the CI without the fix we get the following behaviour

Screenshot 2024-06-14 at 14 23 12

However when run with the fix the CI check runs green, thus in future we shouldn't have this problem/it'll be caught in CI

Copy link
Contributor

@nmoutschen nmoutschen left a comment

Choose a reason for hiding this comment

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

This LGTM and thanks for the detailed explanations! However, might be worth having an extra pair of eyes before merging this.

@jonathanrainer jonathanrainer force-pushed the fix-pnpm-installs branch 10 times, most recently from a7ea164 to 9eee8cf Compare June 14, 2024 13:21
@jonathanrainer jonathanrainer marked this pull request as ready for review June 14, 2024 13:26
@jonathanrainer jonathanrainer requested a review from a team as a code owner June 14, 2024 13:26
@jonathanrainer jonathanrainer force-pushed the fix-pnpm-installs branch 3 times, most recently from 71f3a5f to 90e7e43 Compare June 15, 2024 07:40
We ended up breaking stuff in v0.23.0 because of
changes to the libraries used, this allows us to
both:

- See if this ever happens again
- Prove we've actually fixed the problem
Instead of installing to a location relative to the `binary.js` file,
which leaves us open to problems due to how package managers may
choose to work, use a /binary folder at the same level as the `package.json`
instead.
@jonathanrainer jonathanrainer merged commit 691e5b0 into main Jun 17, 2024
14 checks passed
@jonathanrainer jonathanrainer deleted the fix-pnpm-installs branch June 17, 2024 07:39
@jonathanrainer jonathanrainer added this to the v0.24.0 milestone Jul 10, 2024
@jonathanrainer jonathanrainer added the fix 🩹 fixes a bug label Jul 10, 2024
@jonathanrainer jonathanrainer mentioned this pull request Jul 12, 2024
jonathanrainer added a commit that referenced this pull request Jul 15, 2024
# [0.24.0]

> Important: 1 potentially breaking change below, indicated by **❗
BREAKING ❗**

## ❗ BREAKING ❗

- **Removed the deprecated `plain` and `json` options for `--output` -
@dylan-apollo PR
[#1804](#1804

The `--output` option is now only for specifying a file to write to. The
`--format` option should be used to specify the format of the output.

## 🚀 Features

- **Return the name of the linting rule that is violated, as well as the
code - @jonathanrainer PR
[#1907](#1907

Originally only the message from the linting violation was included in
the response, but now it also includes the name of the specific linting
rule to aid debugging

- **Use the Router's `/health?ready` endpoint to check readiness -
@nmoutschen PR
[#1939](#1939

Previously `rover dev` used a simple query to establish readiness, but
this did not allow for router customizations.

- **Adding architecture and OS metrics - @aaronArinder PR
[#1947](#1947

Allows us to track the Operating Systems and Architectures in use by our
users, this will give us more information as to where to focus support
efforts

- **Allow `aarch64` macOS to pull correct `supergraph` binaries where
available - @jonathanrainer PR
[#1971](#1971

We recently started publishing `supergraph` binaries for `aarch64`, so
if they are available Rover will use them in preference to x86_64
binaries.

## 🐛 Fixes

- **Don't panic if the telemetry client cannot be initialised -
@dylan-apollo PR
[#1897](#1897) - Issue
[#1893](#1893
- **Rename `.cargo/config` to `.cargo/config.toml` - @jonathanrainer PR
[#1921](#1921
- **Fix `pnpm` installs by moving the binary download location -
@jonathanrainer PR
[#1927](#1927) - Issue
[#1881](#1881
  
After we inlined the `binary-install` dependency in v0.23.0 this changed
where the downloaded binary was stored when using `pnpm`. This caused
users running the binary to enter an infinite loop. This moves the
binary to a new location which avoids this.

- **Don't panic on file watcher errors - @nmoutschen PR
[#1935](#1935

Instead of panicking when errors occur watching files return those
errors gracefully to the user.

- **Store binaries with version numbers attached so upgrades are
possible - @jonathanrainer PR
[#1932](#1932) - Issue
[#1563](#1563

When downloading binaries via `npm` they were always stored as `rover`
despite the version. As such, when a new version came out the upgrade
would fail. This now doesn't happen, as binaries are stored with their
versions number in the name.

- **Ensure correct URL is used if `subgraph_url` and `routing_url` are
provided in a supergraph schema - @jonathanrainer PR
[#1948](#1948) - Issue
[#1782](#1782
- **Let `--output` accept paths with missing intermediate directories -
@jonathanrainer PR
[#1944](#1944) - Issue
[#1787](#1787
- **Allow `rover dev` to read Federation Version from supergraph schema
- @jonathanrainer PR
[#1950](#1950) - Issue
[#1735](#1735

The Federation version could be set in the supegraph schema but was
being ignored by `rover dev`. It now is taken into account, along with
the overriding environment variable.

- **Stop .exe being printed after Federation version during composition
- @jonathanrainer PR
[#1951](#1951) - Issue
[#1390](#1390
- **Reinstate support for `glibc` 2.17 - @jonathanrainer PR
[#1953](#1953

In resolving the issues with CentOS 7 we accidentally removed support
for `glibc` 2.17, this has now been restored

- **Be more lenient about `supergraph` binary versions - @dylan-apollo
PR [#1966](#1966

In resolving #1390, we were too restrictive in what counted as a valid
version. This restores the correct behaviour

- **Set `package.json` to a stable version when testing NPM Installers -
@jonathanrainer PR
[#1967](#1967

When testing whether our NPM installers worked correctly we were trying
to download the latest `rover` binary. On release PRs, where the binary
didn't yet exist, this was causing problems.

- **Fix mocking of calls to Orbiter in Installer tests - @jonathanrainer
PR [#1968](#1968
- **Remove noisy errors from intermediate composition states -
@aaronArinder PR
[#1956](#1956
  
When `rover dev` composes multiple subgraphs it does so one at a time.
As such if there are dependencies there can be noisy ephemeral errors,
this fixes that by waiting until all subgraphs are added before trying
composition.

## 🛠 Maintenance

- **Update GitHub CircleCI Orb to v2.3.0 - @Geal PR
[#1831](#1831
- **Update plugins to Fed 2.7 and Router 1.43.0 - @smyrick PR
[#1877](#1877
- **Update CODEOWNERS - @dotdat PR
[#1890](#1890

  Make Betelgeuse the primary owners of the Rover repository

- **Update lychee-lib to v0.15 - @dotData PR
[#1902](#1902
- **Add tests and provide status codes as part of linter errors -
@dotdat PR [#1903](#1903
- **Add nix files to .gitignore - @aaronArinder PR
[#1908](#1908
- **Update apollographql/router to v1.47.0 - @aaronArinder PR
[#1841](#1841
- **Update apollographql/federation-rs to v2.7.8 - @aaronArinder PR
[#1746](#1746
- **Update node.js to v20 - @aaronArinder PR
[#1778](#1778
- **Update Rust to v1.76.0 and the Rust CircleCI Orb to v1.6.1 -
@aaronArinder PR
[#1788](#1788
- **Update serial_test to v3 - @jonathanrainer PR
[#1836](#1836
- **Update which to v6 - @jonathanrainer PR
[#1835](#1835
- **Update apollographql/federation-rs to v2.8.0 - @aaronArinder PR
[#1909](#1909
- **Update tar to v6.2.1 - @aaronArinder PR
[#1888](#1888
- **Update tar to v7 - @aaronArinder PR
[#1914](#1914
- **Update node.js packages - @aaronArinder PR
[#1830](#1830

Includes `eslint` to v8.57.0, `node.js` to v20.14.0, `nodemon` to
v3.1.2, `npm` to v10.8.1 and `prettier` to v3.3.0

- **Update Rust to v1.78.0 - @aaronArinder PR
[#1912](#1912
- **Update apollographql/router to v1.48.0 - @aaronArinder PR
[#1917](#1917
- **Update zip to v2 - @jonathanrainer PR
[#1916](#1916
- **Update eslint to v9.4.0 - @dotdat PR
[#1913](#1913
- **Update hyper to v1.0 - @dotdat PR
[#1789](#1789
- **Add tests for socket names - @jonathanrainer PR
[#1918](#1918
  
In future dependency upgrades we want to ensure that behaviour around
socket naming works as expected, so add a test to ensure that.

- **Update rust packages - @jonathanrainer PR
[#1755](#1755

Consolidates updates of pre-1.0 rust crates, check PR for full details
of crates updated

- **Update notify to v6 - @jonathanrainer PR
[#1603](#1603
- **Include cargo-deny checks on PRs - @jonathanrainer PR
[#1910](#1910

Now we can check for licences that don't correspond to our allowed list
and pick up on dependency issues live on PRs

- **Pin node.js dev dependencies - @aaronArinder PR
[#1923](#1923
- **Allow 0BSD licence - @aaronArinder PR
[#1924](#1923
- **Update interprocess to v2 - @dotdat PR
[#1915](#1915
- **Update apollographql/router to v1.48.1 - @dotdat PR
[#1926](#1926
- **Update Rust to v1.79.0 - @jonathanrainer PR
[#1931](#1931
- **Update git2 to v0.19 - @jonathanrainer PR
[#1930](#1930
- **Update node.js packages - @jonathanrainer PR
[#1929](#1929

Includes `@eslint/compat` to v1.1.0, `eslint` to v9.5.0, `graphql` to
v16.8.2 and `prettier` to v3.3.2

- **Migrate CI to use manylinux rather than CentOS 7 - @jonathanrainer
PR [#1952](#1952

As CentOS 7 has now entered End-of-Life, migrate our CI to use a
different Linux distribution.

- **Update apollographql/router to v1.49.1 - @jonathanrainer PR
[#1933](#1933
- **Update apollographql/federation-rs to v2.8.2 - @jonathanrainer PR
[#1934](#1934
- **Update node.js packages - @jonathanrainer PR
[#1940](#1940

Includes `eslint` to v9.6.0, `node.js` to v20.15.0, `nodemon` to v3.1.4,
`graphql` to v16.9.0

- **Fix clippy warnings - @loshz PR
[#1955](#1955
- **Allow integration tests to accept a pre-compiled binary -
@jonathanrainer PR
[#1957](#1957
- **Run macOS x86_64 integration tests in GitHub Actions - @nmoutschen
PR [#1958](#1958
  
Due to CircleCI's deprecation of x86_64 macOS executors use GitHub
Actions to still run our tests on this architecture

- **Add smoke tests for `rover dev` - @jonathanrainer PR
[#1961](#1961
- **Update apollographql/router to v1.50.0 - @jonathanrainer PR
[#1954](#1954
- **Trigger GitHub Actions from CircleCI - @nmoutschen PR
[#1959](#1959
- **Add docs team to CODEOWNERS - @aaronArinder PR
[#1965](#1965
- **Fix up Release CI and explicitly add tokio `rt-multi-thread flag` -
@jonathanrainer PR
[#1972](#1972
- **Add context to auth output when saving an API Key - @loshz PR
[#1974](#1974


## 📚 Documentation

- **Minor update to README.md - @tratzlaff PR
[#1880](#1880

  Fixes use of numbered lists in the README.md 

- **Remove failing/redundant links from docs - @dotdat PR
[#1894](#1894
- **Update docs style - @Meschreiber PR
[#1883](#1883

  Update formatting and admonitions to most recent conventions.

- **Update frontmatter - @Meschreiber PR
[#1898](#1898

  Updates title casing and adds metadata to subtitles

- **Clarify `subgraph publish` can only create variants not graphs -
@Meschreiber PR
[#1938](#1938
- **Make example using `-` instead of filepath clearer - @aaronArinder
PR [#1963](#1963
- **Update Router terminology - @Meschreiber PR
[#1925](#1925

Update the uses of Apollo Router to GraphOS Router or Apollo Router Core
where necessary
- **Update documentation to make it clear we collect CPU Architecture,
per command - @aaronArinder PR
[#1964](#1964
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix 🩹 fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rover 0.23.0 doesn't work correctly with pnpm
3 participants