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

multidim: Reproducible js-libp2p #286

Closed
wants to merge 6 commits into from

Conversation

MarcoPolo
Copy link
Contributor

Only v0.46, with the thought that we'll soon release v0.47 and drop v0.45.

This solves the problem of the js-libp2p build here not being reproducible. This caused some considerable frustration from @marten-seemann . One problem is that js-libp2p does not commit it's package-lock.json file, and the interop image doesn't commit the lock file either. This means that a build that worked one day, may fail the next because of a subtle dependency change.

This solves the problem by committing the package-lock.json file into this repo. This file is copied into the build and serves to pin the exact versions of every dependency.

Moving forward, we should continue to include the package-lock.json in here until/unless we include one in js-libp2p.

@MarcoPolo MarcoPolo changed the title Reproducible js-libp2p multidim: Reproducible js-libp2p Aug 28, 2023
@MarcoPolo
Copy link
Contributor Author

MarcoPolo commented Aug 28, 2023

This doesn't make the test runs reproducible because playwright will fetch the latest version of the browser. But at least makes the builds reproducible (*mostly – installing cmake via apt-get is not technically reproducible).

@@ -3,9 +3,10 @@
FROM node:18
WORKDIR /app
COPY . .
RUN npm i && npm run build
RUN apt-get update && apt-get install -y cmake
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On arm64, there are no prebuilt binaries for some dependency, so the system needs cmake to build that dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

apt seems unhappy:

 => ERROR [4/7] RUN apt-get update && apt-get install -y cmake                                                                                                                                                       2.0s
------
 > [4/7] RUN apt-get update && apt-get install -y cmake:
1.347 Get:1 http://deb.debian.org/debian bookworm InRelease [151 kB]
1.583 Err:1 http://deb.debian.org/debian bookworm InRelease
1.583   At least one invalid signature was encountered.
1.685 Get:2 http://deb.debian.org/debian bookworm-updates InRelease [52.1 kB]
1.714 Get:3 http://deb.debian.org/debian-security bookworm-security InRelease [48.0 kB]
1.722 Err:2 http://deb.debian.org/debian bookworm-updates InRelease
1.722   At least one invalid signature was encountered.
1.906 Err:3 http://deb.debian.org/debian-security bookworm-security InRelease
1.906   At least one invalid signature was encountered.
1.911 Reading package lists...
1.927 W: GPG error: http://deb.debian.org/debian bookworm InRelease: At least one invalid signature was encountered.
1.927 E: The repository 'http://deb.debian.org/debian bookworm InRelease' is not signed.
1.927 W: GPG error: http://deb.debian.org/debian bookworm-updates InRelease: At least one invalid signature was encountered.
1.927 E: The repository 'http://deb.debian.org/debian bookworm-updates InRelease' is not signed.
1.927 W: GPG error: http://deb.debian.org/debian-security bookworm-security InRelease: At least one invalid signature was encountered.
1.927 E: The repository 'http://deb.debian.org/debian-security bookworm-security InRelease' is not signed.
------
Dockerfile:6
--------------------
   4 |     WORKDIR /app
   5 |     COPY . .
   6 | >>> RUN apt-get update && apt-get install -y cmake
   7 |     RUN npm ci && npm run build
   8 |
--------------------
ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get install -y cmake" did not complete successfully: exit code: 100
make: *** [image.json] Error 1

@MarcoPolo
Copy link
Contributor Author

Had to specify tlsv1.3 explicitly for wget: https://github.com/orgs/community/discussions/65227

@@ -2,7 +2,9 @@ image_name := js-v0.46
commitSha := b7e608998cc88860d9ec8a3ed7c03fdfb3eccb3b
Copy link
Member

@maschad maschad Aug 29, 2023

Choose a reason for hiding this comment

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

I would suggest updating this to the latest js-libp2p

Suggested change
commitSha := b7e608998cc88860d9ec8a3ed7c03fdfb3eccb3b
commitSha := 1511105774db3c3c7707809b0262e57bdfa544d8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s do in a follow up pr.

