Binary install does not upgrade rover when npm version is upgraded

[justus-lattice]

Description

Rover does not upgrade to the latest binary whenever you upgrade from a previous npm version.

Steps to reproduce

• npm init
• npm install @apollo/rover@0.10.0
• npm install @apollo/rover@0.11.1
• node_modules/.bin/rover --version

Expected result

It should return Rover 0.11.1.

Actual result

Rover 0.10.0

Environment

Rover Info:
Version: 0.10.0
Install Location: /code/rover-sandbox/node_modules/binary-install/node_modules/.bin/rover
OS: Mac OS 12.6.4 [64-bit]
Shell: /bin/zsh

[maschwenk]

https://github.com/EverlastingBugstopper/binary-install/blob/main/packages/binary-install/index.js#L54-L66

More specifically I think the issue is in binary-install package, or Rover's use of it, because the binary install directory is stable across different versions of the Rover node module, so it keeps calling exists() and thinking it's already there

[maschwenk]

So I think the binary path needs to be keyed off the version of the binary or something similar, doesn't seem like an issue with the binary-install package

[abernix]

Hi! Thanks for opening this issue! I'm not able to reproduce the problem that you're encountering with the steps that you've included above:

As you can see, I'm correctly receiving the version of Rover that I requested.

I'm using Node v18.15.0 and npm version v9.5.0:

I don't see any recent bug that could have introduced behavior like this and I'm surprised that this hasn't been a problem more broadly, so I suspect there must be something more nuanced here.

I think this could take a bit of back and forth to get to the bottom of, but a few things that could help drive progress in the near term:

• Can you confirm that your reproduction instructions are sufficient to reproduce this in a fresh reproduction? (Can anyone else reproduce using just the steps above?)
• Assuming that you're encountering this in a larger project than this reproduction, could you verify that there is only a single version of @apollo/rover in your dependency tree? Running npm ls @apollo/rover could help highlight whether there are multiple competing dependencies that might be stomping on the (single) ./node_modules/.bin/rover binary.
• As a shorter-term work-around, is there a way you could leverage a different method of installing rover? For example, we have an example of installing rover without npm with the pinned-version instructions that lives in the Rover documentation. You'd want to avoid calling ./node_modules/.bin/rover at that point and make sure to just use the rover that is installed in the system's PATH, but this might help unblock you?

[kleinsch]

If you upgrade, you won't get the version of Rover you requested. That's the problem we're facing

Attempts to have devs upgrade the @apollo/rover package to 0.11.1, resulted in no change to the binary because of a cached version of their old binary, and new installs grabs the latest binary

nkleinschmidt@NKleinschmidt rovertest % npm init
This utility will walk you through creating a package.json file.
It only covers the most common items, and tries to guess sensible defaults.

See `npm help init` for definitive documentation on these fields
and exactly what they do.

Use `npm install <pkg>` afterwards to install a package and
save it as a dependency in the package.json file.

Press ^C at any time to quit.
package name: (rovertest) 
version: (1.0.0) 
description: 
entry point: (index.js) 
test command: 
git repository: 
keywords: 
author: 
license: (ISC) 
About to write to /private/tmp/rovertest/package.json:

{
  "name": "rovertest",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC"
}


Is this OK? (yes) 
nkleinschmidt@NKleinschmidt rovertest % npm install @apollo/rover@0.9.0    

added 33 packages, and audited 34 packages in 1s

