URLs npm prefers to use for jsbeeb break CI about bbcmicrobot HOT 25 CLOSED

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.

from bbcmicrobot.

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

from bbcmicrobot.

(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.

from bbcmicrobot.

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?

from bbcmicrobot.

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....

from bbcmicrobot.

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://[email protected]/mattgodbolt/jsbeeb.git#78eeeb3299c4640ca709df189c76d7447977258f",
package-lock.json:      "version": "git+ssh://[email protected]/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?

from bbcmicrobot.

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.

from bbcmicrobot.

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.

from bbcmicrobot.

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.

from bbcmicrobot.

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...

from bbcmicrobot.

Ahh! npm install not npm update!

from bbcmicrobot.

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!"

from bbcmicrobot.

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

from bbcmicrobot.

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.

from bbcmicrobot.

in this repo it seems to regenerate certificates

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

from bbcmicrobot.

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 :)

from bbcmicrobot.

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.

from bbcmicrobot.

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

from bbcmicrobot.

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

from bbcmicrobot.

#58

from bbcmicrobot.

By all means use that^ or whatever else :)

from bbcmicrobot.

Meanwhile, the "npm rewrites our git remote URLs and breaks CI" bug seems to be npm/cli#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) [[email protected]](mailto:[email protected]):
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.

from bbcmicrobot.

@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...

from bbcmicrobot.

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 :)

from bbcmicrobot.

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.

from bbcmicrobot.