This one should just be a fix. I don’t want to debug adding a new version.

Feel free to create a new pr off this one though

@maschad
Copy link
Member

maschad commented Aug 29, 2023

Whilst this does solve the problem of making the js-builds more reproducible I do think creating an npm package for multdim interop would not only make builds faster but also remove the need for some unnecessary dependencies i.e. a breakage in kad-dht or @libp2p/crypto shouldn't affect whether the js interop test can be built.

It could be done in a follow up PR if there is agreement

@MarcoPolo
Copy link
Contributor Author

Whilst this does solve the problem of making the js-builds more reproducible I do think creating an npm package for multdim interop would not only make builds faster but also remove the need for some unnecessary dependencies i.e. a breakage in kad-dht or @libp2p/crypto shouldn't affect whether the js interop test can be built.

It could be done in a follow up PR if there is agreement

I disagree. It’s very useful in debugging to have access to the source.

@maschad
Copy link
Member

maschad commented Aug 29, 2023

I disagree. It’s very useful in debugging to have access to the source.

What exactly do you mean by source? All of the dependencies of the interop test would be installed to the node_modules folder upon doing an npm i so they could be accessed there if needs be.

@marten-seemann
Copy link
Contributor

One problem is that js-libp2p does not commit it's package-lock.json file, and the interop image doesn't commit the lock file either. This means that a build that worked one day, may fail the next because of a subtle dependency change.

I'm not a JS dev, but that sounds like a surprising decision to me. Why would you not commit the lock file? You're just setting yourself up for non-reproducible build failures, like the one I ran into.

@maschad
Copy link
Member

maschad commented Aug 29, 2023

Why would you not commit the lock file?

There was a discussion here and it was decided that the dependency issues would be resolved by the change to a monorepo

I do think there is value in committing the lockfile.

@marten-seemann
Copy link
Contributor

There was a discussion here and it was decided that the dependency issues would be resolved by the change to a monorepo

... assuming you have no 3rd party dependencies.

@maschad
Copy link
Member

maschad commented Aug 29, 2023

FYI: turns out the changes to wget aren't even necessary any more since the Git Protocols team has a more specific restriction pattern

@@ -3,9 +3,10 @@
FROM node:18
WORKDIR /app
COPY . .
RUN npm i && npm run build
RUN apt-get update && apt-get install -y cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

apt seems unhappy:

 => ERROR [4/7] RUN apt-get update && apt-get install -y cmake                                                                                                                                                       2.0s
------
 > [4/7] RUN apt-get update && apt-get install -y cmake:
1.347 Get:1 http://deb.debian.org/debian bookworm InRelease [151 kB]
1.583 Err:1 http://deb.debian.org/debian bookworm InRelease
1.583   At least one invalid signature was encountered.
1.685 Get:2 http://deb.debian.org/debian bookworm-updates InRelease [52.1 kB]
1.714 Get:3 http://deb.debian.org/debian-security bookworm-security InRelease [48.0 kB]
1.722 Err:2 http://deb.debian.org/debian bookworm-updates InRelease
1.722   At least one invalid signature was encountered.
1.906 Err:3 http://deb.debian.org/debian-security bookworm-security InRelease
1.906   At least one invalid signature was encountered.
1.911 Reading package lists...
1.927 W: GPG error: http://deb.debian.org/debian bookworm InRelease: At least one invalid signature was encountered.
1.927 E: The repository 'http://deb.debian.org/debian bookworm InRelease' is not signed.
1.927 W: GPG error: http://deb.debian.org/debian bookworm-updates InRelease: At least one invalid signature was encountered.
1.927 E: The repository 'http://deb.debian.org/debian bookworm-updates InRelease' is not signed.
1.927 W: GPG error: http://deb.debian.org/debian-security bookworm-security InRelease: At least one invalid signature was encountered.
1.927 E: The repository 'http://deb.debian.org/debian-security bookworm-security InRelease' is not signed.
------
Dockerfile:6
--------------------
   4 |     WORKDIR /app
   5 |     COPY . .
   6 | >>> RUN apt-get update && apt-get install -y cmake
   7 |     RUN npm ci && npm run build
   8 |
