URLs npm prefers to use for jsbeeb break CI

[ojwb]

I've pushed changes to the mastodon branch and the testsuite now passes again for me locally.

However it fails in CI like so:

Run npm ci
  npm ci
  shell: /usr/bin/bash -e {0}
npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t ssh://git@github.com/mattgodbolt/jsbeeb.git
npm ERR! 
npm ERR! git@github.com: Permission denied (publickey).
npm ERR! fatal: Could not read from remote repository.
npm ERR! 
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR! 
npm ERR! exited with error code: 128

However package.json says:

package.json:    "jsbeeb": "https://github.com/mattgodbolt/jsbeeb.git#78eeeb3299c4640ca709df189c76d7447977258f",

It seems npm thinks it's so smart and knows better than us, so it rewrites that to an ssh URL, which then doesn't work. I've read the help but can't work out how to stop it doing this.

Any idea how to fix this? It's frustrating to be so close to having this working once more.

Perhaps we should just rewrite the bot in BBC BASIC so it can be self hosting...

[mattgodbolt]

what the heck! that's bonkers. Is...there also like a git submodule going on too?

[mattgodbolt]

(does'nt look like it) Tht's super odd! I've not seen that before! Darn it, sorry @ojwb -- maybe I need to "release" a proper npm package somehow rather than rely on a nasty sha'd thing.

[mattgodbolt]

Looks like the package-lock.json has it: https://github.com/8bitkick/BBCMicroBot/blob/mastodon/package-lock.json#L2200

Mayyyybe deleting that line (and others) and then npm install again will rewrite them properly?

[ojwb]

Oh, that would probably explain it. Thanks, will try that.

Is it normal to have package-lock.json under version control? I almost always seem to end up with a different version locally which seems a clumsy way to have to work.

In case it's not obvious, I know very little about the node ecosystem. Maybe I'm running the wrong npm commands....

[ojwb]

If I remove package-lock.json and node_modules and run npm install it creates a new package-lock.json with the problematic URLs:

package-lock.json:      "resolved": "git+ssh://git@github.com/mattgodbolt/jsbeeb.git#78eeeb3299c4640ca709df189c76d7447977258f",
package-lock.json:      "version": "git+ssh://git@github.com/mattgodbolt/jsbeeb.git#78eeeb3299c4640ca709df189c76d7447977258f",

So it does seem this that npm is causing this issue.

If I hand-modify these to the desired git+https URL then npm ci (which CI runs) doesn't change it back, but this seems brittle as developer actions followed by checking in an updated package-lock.json could easily break this.

I guess I could get the CI to hit package-log.json with a sed hammer to change these instead. Or maybe get git+ssh to actually work in CI.

@mattgodbolt Or perhaps a proper npm release for jsbeeb would be better. Is that easy to do?

[ojwb]

CI is no longer failing on this, so this is no longer a blocker at least. I think this really needs fixing in a more satisfactory way though.

[ojwb]

And CI is now even passing! (I had to disable a few testcases for various reasons, which I'll now try to resolve.)

Retitling to reflect what this is actually about.

[mattgodbolt]

Is it normal to have package-lock.json under version control? I almost always seem to end up with a different version locally which seems a clumsy way to have to work.

Absolutely! This is how we can have an underspecified package.json (yay, specifies our assumptions) but yet have reproducible builds (the lock file), and then it's opt in to update versions with npm update instead of "pot luck you get whatever the internet has to offer every time you build!"

I almost always seem to end up with a different version locally which seems a clumsy way to have to work.

Ah, that sounds like a problem - next time you get a local modification can you let us know, and we can try and diagnose why that's happening.

[ojwb]

I think it's when a new dependency is added and I run npm update assuming that's the way to sort things out for it. I'm guessing there's a different command that I should run instead in this situation...

[mattgodbolt]

Ahh! npm install not npm update!

[mattgodbolt]

If you run npm update you're saying "hey, go to the internet, find all the new package versions that match the package.json, solve and then update them. Then install those new versions and update package-lock.json so that everyone else on this same project gets the new versions too!"

But npm install says "hey, install the things the package.json says, using the versions in package-lock.json!"

[mattgodbolt]

https://stackoverflow.com/questions/12478679/npm-install-vs-update-whats-the-difference covers this :)

[ojwb]

I tried npm install but in this repo it seems to regenerate certificates which didn't seem like it was the right thing to do. Maybe that's a bug in the setup here though.

[mattgodbolt]

in this repo it seems to regenerate certificates

oh crikey. I'll take a look... but also I should defer to @8bitkick on this :)

[mattgodbolt]

I realise I've never looked at this project. Looks like npm install builds beebjit, and makes certificates. but applies cleanly. After git cloneing and npm install I have no local changes.

Looks like there's an install script that unconditionally does the certificate thing every time, which is a bit of a pain but harmless. Takes 17s to do that on my laptop. I'll leave it to Dom :)

[ojwb]

Looks like it's code in install.sh:

echo Creating directories
########################
mkdir tmp
mkdir certs

echo Generating certificates
############################
cd certs

openssl req -x509 -newkey rsa:4096 -keyout server_key.pem -out server_cert.pem -nodes -days 365 -subj "/CN=localhost/O=BBC\ Micro\ Bot"
openssl req -newkey rsa:4096 -keyout client_key.pem -out client_csr.pem -nodes -days 365 -subj "/CN=Emulator Client"
openssl x509 -req -in client_csr.pem -CA server_cert.pem -CAkey server_key.pem -out client_cert.pem -set_serial 01 -days 365
chmod og-rwx *

It should probably skip that if certs already exists.

Also not totally ideal to set the permissions after generating the certificate.

[mattgodbolt]

Right; agreed there! I get:

mkdir: cannot create directory ‘tmp’: File exists
mkdir: cannot create directory ‘certs’: File exists

so those should probably be mkdir -p or like you say, create if not exist, type things

[mattgodbolt]

also the lack of set -euo pipefail means if any of those commands fail it plays on :) which...probasbly isnt' ok