4 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
nkleinschmidt@NKleinschmidt rovertest % ./node_modules/.bin/rover --version
Rover 0.9.0
nkleinschmidt@NKleinschmidt rovertest % cat ./node_modules/@apollo/rover/package.json | head -n 3
{
  "name": "@apollo/rover",
  "version": "0.9.0",
nkleinschmidt@NKleinschmidt rovertest % npm install @apollo/rover@0.10.0                     

changed 1 package, and audited 34 packages in 232ms

4 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
nkleinschmidt@NKleinschmidt rovertest % cat ./node_modules/@apollo/rover/package.json | head -n 3
{
  "name": "@apollo/rover",
  "version": "0.10.0",
nkleinschmidt@NKleinschmidt rovertest % ./node_modules/.bin/rover --version                  
Rover 0.9.0

[abernix]

@kleinsch Thanks for that detail! I can confirm the same behavior locally using the additional step of upgrading.

@justus-lattice Can you confirm that this is the same problem that you're reporting? Looking at the nuance in your text about upgrading, I'll assume that to be the case. If you agree, do you mind updating your reproduction steps in your original post to include the additional step of updating to another version?

@EverlastingBugstopper The assessment above by @maschwenk does seem to be the root cause in that, if the "exists" detects that it is there, it doesn't actually write over it with the new version. Not clobbering the existing binary totally makes sense! I could imagine that the comparison of the --version output would be one solution, but that doesn't actually apply as a solution since binary-install is meant to be a multi-purpose OSS npm package that works for all binaries. In that spirit, perhaps it is best that the installDirectory generation itself (within binary-install) be keyed as ${packageName}--${package-json-version} similar to how @maschwenk has suggested above?

[abernix]

@kleinsch, @maschwenk, @justus-lattice — just to make sure you catch my suggestion from above while we get to the bottom of this: there is a work-around in a non-npm sense that you can use: you can remove @apollo/rover and instead install it with the normal installer which absolutely does support pinning the version (docs linked above)

[justus-lattice]

@abernix Yes that is the same problem I am reporting, apologize for the incorrect reproduction steps. I have updated the original description

[maschwenk]

@kleinsch Thanks for that detail! I can confirm the same behavior locally using the additional step of upgrading.

@justus-lattice Can you confirm that this is the same problem that you're reporting? Looking at the nuance in your text about upgrading, I'll assume that to be the case. If you agree, do you mind updating your reproduction steps in your original post to include the additional step of updating to another version?

@EverlastingBugstopper The assessment above by @maschwenk does seem to be the root cause in that, if the "exists" detects that it is there, it doesn't actually write over it with the new version. Not clobbering the existing binary totally makes sense! I could imagine that the comparison of the --version output would be one solution, but that doesn't actually apply as a solution since binary-install is meant to be a multi-purpose OSS npm package that works for all binaries. In that spirit, perhaps it is best that the installDirectory generation itself (within binary-install) be keyed as ${packageName}--${package-json-version} similar to how @maschwenk has suggested above?

Yep, I think the simple fix is just keying the folder by the version of the package. You'll accumulate old version of the binary over time but I think that's better than the alternative.

@kleinsch, @maschwenk, @justus-lattice — just to make sure you catch my suggestion from above while we get to the bottom of this: there is a work-around in a non-npm sense that you can use: you can remove @apollo/rover and instead install it with the normal installer which absolutely does support pinning the version (docs linked above)

For sure, but the convenience of not having to have a bespoke step is nice. Particularly if you want to be able to have the same behavior across mac/linux/dev/CI.

[LongLiveCHIEF]

This would be a bug we need to o pen in https://github.com/EverlastingBugstopper/binary-install instead of rover, since the logic for this exists in that package.

[maschwenk]

@LongLiveCHIEF y'all have already inlined that dependency into your library. For reference I have just done the following for my purposes:

this.installDirectory = join(
      __dirname,
      '..',
      'node_modules',
      '.bin',
      `${this.roverBinaryName}-v${roverVersionFromConfig}`,
    );

where I just key the install location on the version name. pretty simple.

[LongLiveCHIEF]

There's a world of difference between "behaves as expected automatically" and "simple to implement a workaround". It may be simple, but it's also an extra step to be documented for others, and devs aren't great at reading documentation.

Now add a few dozen devs to the effort, and you'll have problems frequently with devs saying things aren't working because they missed the instructions to patch a dependency, especially when projects of this nature typically install/update and startup without having to do any manual setup.

[maschwenk]

To be clear we had to fork @apollo/rover internally for this issue and

• Binary install does not upgrade rover when npm version is upgraded #1563

so just sharing that that's what we did to get around this issue. What I'm saying is that y'all have already inlined binary-install into your library as of this PR

• deps: inline binary-install, with updated axios version #1819

so if y'all want to make changes here, you can just do it! no dependency on binary-install anymore.