Page MenuHomePhabricator
Differential D11202

[GH Actions] Move Android CI workflows to `macOS`
ClosedPublic
Actions

Authored by atul on Feb 29 2024, 2:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 28, 7:31 PM
Unknown Object (File)
Mon, Mar 24, 8:33 AM
Unknown Object (File)
Sat, Mar 8, 11:40 AM
Unknown Object (File)
Sat, Mar 8, 11:40 AM
Unknown Object (File)
Sat, Mar 8, 11:40 AM
Unknown Object (File)
Sat, Mar 8, 11:40 AM
Unknown Object (File)
Sat, Mar 8, 11:39 AM
Unknown Object (File)
Sat, Mar 8, 11:39 AM
View All 97 Files
Subscribers
None

Details

Reviewers
ashoat
ginsu
tomek
varun
Commits
rCOMMff7b0bf442fe: [GH Actions] Move Android CI workflows to `macOS`
Summary

Context: https://linear.app/comm/issue/ENG-7023/investigate-github-action-android-ci-builds-failing

Test Plan

Android Build workflow worked on branch:

2647c7.png (1×2 px, 453 KB)

Release should work since that just adds a JS script that's platform agnostic (and I wrote and tested in macOS environment). We should kick off a native release just to give it a shot anyways.

Diff Detail

Repository
rCOMM Comm
Branch
android-ci-to-macos (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Feb 29 2024, 2:42 PM
atul published this revision for review.Feb 29 2024, 2:56 PM
atul added inline comments.
.github/workflows/android_ci.yml
41

To account for differences between Ubuntu and macOS provided base64.

54

I previously tried

JAVA_HOME=${JAVA_HOME_11_arm64} ./gradlew bundleRelease

and setting env the same way as ANDROID_SIGNING_PASSWORD, but those workflows failed (very likely for other reasons).

Leaving for now since this worked, but will try moving to match ANDROID_SIGNING_PASSWORD and update if that run works.

60

~/.gradle already exists on the CI runners so mkdir returns error.

.github/workflows/android_release.yml
28–32

This also differs from the Android Build workflow.

Wasn't able to test the release workflow on my fork, but did check that mkdir -p works as expected on macOS and there are no differences there.

atul edited the test plan for this revision. (Show Details)Feb 29 2024, 2:57 PM
Harbormaster completed remote builds in B27243: Diff 37717.Feb 29 2024, 2:58 PM
atul edited the test plan for this revision. (Show Details)Feb 29 2024, 2:59 PM
ashoat accepted this revision.Feb 29 2024, 3:01 PM

Thanks for explaining changes inline

.github/workflows/android_ci.yml
60

Can we make this mkdir -p instead? Better to make it worth for both imo

.github/workflows/android_release.yml
59

mkdir -p

This revision is now accepted and ready to land.Feb 29 2024, 3:01 PM
atul updated this revision to Diff 37718.Feb 29 2024, 3:04 PM

bring back mkdir w/ -p flag

Harbormaster completed remote builds in B27244: Diff 37718.Feb 29 2024, 3:20 PM
atul updated this revision to Diff 37719.Feb 29 2024, 3:21 PM

setting JAVA_HOME before command worked, using the env approach with ANDROID_SIGNING_PASSWORD didn't.

This revision was landed with ongoing or failed builds.Feb 29 2024, 3:22 PM
Closed by commit rCOMMff7b0bf442fe: [GH Actions] Move Android CI workflows to `macOS` (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMMff7b0bf442fe: [GH Actions] Move Android CI workflows to `macOS`.
Harbormaster completed remote builds in B27245: Diff 37719.Feb 29 2024, 3:37 PM
ashoat mentioned this in D13247: [GitHub CI] Clear unused NDKs and iOS simulator to fix Android CI.Sep 5 2024, 9:17 AM
ashoat mentioned this in rCOMM3e5b8b08e401: [GitHub CI] Clear unused NDKs and iOS simulator to fix Android CI.Sep 5 2024, 9:22 AM

Revision Contents
Changeset List

PathSize
.github/
workflows/
27 lines
27 lines

Diff 37717

View Options

.github/workflows/android_ci.yml

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;
View Options

.github/workflows/android_release.yml

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;