-
Notifications
You must be signed in to change notification settings - Fork 987
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
Status 💛 Yarn. Lock dependencies. #6928
Conversation
Pull Request Checklist
|
In order to keep determinism in line, you have to use |
also, great work @mandrigin!! |
Yeah, I was thinking about it. we need to block PRs when the yarn.lock is changed. |
Okay, it looks like 2 parallel yarn commands can conflict if running on the same machine. Custom cache path will help with that, even though it will 2x increase a disk space per machine... That is only required for Jenkins. |
We have plenty of space on jenkins machines, no worries. |
But this also means I will have to upgrade node on all linux and macos hosts for Jenkins to 9.3.0. |
why is that? it looks like yarn works just fine with 8.x that is installed. I need to double-check that Desktop is buildable on 9.3.0. |
I guess if it works we can leave it as it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Great work.
98% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (58)Click to expand |
@mandrigin, each build #6928 (comment) tested. |
Please be sure to update this docs issue once this is merged 🎉 https://github.com/status-im/status.im/issues/51 |
Wow, awesome work, let's hope this one goes through! We've had a bunch of attempts at getting yarn into the code base, so really impressive to see this one progressing so quickly and smoothly (and judging by PR checklist, with all the required changed). Fingers crossed :) |
I would like to check determinism of the resulting builds. I'm happy to see this done quickly and smoothly, but we need to see if we're getting the same resulting hash across multiple platform builds. |
@corpetty it doesn't guarantee determinism in the whole build, because we use other package managers in the build chain too (like CocoaPods), they can affect that. |
@mandrigin we could at least hash the part we've changed to see if that is deterministic, then we know we can work forward from there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can worry about pushing determinism forward from here. For now, these changes are great and help multiple things. Great job @mandrigin
@corpetty you mean the dependencies? hashes of all of them are in the lock file, and Regarding the other parts of the toolchain, they are out of what |
@mandrigin, what kind of issues you had with |
@vkjr it looks like they were cause by the old |
Great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch for doing this! Great job!
833a6dd
to
955db25
Compare
Signed-off-by: Igor Mandrigin <i@mandrigin.ru>
955db25
to
37ef82b
Compare
One more step towards reproducible builds.
yarn (as opposed of npm) promises integrity checks and using lock files to download dependencies.
That way, we can avoid security issues with dependencies, just like this one.
Notable change: I had to rename
package-lock.json
in desktop_files and mobile_files topackage-lock.json.orig
. That is done to avoid duplicate symbol error when bundling JS.🚨 🚨 🚨 We use
yarn install --frozen-lockfile
command. It will fail if the lockfile will change. Useyarn upgrade
to upgrade the lockfileadd custom cache folders per runner (so 2 parallel jobs can't conflict)not needed as soon as the cache is downloadedyarn
installedyarn
everywhere elsereact-native-webview-bridge
error--
fixes #6906
fixes #3838
status: ready