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

[expo-updates] Add fingerprintExperimental runtime version policy #24126

Merged
merged 24 commits into from Sep 11, 2023
Merged

[expo-updates] Add fingerprintExperimental runtime version policy #24126

merged 24 commits into from Sep 11, 2023

Conversation

mccraveiro
Copy link
Contributor

@mccraveiro mccraveiro commented Aug 27, 2023

Why

Manually handling runtimeVersions is a big pain for projects that uses native modules. This PR adds a new runtime policy that handles this automatically.

See:
https://discord.com/channels/695411232856997968/1142023605908156476/1142023605908156476

How

Using expo-fingerprint to auto-generate a hash for the project.

Test Plan

I made the changes locally on a working project and the build was successfully with expo prebuild. I checked the native files and the runtime version was set to the project hash.

Checklist

@ItsWendell
Copy link

Nice this would definitely be cool to have! Here are some interesting, relevant conversations around @expo/fingerprint:

#24069

It still has some edge cases (e.g. monorepos), but it's getting there! In a script that I wrote, I do something similar as you do here but then store the hash in a json file which I then exclude from the fingerprint, and import in my app.config.ts file.

The beauty of using @expo/fingerprint is that it outputs its sources and the individual hash per source, so you could use this to 'tweak' the hash to be optimal for runtime versions.

E.g. do you want your eas.json file or your package.json scripts to be included in the hash for your runtime version, I think what is included in your fingerprint for runtime versions is something to think about, document and discuss, and to allow possibly for customizations where needed.

@brentvatne brentvatne requested review from Kudo, wschurman and quinlanj and removed request for Simek, byCedric, EvanBacon and amandeepmittal August 29, 2023 22:22
Copy link
Member

@quinlanj quinlanj left a comment

Choose a reason for hiding this comment

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

This is a good first pass! The fingerprint policy is also on our roadmap. Reading the discord and PR thread, here is what the fingerprint runtime policy could look like as it becomes more mature:

Features

  • Users should be able to audit their fingerprint and understand it’s structure. It'll be helpful for folks to see the input sources that go into generating the fingerprint. Possible solution: write fingerprint.json to project directory every time we generate runtime
  • Ability to customize input sources. @ItsWendell does this in a script, but when we expose a fingerprint policy, we should make it easier for folks to achieve the same thing. Possible solution: expose an .ignore file for fingerprint

Stability

A couple concerns about releasing it to production today.

  • fingerprint package is still quite new. We still need to dogfood the edge cases and ensure we can produce stable hashes under different conditions (ie) monorepos, different OS’es, etc
  • An option is to tell users about the experimental nature of the fingerprint policy in the docs and in a Log.warn in eas-cli.

packages/@expo/config-plugins/src/utils/Updates.ts Outdated Show resolved Hide resolved
@quinlanj quinlanj self-requested a review August 30, 2023 21:21
@Kudo
Copy link
Contributor

Kudo commented Aug 31, 2023

Possible solution: expose an .ignore file for fingerprint

that's great idea. i will take this one for fingerprint.

no async getRuntimeVersionNullable

since this pr changes the getRuntimeVersionNullable interface, how about just introducing new getRuntimeVersionNullableAsync and deprecating the original function? maybe it's safer than just changing the original interface for eas-cli integration.

@mccraveiro
Copy link
Contributor Author

Ability to customize input sources. @ItsWendell does this in a script, but when we expose a fingerprint policy, we should make it easier for folks to achieve the same thing. Possible solution: expose an .ignore file for fingerprint

@quinlanj I'm not sure I get the use case where someone would customize the files that goes into the fingerprint. On @ItsWendell's script this seems to be the case because of an underlining bug in expo-fingerprint. Not sure we should plan around that.

An option is to tell users about the experimental nature of the fingerprint policy in the docs and in a Log.warn in eas-cli.

@quinlanj I agree 100% with this. I'll update the code and documentation.

since this pr changes the getRuntimeVersionNullable interface, how about just introducing new getRuntimeVersionNullableAsync and deprecating the original function? maybe it's safer than just changing the original interface for eas-cli integration.