--------------------
ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get install -y cmake" did not complete successfully: exit code: 100
make: *** [image.json] Error 1


WORKDIR /app/interop
RUN npm i && npm run build
RUN npm ci && npm run build
Copy link
Contributor

Choose a reason for hiding this comment

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

Skipping the apt-get step due to the failure from above, this leads to an error (not sure if related or not):

 > [4/6] RUN npm ci && npm run build:
7.995 npm WARN deprecated workbox-cacheable-response@6.6.0: workbox-background-sync@6.6.0
9.757 npm WARN deprecated w3c-hr-time@1.0.2: Use your platform's native performance.now() and performance.timeOrigin.
11.17 npm WARN deprecated svgo@1.3.2: This SVGO version is no longer supported. Upgrade to v2.x.x.
11.35 npm WARN deprecated stable@0.1.8: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
11.48 npm WARN deprecated sourcemap-codec@1.4.8: Please use @jridgewell/sourcemap-codec instead
12.27 npm WARN deprecated rollup-plugin-terser@7.0.2: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-terser
15.84 npm WARN deprecated multibase@4.0.6: This module has been superseded by the multiformats module
32.32 npm WARN deprecated ipfs-core-utils@0.18.1: js-IPFS has been deprecated in favour of Helia - please see https://github.com/ipfs/js-ipfs/issues/4336 for details
32.63 npm WARN deprecated ipfs-core-types@0.14.1: js-IPFS has been deprecated in favour of Helia - please see https://github.com/ipfs/js-ipfs/issues/4336 for details
115.4 npm notice
115.4 npm notice New minor version of npm available! 9.6.7 -> 9.8.1
115.4 npm notice Changelog: <https://github.com/npm/cli/releases/tag/v9.8.1>
115.4 npm notice Run `npm install -g npm@9.8.1` to update!
115.4 npm notice
115.4 npm ERR! code 2
115.4 npm ERR! path /app/node_modules/node-datachannel
115.4 npm ERR! command failed
115.4 npm ERR! command sh -c prebuild-install || (npm install --ignore-scripts --production=false && npm run _prebuild)
115.4 npm ERR! added 602 packages, removed 1 package, and audited 660 packages in 50s
115.4 npm ERR!
115.4 npm ERR! 68 packages are looking for funding
115.4 npm ERR!   run `npm fund` for details
115.4 npm ERR!
115.4 npm ERR! 10 vulnerabilities (6 moderate, 4 high)
115.4 npm ERR!
115.4 npm ERR! To address issues that do not require attention, run:
115.4 npm ERR!   npm audit fix
115.4 npm ERR!
115.4 npm ERR! To address all issues (including breaking changes), run:
115.4 npm ERR!   npm audit fix --force
115.4 npm ERR!
115.4 npm ERR! Run `npm audit` for details.
115.4 npm ERR!
115.4 npm ERR! > node-datachannel@0.4.3 _prebuild
115.4 npm ERR! > prebuild --backend cmake-js
115.4 npm ERR!
115.4 npm ERR! [
115.4 npm ERR!   '/usr/local/bin/node',
115.4 npm ERR!   '/app/node_modules/node-datachannel/node_modules/.bin/cmake-js',
115.4 npm ERR!   'rebuild',
115.4 npm ERR!   '--runtime-version=18.17.1',
115.4 npm ERR!   '--arch=arm64',
115.4 npm ERR!   '--runtime=node'
115.4 npm ERR! ]
115.4 npm ERR! prebuild-install WARN install libcrypto.so.1.1: cannot open shared object file: No such file or directory
115.4 npm ERR! npm WARN config production Use `--omit=dev` instead.
115.4 npm ERR! npm WARN deprecated w3c-hr-time@1.0.2: Use your platform's native performance.now() and performance.timeOrigin.
115.4 npm ERR! npm WARN deprecated har-validator@5.1.5: this library is no longer supported
115.4 npm ERR! npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
115.4 npm ERR! npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
115.4 npm ERR! npm WARN deprecated tar@2.2.2: This version of tar is no longer supported, and will not receive security updates. Please upgrade asap.
115.4 npm ERR! npm WARN deprecated tar@2.2.2: This version of tar is no longer supported, and will not receive security updates. Please upgrade asap.
115.4 npm ERR! prebuild info begin Prebuild version 11.0.4
115.4 npm ERR! prebuild info build Preparing to prebuild node-datachannel@0.4.3 for node 18.17.1 on linux-arm64 using cmake-js
115.4 npm ERR! info TOOL Using Unix Makefiles generator.
115.4 npm ERR! info DIST Downloading distribution files to: /root/.cmake-js/node-arm64/v18.17.1
115.4 npm ERR! http DIST 	- https://nodejs.org/dist/v18.17.1/SHASUMS256.txt
115.4 npm ERR! http DIST 	- https://nodejs.org/dist/v18.17.1/node-v18.17.1-headers.tar.gz
115.4 npm ERR! ERR! OMG CMake executable is not found. Please use your system's package manager to install it, or you can get installers from there: http://cmake.org.
115.4 npm ERR! prebuild ERR! build Error: Failed to build cmake with exit code 1
115.4 npm ERR! prebuild ERR! build     at ChildProcess.<anonymous> (/app/node_modules/node-datachannel/node_modules/prebuild/cmakebuild.js:32:19)
115.4 npm ERR! prebuild ERR! build     at ChildProcess.emit (node:events:514:28)
115.4 npm ERR! prebuild ERR! build     at ChildProcess._handle.onexit (node:internal/child_process:291:12)
115.4
115.4 npm ERR! A complete log of this run can be found in: /root/.npm/_logs/2023-08-30T08_32_58_438Z-debug-0.log
------
Dockerfile:7
--------------------
   5 |     COPY . .
   6 |     # RUN apt-get update && apt-get install -y cmake
   7 | >>> RUN npm ci && npm run build
   8 |
   9 |     WORKDIR /app/interop
--------------------
ERROR: failed to solve: process "/bin/sh -c npm ci && npm run build" did not complete successfully: exit code: 2
make: *** [image.json] Error 1

Copy link
Member

Choose a reason for hiding this comment

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

The failing apt install step above is trying to install cmake. As it's skipped the npm build step then duly fails with:

115.4 npm ERR! ERR! OMG CMake executable is not found. Please use your system's package manager to install it, or you can get installers from there: http://cmake.org./

Looks like apt-get fails because there's a GPG signature failure?

The internet seems to think this can happen when a mirror is out of date or not completely synced?

Copy link
Member

@maschad maschad Aug 30, 2023

Choose a reason for hiding this comment

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

whilst this build issue could be resolved, as @achingbrain and I have mentioned the build should be done ideally through the use of the npm package. We are not even testing webRTC at the moment even though it's dependent i.e. node-datachannels is what's causing this build failure, and so I don't think that a consumer should be required to build a dep that is not being used by the package.

I've opened libp2p/js-libp2p#2006 and #288 to address this.

@marten-seemann Could you try adjusting your Docker dedicated resources as suggested to see if this addresses the GPG signature failure? Ultimately you need cmake install to build the pre-binaries for node-datachannels

@achingbrain
Copy link
Member

I think @maschad is right here, this repo should really use published versions of js-libp2p (and friends) instead of building from source every time. In my head at least, the only place we should be building js-libp2p from source while running multidim interop tests is during a js-libp2p CI run.

Lockfile discussion aside it'll make things a lot faster as the move to a monorepo means there's a lot more to be done before you have runnable code, plus it ends up building a lot of stuff that won't be used, pulling down development dependencies, all sorts.

The ts source code is also bundled with the modules too so you still have access for debugging.

This repo is, if you like, an application that uses js-libp2p so it should contain a lockfile so glad to see it's being added here.

@p-shahi
Copy link
Member

p-shahi commented Sep 6, 2023

closing in favor of #288

@p-shahi p-shahi closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants