-
Notifications
You must be signed in to change notification settings - Fork 9
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
[build-tools] [ENG-12652] display fingerprint sources diff is not the same #407
base: main
Are you sure you want to change the base?
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.
The changes and the addition of the log looks great. Just one note about using the package.
outputs.resolved_eas_update_runtime_version.set( | ||
resolvedRuntimeVersion.runtimeVersion ?? undefined | ||
); |
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.
This function will probably need to have a second output resolved_eas_update_fingerprint_sources
so we do exactly the same thing for custom builds as for standard builds
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.
The fingerprints are now stored in a GC bucket.
I think this means that the fingerprint diffing won't work on a local build because you won't be able to access the signed url (due to cors?).
I imagine it's ok to have this as an EAS cloud only feature though.
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.
Yeah, that's true, in its current form, it wouldn't work for local builds because there would be no place to generate the signed URL for the fingerprint to be downloaded (local builds don't go through Turtle and WWW).
Local builds use something called local-build-plugin
eas-build/packages/local-build-plugin/src/main.ts
Lines 1 to 23 in 69bbd3e
import chalk from 'chalk'; | |
import { parseInputAsync } from './parseInput'; | |
import { buildAsync } from './build'; | |
import { listenForInterrupts, shouldExit } from './exit'; | |
import { checkRuntimeAsync } from './checkRuntime'; | |
listenForInterrupts(); | |
async function main(): Promise<void> { | |
try { | |
const { job, metadata } = await parseInputAsync(); | |
await checkRuntimeAsync(job); | |
await buildAsync(job, metadata); | |
} catch (err: any) { | |
if (!shouldExit()) { | |
console.error(chalk.red(err.message)); | |
process.exit(1); | |
} | |
} | |
} | |
void main(); |
You can add support for it by basically not uploading the fingerprint to GCS when running the local build but copying it to some temp dir locally and then getting it from there during the build process.
That's what we do for project archive sources.
This will require supporting PATH
source for fingerprint.
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.
Back to your queue until we figure out what to do about the expiring URL.
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 more PR in the turtle-v2
repo will be needed to add support for generating the pre-signed download URL for the fingerprint source #407 (comment)
Generally, we will need to support 3 kinds of fingerprint sources:
GCS
- type created by EAS CLI when starting cloud build, it contains data aboutbucketKey
and is passed to WWW and then to Turtle launcher which will generate the pre-signed URL based on itURL
- pre-signed URL generated by launcher, passed to cloud workerPATH
- for local builds
Let me know if it's not clear or if you have any more questions
outputs.resolved_eas_update_runtime_version.set( | ||
resolvedRuntimeVersion.runtimeVersion ?? undefined | ||
); |
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.
Yeah, that's true, in its current form, it wouldn't work for local builds because there would be no place to generate the signed URL for the fingerprint to be downloaded (local builds don't go through Turtle and WWW).
Local builds use something called local-build-plugin
eas-build/packages/local-build-plugin/src/main.ts
Lines 1 to 23 in 69bbd3e
import chalk from 'chalk'; | |
import { parseInputAsync } from './parseInput'; | |
import { buildAsync } from './build'; | |
import { listenForInterrupts, shouldExit } from './exit'; | |
import { checkRuntimeAsync } from './checkRuntime'; | |
listenForInterrupts(); | |
async function main(): Promise<void> { | |
try { | |
const { job, metadata } = await parseInputAsync(); | |
await checkRuntimeAsync(job); | |
await buildAsync(job, metadata); | |
} catch (err: any) { | |
if (!shouldExit()) { | |
console.error(chalk.red(err.message)); | |
process.exit(1); | |
} | |
} | |
} | |
void main(); |
You can add support for it by basically not uploading the fingerprint to GCS when running the local build but copying it to some temp dir locally and then getting it from there during the build process.
That's what we do for project archive sources.
This will require supporting PATH
source for fingerprint.
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.
A few inline comments.
const localFingerprint = await result.json(); | ||
const easFingerprint = { | ||
hash: resolvedRuntimeVersion, | ||
sources: resolvedFingerprintSources as FingerprintSource[], |
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.
Might be cleaner to hoist this case all the way up to resolveRuntimeVersionAsync
and use FingerprintSource[]
instead of object[]
as types everywhere.
Optionally could also add validation in resolveRuntimeVersionAsync
to check that runtimeVersionResult.fingerprintSources
adheres to the expected type before returning it using a library like zod
. This would be a good stacked PR though as it's a bit outside the scope of this PR.
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!
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.