@Kudo I'm not sure this achieves the desired goal. As stated on Expo's Config Plugins page: "Plugins are synchronous functions". Therefore, creating a getRuntimeVersionNullableAsync wouldn't be enough. For this to work truly asynchronous we would need to rewrite how config plugins work.

@Kudo
Copy link
Contributor

Kudo commented Aug 31, 2023

@Kudo I'm not sure this achieves the desired goal. As stated on Expo's Config Plugins page: "Plugins are synchronous functions". Therefore, creating a getRuntimeVersionNullableAsync wouldn't be enough. For this to work truly asynchronous we would need to rewrite how config plugins work.

plugins are synchronous but mods could be asynchronous. an example is like this one. anyhow, i agree that introducing getRuntimeVersionNullableAsync requires a lot of changes since current update related config-plugins code is not async. but after the change, it could open more possibilities. (though just to discuss something we could enhance internally in the future)

@mccraveiro

This comment was marked as resolved.

@mccraveiro
Copy link
Contributor Author

An option is to tell users about the experimental nature of the fingerprint policy in the docs and in a Log.warn in eas-cli.

@quinlanj It seems that we would also need a Log.warn in expo-cli. Any tips on how to handle this and reduce code duplication?

@mccraveiro
Copy link
Contributor Author

@Kudo @quinlanj I updated the code to work asynchronous and It was easier than expected. Not sure I'm missing something.

Copy link
Member

@quinlanj quinlanj left a comment

Choose a reason for hiding this comment

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

Couple nits, but otherwise lgtm 🚀 Let's also wait for @douglowder or @wschurman to take a look as well.

In response to the other messages on this thread:

I'm not sure I get the use case where someone would customize the files that goes into the fingerprint. On @ItsWendell's script this seems to be the case because of an underlining bug in expo-fingerprint. Not sure we should plan around that.

Here are my hypothesis on why people would want to customize:

  • Fingerprint returns different hash, but user wants equivalent runtime: Developer has extra information that fingerprint doesnt. For example, if they made a change in app.json they know doesn't affect native compatibility.
  • Fingerprint returns same hash, but user wants different runtime: Developer has made JS/TS code changes that is compatible with the native code but is incompatible from a product standpoint, so they want to bump the runtime version.

These are just my hypothesis, so I'm curious if any of these cases resonate with you.

It seems that we would also need a Log.warn in expo-cli. Any tips on how to handle this and reduce code duplication?

I saw you added console.warn. This is sufficient and will print when the cli is run, so we can skip adding Log.warn

packages/@expo/config-plugins/src/android/Updates.ts Outdated Show resolved Hide resolved
docs/pages/eas-update/runtime-versions.mdx Outdated Show resolved Hide resolved
docs/pages/eas-update/runtime-versions.mdx Outdated Show resolved Hide resolved
docs/pages/eas-update/runtime-versions.mdx Outdated Show resolved Hide resolved
@mccraveiro
Copy link
Contributor Author

Here are my hypothesis on why people would want to customize:

  • Fingerprint returns different hash, but user wants equivalent runtime: Developer has extra information that fingerprint doesnt. For example, if they made a change in app.json they know doesn't affect native compatibility.
  • Fingerprint returns same hash, but user wants different runtime: Developer has made JS/TS code changes that is compatible with the native code but is incompatible from a product standpoint, so they want to bump the runtime version.

These are just my hypothesis, so I'm curious if any of these cases resonate with you.

@quinlanj, you bring up interesting use cases. Still, I think the existing custom runtime version covers those bases. The fingerprint is more for people who want a 'set-it-and-forget-it' option for releases.

Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

since this pr introduced breaking changes. we should also update eas-cli and eas-build to migrate for the new getRuntimeVersionAsync and getRuntimeVersionNullableAsync. that would be something we need to follow up after the pr landed.

docs/pages/eas-update/runtime-versions.mdx Outdated Show resolved Hide resolved
packages/@expo/config-plugins/CHANGELOG.md Show resolved Hide resolved
packages/@expo/config-plugins/src/android/Updates.ts Outdated Show resolved Hide resolved
packages/@expo/config-plugins/src/android/Updates.ts Outdated Show resolved Hide resolved
packages/@expo/config-plugins/src/utils/Updates.ts Outdated Show resolved Hide resolved
mccraveiro and others added 2 commits September 6, 2023 04:13
Co-authored-by: Kudo Chien <ckchien@gmail.com>
Co-authored-by: Kudo Chien <ckchien@gmail.com>
@douglowder
Copy link
Contributor

Generally looks good. Will take a look to see what is causing Updates E2E to fail.

@mccraveiro mccraveiro requested a review from Kudo September 6, 2023 21:39
@wschurman wschurman removed their request for review September 6, 2023 23:32
Copy link
Member

@quinlanj quinlanj left a comment

Choose a reason for hiding this comment

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

@mccraveiro I spoke to various members of our team and we'd like to rename the policy name to fingerprintExperimental. Many folks don't read the docs or console outputs, so this is an additional way for us to communicate that this feature is experimental.

Thanks again for navigating the discord threads, putting together this PR and implementing all our feedback. Really appreciate it!

@mccraveiro
Copy link
Contributor Author

@quinlanj That makes sense. I'll update the PR.

@mccraveiro mccraveiro changed the title [expo-updates] Add fingerprint runtime version policy [expo-updates] Add fingerprintExperimental runtime version policy Sep 7, 2023
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

it's almost there! sorry that i just found one problem from a last-minute review. after that it's perfect for me.

the ci updates e2e tests failures on eas build are expected for external contributors.

…andlerMiddleware.ts

Co-authored-by: Kudo Chien <ckchien@gmail.com>
Kudo added a commit that referenced this pull request Sep 8, 2023
# Why

add `.fingerprintignore` support which is commented from #24126 (review)

# How

- add `options.ignorePaths` that support both dirs/files and try to support patterns more like .gitignore
- read `{projectRoot}/.fingerprintignore` as additional `options.ignorePaths`

usage example: add `{projectRoot/.fingerprintignore`. the contents could be as following example

```
# Comment prefixed with # is supported

# ignores whole ios directory
ios/**/*

# but not ios/Podfile
!ios/Podfile
```
fingerprint dependency needs to be 0.2.0 to be compatible with the monorepo
@douglowder douglowder self-requested a review September 11, 2023 19:19
Copy link
Contributor

@douglowder douglowder left a comment

Choose a reason for hiding this comment

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

Fixed fingerprint dependency, resolved CHANGELOG conflict, and ran updates E2E tests manually to verify that they pass. Looks good. Will allow CI to run before merging. @quinlanj

@douglowder douglowder merged commit f0d67e1 into expo:main Sep 11, 2023
11 of 14 checks passed
@mccraveiro
Copy link
Contributor Author

Thanks @douglowder!

alanjhughes pushed a commit that referenced this pull request Sep 14, 2023
…4126)

# Why

Manually handling runtimeVersions is a big pain for projects that uses
native modules. This PR adds a new runtime policy that handles this
automatically.

See: 

https://discord.com/channels/695411232856997968/1142023605908156476/1142023605908156476

# How

Using
[expo-fingerprint](https://github.com/expo/expo/tree/main/packages/@expo/fingerprint#readme)
to auto-generate a hash for the project.

# Test Plan

I made the changes locally on a working project and the build was
successfully with expo prebuild. I checked the native files and the
runtime version was set to the project hash.

# Checklist

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [x] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [x] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Co-authored-by: Aman Mittal <amandeepmittal@live.com>
Co-authored-by: Kudo Chien <ckchien@gmail.com>
Co-authored-by: Douglas Lowder <douglowder@mac.com>
@a-eid
Copy link

a-eid commented Jan 19, 2024

after upgrading to SDK 50 I get the following error when I run my dev client ( SDK 50 ).

CommandError: Must specify --private-key-path argument to sign development manifest for requested code signing key

Screenshot 2024-01-19 at 12 26 13 PM

@wschurman
Copy link
Member

wschurman commented Jan 19, 2024

@a-eid - Please file a new issue with more information including a repro, the commands you are running, etc. https://github.com/expo/expo/issues/new/choose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants