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(multidim): make js-libp2p builds reproducible #288

Merged
merged 10 commits into from
Sep 7, 2023

Conversation

maschad
Copy link
Member

@maschad maschad commented Aug 30, 2023

Uses the @libp2p/multidim-interop for interop tests.

Depends on libp2p/js-libp2p#2023

Closes #279

@p-shahi
Copy link
Member

p-shahi commented Sep 5, 2023

I just re-triggered failing CI jobs (which ran 4 days ago and failed because they couldn't find @libp2p/multidim-interop v1.0.0; resolved per libp2p/js-libp2p#2008 (comment))

Now failing due to:

#5 [2/3] WORKDIR /app
#5 DONE 0.0s

#6 [3/3] RUN npm run build
#6 0.599 npm ERR! code ENOENT
#6 0.601 npm ERR! syscall open
#6 0.601 npm ERR! path /app/package.json
#6 0.602 npm ERR! errno -2
#6 0.603 npm ERR! enoent ENOENT: no such file or directory, open '/app/package.json'

Assuming this PR supplants #286 - want to close that (once CI is green and we have reproducible builds here)

@maschad maschad marked this pull request as ready for review September 6, 2023 01:03
@maschad
Copy link
Member Author

maschad commented Sep 6, 2023

@p-shahi the build required multidim interop to be published with aegir as a dependency once that was resolved I had intended to test it locally then push my changes, which I have done now.

Copy link
Member

@p-shahi p-shahi left a comment

Choose a reason for hiding this comment

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

Why aren't you including the package-lock.json that Marco added here: https://github.com/libp2p/test-plans/pull/286/files#diff-91f1832c796ab4c1041210aeb8f5d0d179090661044a631ac2919098e1338b07

Everyone was in agreement that it's important to add here since js-libp2p monorepo doesn't have a lock file. Do we have a reason for not including it?

multidim-interop/impl/js/v0.46/Makefile Show resolved Hide resolved
@maschad
Copy link
Member Author

maschad commented Sep 6, 2023

Why aren't you including the package-lock.json that Marco added here: #286 (files)

Everyone was in agreement that it's important to add here since js-libp2p monorepo doesn't have a lock file. Do we have a reason for not including it?

My original thought was that we would fix the dependencies in the @libp2p/multidim-interop package given js-libp2p controls the releases but upon reconsideration it may be more maintainable to commit the lockfile.

@maschad maschad requested a review from p-shahi September 6, 2023 16:13
@maschad
Copy link
Member Author

maschad commented Sep 6, 2023

Unrelated failure between two rust v0.52 nodes on webRTCDirect. @mxinden have you seen this type of failure before?

@p-shahi
Copy link
Member

p-shahi commented Sep 6, 2023

My original thought was that we would fix the dependencies in the @libp2p/multidim-interop package given js-libp2p controls the releases but upon reconsideration it may be more maintainable to commit the lockfile.

Sorry I don't understand how "fix the dependencies in the @libp2p/multidim-interop package" would solve the build reproducibility issue described here
"One problem is that js-libp2p does not commit it's libp2p/js-libp2p#1724, 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 might be missing something on how JS specific package management works though🤔 ?

@maschad
Copy link
Member Author

maschad commented Sep 6, 2023

Sorry I don't understand how "fix the dependencies in the @libp2p/multidim-interop package" would solve the build reproducibility issue described #286 (comment)

Perhaps the use of the word "fix" in this context was a bit misleading, if the dependencies of @libp2p/multdim-interop are set to a static version e.g. "0.46.9" as opposed to a version range then npm will download specifically that version, which is the equivalent behaviour of a lockfile and thus reproducible.

Given we maintain the @libp2p/multdim-interop repo and it's dependencies, and it is only consumed by this repository, the versions can be set for this use case as opposed to other packages which have multiple consumers.

The downside is that this is requires manually updating each dependency in that package (thus more maintenance work), the upside is that we have more granular control over which dependencies change in the next package (as opposed to a new @libp2p/multidim-interop package which may have multiple dependency updates).

maschad and others added 3 commits September 6, 2023 17:43
Co-authored-by: Prithvi Shahi <shahi.prithvi@gmail.com>
Co-authored-by: Prithvi Shahi <shahi.prithvi@gmail.com>
@maschad maschad requested a review from p-shahi September 6, 2023 23:20
@p-shahi p-shahi changed the title fix: use multidim interop package to run js builds fix(multidim): make js-libp2p builds reproducible Sep 6, 2023
@p-shahi
Copy link
Member

p-shahi commented Sep 7, 2023

@marten-seemann can you verify this branch builds js-libp2p for you; otherwise it shouldn't be merged

@marten-seemann
Copy link
Contributor

@marten-seemann can you verify this branch builds js-libp2p for you; otherwise it shouldn't be merged

Works on my machine.

@p-shahi p-shahi merged commit 43b67cc into libp2p:master Sep 7, 2023
2 checks passed
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.

interop: building js-libp2p v0.46 image fails
3 participants