-
Notifications
You must be signed in to change notification settings - Fork 85
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
[eas-cli] [ENG-12650] add fingerprint sources to build metadata #2422
Conversation
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.
I think the build-tools PR will need to be deployed first (and maybe even the www one) but I'm not super familiar with the push safety between these repos as far as the build job validation goes.
@wschurman you're right, this is what the package dependencies look like: It seems like actually I might have to break the So the order would be:
|
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.
It seems like actually I might have to break the @expo/eas-build-job into a separate PR so it could be merged separately since the dependency is used in @expo/build-tools.
So the order would be:
@expo/eas-build-job -> merge and publish
@expo/build-tools -> add dependency (@expo/eas-build-job), merge and release
turtle worker -> add dependencies (@expo/eas-build-job and @expo/build-tools), merge and deploy
www -> merge and deploy
eas-cli -> add dependency (@expo/eas-build-job), merge and release
Re 1
and 2
: The eas-build
repo is set up in a way that packages from eas-build
use local versions of other packages from the same repo by default, so you just need to rebuild the @expo/eas-build-job
package to be able to use it in the @expo/build-tools
. You don't even need to split the @expo/eas-build-job
and @expo/build-tools
changes into 2 PRs. You can just merge it all together and once you run yarn release
our release script will publish correct versions in correct order and bump everything in package.json
.
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.
Oh, looking at https://github.com/expo/universe/pull/15517#pullrequestreview-2113659688 I believe that the fingerprint file upload to GCS should happen in CLI (same as we do for project archive and build metadata) and we should just pass a data necessary to link uploaded file with build to mutation
Also one more question: What about builds triggered with GH trigger? We don't install node modules nor run expo updates CLI before starting the GH build. We run EAS CLI inside the build process to resolve and backfill metadata at the beginning of the build process in the Will it be compatible with your changes? I mean in that case both "before" and "after" fingerprints will come from the build worker then (but from different build phases). |
Updated the PR to store fingerprints in Google cloud instead. Regarding your question on builds triggered via the GH @szdziedzic - I don't know for sure, but if the project runtime is currently calculated correctly on GH builds for projects that use fingerprint then this should work as well. |
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.
One minor change to naming and a question, otherwise looks good.
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.
Code looks good. Double check CI failures before landing. And also ensure the other PRs land ahead of this.
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.
If comments from expo/eas-build#407 (review) end up being applicable we will probably also need to change metadata structure here
Size Change: -1 kB (0%) Total Size: 52 MB
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
- Coverage 53.47% 53.44% -0.02%
==========================================
Files 530 530
Lines 19508 19552 +44
Branches 3958 3968 +10
==========================================
+ Hits 10430 10448 +18
- Misses 8327 8350 +23
- Partials 751 754 +3 ☔ View full report in Codecov by Sentry. |
I think this should do it @szdziedzic ! Tested both iOS and Android on both On EAS
With
|
Android | iOS |
---|---|
✅ Thank you for adding the changelog entry! |
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.
awesome! good job! 🙌 🚀
@kadikraman Hi Kadi, thanks for this amazing work. I still see the old message:
My env info:
Am I missing something? |
@giorgiofellipe - please file a new issue with steps to reproduce the issue you're seeing. Fingerprint is still in beta (appreciate you testing it out!), so we're still working out the kinks, and a step-by-step set of repro steps is the easiest way to communicate the issue and get to a fix. |
@kadikraman Doesn't work with |
@tfcornerstone - please file a new issue. See #2422 (comment) |
Related PRs
Why
It's very difficult to debug why the local and remote fingerprint are different.
Currently the only way to do it would be manual: add step to print out the remote sources, calculate local fingerprint, diff the two. Ideally we'd like to show the diff in the build step instead.
How
Save the local fingerprint in a google cloud bucket and show a diff if the fingerprints are different.
Test Plan
Ran turtle and www locally with changes from the these packages manually linked to turtle.