[ojwb]

It'll also re-download the GXR every time (and thanks to the lack of set -e if that fails it'll just stomp on the existing working ROM image and ignore the error and keep going).

Dom seems really busy so I might just clean this up.

[mattgodbolt]

https://github.com/8bitkick/BBCMicroBot/pull/58

[mattgodbolt]

By all means use that^ or whatever else :)

[ojwb]

Meanwhile, the "npm rewrites our git remote URLs and breaks CI" bug seems to be https://github.com/npm/cli/issues/2610 which seems a somewhat long and sorry tale.

Someone there suggested getting git to rewrite the remote URL back to https, e.g. with:

git config --global url."[https://github.com/".insteadOf](https://github.com/%22.insteadOf) [git@github.com](mailto:git@github.com):
git config --global url."https://".insteadOf git://

That seems a plausible workaround for our situation - we could just do that at the start of each CI run, while people checking out the repo would get the ssh rewrite which is likely to work OK for them.

[8bitkick]

@ojwb @mattgodbolt Just go with whatever fix works I trust you! (probably best thing is to skip certgen if they already exist)

Yeah the install script is not good for dev (assumes deploy and run scenario). But you can donpm install <module name> if you're just trying to install a specific module...

[mattgodbolt]

The point here is when someone git pulls the repo and then wants to ensure they have the dependencies installed. That should be npm install :). And if you don't know what changed (and you don't have a system like make checking), then the wisdom is just do an install after every pull.

Not talking about installing a new package here :)

[ojwb]

probably best thing is to skip certgen if they already exist

We do now (#58 implemented that, and other laziness).

I think I'm going to add the git config workaround for CI, as otherwise we need to fix up package-lock.json by hand every time we commit changes that npm makes to it, and which would mean this issue no longer causes problems.