[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

radoslawkrzemien · 2023-10-02T12:47:48Z

Why

https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds
Companion for: expo/eas-build#283, https://github.com/expo/universe/pull/13524, and https://github.com/expo/universe/pull/13523

How

distribution value is kept instead of hardcoded to simulator if simulator flag is set, keeping its original value
simulator field has been added to metadata to retain its value

Test Plan

Existing tests pass

Screens

Build run with old cli version

Logged in

Internal build

Internal simulator build

Store build

Store simulator build

Not logged in

Internal build

Internal simulator build

Store build

Store simulator build

Build run with new cli version

Logged in

Internal build

Internal simulator build

Store build

Store simulator build

Not logged in

Internal build

Internal simulator build

Store build

Store simulator build

Deployment

www
website
cli

Updated eas-build-job to new version See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Stops overwriting distribution type for simulator builds, and sets simulator field in metadata instead See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

linear · 2023-10-02T12:47:51Z

ENG-9957 Fix internal distribution pages for simulator builds

While looking into ENG-6853I noticed that I wasn't able to access build pages for internal distribution builds that target simulators. We should support people using internal distribution for this case as well, it would allow you to share a URL to your build page where someone could then just press a button to launch in Orbit, or download and do whatever they want with it. This would work fine on Android because APKs are directly installable for devices and emulators.

For example, I created a build with this profile named "simulator": https://github.com/brentvatne/microfoam-app/blob/e29d622809172be8d271622f0a699f306bffb05e/apps/mobile/eas.json#L50-L60

That build should be accessible from an incognito tab without signing in to expo.dev, as is the case with all internal distribution builds, but it is not: https://expo.dev/accounts/brents/projects/microfoam/builds/87343bae-3105-4517-a46b-d3149dea442c

github-actions · 2023-10-02T12:52:01Z

Size Change: +457 B (0%)

Total Size: 51.5 MB

Filename	Size	Change
`./packages/eas-cli/dist/eas-linux-x64.tar.gz`	51.5 MB	+457 B (0%)

_{compressed-size-action}

codecov · 2023-10-02T12:56:03Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (ceb561a) 54.16% compared to head (049f177) 54.16%.

Files	Patch %	Lines
packages/eas-cli/src/commands/build/list.ts	16.67%	4 Missing and 1 partial ⚠️
packages/eas-cli/src/build/metadata.ts	33.34%	2 Missing ⚠️
packages/eas-cli/src/commands/build/run.ts	50.00%	1 Missing ⚠️
packages/eas-cli/src/run/utils.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2073      +/-   ##
==========================================
- Coverage   54.16%   54.16%   -0.00%     
==========================================
  Files         516      516              
  Lines       18808    18814       +6     
  Branches     3968     3969       +1     
==========================================
+ Hits        10186    10188       +2     
- Misses       7931     7934       +3     
- Partials      691      692       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

szdziedzic · 2023-10-03T17:43:01Z

packages/eas-cli/src/build/metadata.ts

-    ('simulator' in ctx.buildProfile && ctx.buildProfile.simulator
-      ? 'simulator'
-      : ctx.buildProfile.distribution) ?? 'store';
+  const distribution = ctx.buildProfile.distribution ?? 'store';


This change will break stuff like eas build:run 🤔

I'm wondering if altering the distributionType behavior is necessary 🤔.

Maybe we can add some new field to Metadata to keep the raw buildProfile.distribution value there? I mean it's probably not optimal but I feel that this change can introduce some regressions in some parts of the codebase.

What do you think?

Can you elaborate on why eas build:run would be broken by this?
As for another field for "original distribution" or whatever we want to call it, I'm open for discussion but it feels to me like hacking around an existing hack (overwriting the distribution). I feel like we should remove the reliance on this hardcoded simulator value for distribution instead of building upon it, wdyt?

I see, it runs a simulator build and fetches it by filtering by distribution = simulator.
Maybe it should handle both options (distribution = simulator or simulator = true) for a while instead of introducing another field?

I concur, we should list builds with both simulator: true and distribution: simulator.

It's handled in https://github.com/expo/universe/pull/13524

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # packages/eas-cli/package.json # packages/eas-cli/src/build/metadata.ts # yarn.lock

Merged changes from main branch See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Updated build:run command to use simulator flag instead of overwritten distribution type See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # packages/eas-cli/src/build/metadata.ts # packages/eas-cli/src/commands/build/run.ts # packages/eas-json/package.json # yarn.lock

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # packages/eas-cli/src/graphql/generated.ts

Update graphql schema with simulator fields See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Add simulator flag to build:list command See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Fixed check function with correct field name See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Fixed failing tests after BuildFragment structure changed See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Added warning to build:list command that the distribution=simulator is deprecated See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

sjchmiela

👍, but I'm with @szdziedzic, I think we should not "break" listing simulator builds for now.

…-9957-dont-overwrite-distribution-for-simulator-builds

sjchmiela · 2024-01-22T12:11:52Z

packages/eas-cli/src/run/utils.ts

@@ -13,6 +13,7 @@ export function isRunnableOnSimulatorOrEmulator(build: BuildFragment): boolean {
    build.status === BuildStatus.Finished &&
    !!build.artifacts?.applicationArchiveUrl &&
    ((build.platform === AppPlatform.Ios && build.distribution === DistributionType.Simulator) ||


Suggested change

((build.platform === AppPlatform.Ios && build.distribution === DistributionType.Simulator) ||

I guess the isForSimulator already takes care of it?

szdziedzic

Looks good, few small comments

szdziedzic · 2024-01-22T22:48:56Z

packages/eas-cli/src/build/metadata.ts

-    ('simulator' in ctx.buildProfile && ctx.buildProfile.simulator
-      ? 'simulator'
-      : ctx.buildProfile.distribution) ?? 'store';
+  const distribution = ctx.buildProfile.distribution ?? 'store';


Suggested change

const distribution = ctx.buildProfile.distribution ?? 'store';

const distribution = ctx.buildProfile.distribution ?? BuildDistributionType.STORE;

szdziedzic · 2024-01-22T22:50:18Z

packages/eas-cli/src/commands/build/list.ts

+      Log.warn(
+        `Using "distribution" flag with "simulator" value is deprecated - use "simulator" flag instead`
+      );


Nice 👏

Suggested change

Log.warn(

`Using "distribution" flag with "simulator" value is deprecated - use "simulator" flag instead`

);

Log.warn(

`Using --distribution flag with "simulator" value is deprecated - use --simulator flag instead`

);

szdziedzic · 2024-01-22T22:52:07Z

packages/eas-cli/src/commands/build/list.ts

@@ -51,6 +52,7 @@ export default class BuildList extends EasCommand {
    ...EasPaginatedQueryFlags,
    limit: getLimitFlagWithCustomValues({ defaultTo: 10, limit: BUILDS_LIMIT }),
    ...EasNonInteractiveAndJsonFlags,
+    simulator: Flags.boolean(),


Maybe it would be good to add a description to this flag, putting emphasis on the fact that it will only filter the iOS simulator builds. People tend to confuse Android emulator with iOS simulator and one could think that this will also query for Android emulator builds.

szdziedzic · 2024-01-22T22:53:44Z

packages/eas-cli/src/commands/build/list.ts

@@ -51,6 +52,7 @@ export default class BuildList extends EasCommand {
    ...EasPaginatedQueryFlags,
    limit: getLimitFlagWithCustomValues({ defaultTo: 10, limit: BUILDS_LIMIT }),
    ...EasNonInteractiveAndJsonFlags,
+    simulator: Flags.boolean(),


Maybe we should also enforce that this is only applicable when using the ios value for the --platform flag? What do you think?

Added description to the new simulator flag See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Added validation so the simulator flag can only be used with --platofrm=ios See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Updated the warning displayed when using deprecated --distribution=simulator flag value See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Replaced hardcoded string with variable See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

Removed no longer necessary check for simulator build See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

szdziedzic

Looks good

Remember to regenerate the GQL schema if there are any changes to it in WWW PR

I would also do one test run of the eas build command with the simulator target and see if you are prompted correctly to install it on your device afterward and run the eas build:run command just to double check

Updated graphql schema after recent changes See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien · 2024-01-30T13:55:51Z

/changelog-entry chore Add simulator flag to metadata

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # CHANGELOG.md

radoslawkrzemien · 2024-01-31T16:05:53Z

Added screens for comparison

…-9957-dont-overwrite-distribution-for-simulator-builds

Updated graphql schema after recent changes See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

github-actions · 2024-01-31T16:10:53Z

✅ Thank you for adding the changelog entry!

radoslawkrzemien added 2 commits October 2, 2023 14:31

[eas-cli] Update dependencies

c48928b

Updated eas-build-job to new version See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Don't overwrite distribution

7679dee

Stops overwriting distribution type for simulator builds, and sets simulator field in metadata instead See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien requested review from szdziedzic and khamilowicz as code owners October 2, 2023 12:47

szdziedzic reviewed Oct 3, 2023

View reviewed changes

radoslawkrzemien added 3 commits October 30, 2023 11:44

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

4b41ba2

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # packages/eas-cli/package.json # packages/eas-cli/src/build/metadata.ts # yarn.lock

[eas-cli] Merge main

de4712a

Merged changes from main branch See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Check for simulator flag

2dd7e3b

Updated build:run command to use simulator flag instead of overwritten distribution type See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien marked this pull request as draft October 30, 2023 14:30

radoslawkrzemien added 7 commits December 13, 2023 14:49

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

c9d25b7

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # packages/eas-cli/src/build/metadata.ts # packages/eas-cli/src/commands/build/run.ts # packages/eas-json/package.json # yarn.lock

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

35b99d7

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # packages/eas-cli/src/graphql/generated.ts

[eas-cli] Update schema

f9b738a

Update graphql schema with simulator fields See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Add flag

4e1da63

Add simulator flag to build:list command See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Fix check

cf09ead

Fixed check function with correct field name See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Fix tests

9e25d60

Fixed failing tests after BuildFragment structure changed See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Add warning

dfa81f3

Added warning to build:list command that the distribution=simulator is deprecated See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien marked this pull request as ready for review January 19, 2024 14:02

radoslawkrzemien requested review from sjchmiela and szdziedzic January 19, 2024 14:49

sjchmiela approved these changes Jan 19, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

e19f1cb

…-9957-dont-overwrite-distribution-for-simulator-builds

sjchmiela approved these changes Jan 22, 2024

View reviewed changes

szdziedzic reviewed Jan 22, 2024

View reviewed changes

radoslawkrzemien added 4 commits January 23, 2024 11:53

[eas-cli] Add description

9b442e2

Added description to the new simulator flag See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Add validation

7bc9744

Added validation so the simulator flag can only be used with --platofrm=ios See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Change message

c15b9dd

Updated the warning displayed when using deprecated --distribution=simulator flag value See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Replace string

12895bd

Replaced hardcoded string with variable See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Remove redundant check

935dcbc

Removed no longer necessary check for simulator build See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien requested a review from szdziedzic January 23, 2024 11:05

szdziedzic approved these changes Jan 23, 2024

View reviewed changes

radoslawkrzemien added 2 commits January 23, 2024 17:11

[eas-cli] Update schema

24156cd

Updated graphql schema after recent changes See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

[eas-cli] Update schema

1fce06f

Updated graphql schema after recent changes See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien added 2 commits January 30, 2024 13:56

update CHANGELOG.md

59e7978

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

dd45591

…-9957-dont-overwrite-distribution-for-simulator-builds # Conflicts: # CHANGELOG.md

radoslawkrzemien added 2 commits January 31, 2024 17:09

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

577edaa

…-9957-dont-overwrite-distribution-for-simulator-builds

[eas-cli] Update schema

049f177

Updated graphql schema after recent changes See: https://linear.app/expo/issue/ENG-9957/fix-internal-distribution-pages-for-simulator-builds

radoslawkrzemien merged commit 315b5b2 into main Jan 31, 2024
9 checks passed

radoslawkrzemien deleted the @radoslawkrzemien/ENG-9957-dont-overwrite-distribution-for-simulator-builds branch January 31, 2024 16:21

szdziedzic mentioned this pull request Jan 31, 2024

Revert "[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds" #2207

Merged

radoslawkrzemien mentioned this pull request Feb 1, 2024

[eas-cli] [ENG-9957] Reapply changes after fixing query in www #2209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

radoslawkrzemien commented Oct 2, 2023 •

edited

Loading

linear bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023 •

edited

Loading

codecov bot commented Oct 2, 2023 •

edited

Loading

szdziedzic Oct 3, 2023

radoslawkrzemien Oct 4, 2023

radoslawkrzemien Oct 4, 2023

sjchmiela Jan 19, 2024

radoslawkrzemien Jan 22, 2024

sjchmiela left a comment

sjchmiela Jan 22, 2024

szdziedzic left a comment

szdziedzic Jan 22, 2024

szdziedzic Jan 22, 2024

szdziedzic Jan 22, 2024

szdziedzic Jan 22, 2024

szdziedzic left a comment

radoslawkrzemien commented Jan 30, 2024

radoslawkrzemien commented Jan 31, 2024

github-actions bot commented Jan 31, 2024

	const distribution = ctx.buildProfile.distribution ?? 'store';
	const distribution = ctx.buildProfile.distribution ?? BuildDistributionType.STORE;

[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

Conversation

radoslawkrzemien commented Oct 2, 2023 • edited Loading

Why

How

Test Plan

Screens

Build run with old cli version

Logged in

Internal build

Internal simulator build

Store build

Store simulator build

Not logged in

Internal build

Internal simulator build

Store build

Store simulator build

Build run with new cli version

Logged in

Internal build

Internal simulator build

Store build

Store simulator build

Not logged in

Internal build

Internal simulator build

Store build

Store simulator build

Deployment

linear bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023 • edited Loading

codecov bot commented Oct 2, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjchmiela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szdziedzic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szdziedzic left a comment

Choose a reason for hiding this comment

radoslawkrzemien commented Jan 30, 2024

radoslawkrzemien commented Jan 31, 2024

github-actions bot commented Jan 31, 2024

radoslawkrzemien commented Oct 2, 2023 •

edited

Loading

github-actions bot commented Oct 2, 2023 •

edited

Loading

codecov bot commented Oct 2, 2023 •

edited

Loading