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

Upgrades: Android 12, Androidx-test 1.4, and RN 66 #3035

Merged
merged 26 commits into from
Dec 22, 2021
Merged

Conversation

jonathanmos
Copy link
Contributor

@jonathanmos jonathanmos commented Oct 24, 2021

Upgrades the android version to 12, androidx-test version to 1.4.0 and RN to 0.66.1.
Fixes #2899
Fixes #2817
Fixes #3033
Fixes #3034

@jonathanmos jonathanmos requested a review from d4vidi as a code owner October 24, 2021 13:51
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I've done a couple Android12 forward ports and the exported symbols should be all that's required.
I saw a couple spacing issues which I made suggestions for, that's just nitpicks.

I will say that you may do a "minimal" forward port - if bumping all the versions is problematic - by simply adding the exported bits to AndroidManifest.xml without actually changing versions anywhere, as they are backwards compatible.

Or a middle ground would be to change the test app perhaps but not the main detox/android/build.gradle.

Although changing all of them should be fine really

Cheers!

@mikehardy
Copy link
Contributor

Indeed, I see this in the build logs on one of the failures:

08:54:20 FAILURE: Build failed with an exception.
08:54:20 
08:54:20 * What went wrong:
08:54:20 Could not determine the dependencies of task ':detox:compileFullReleaseAidl'.
08:54:20 > Installed Build Tools revision 31.0.0 is corrupted. Remove and install again using the SDK Manager.
08:54:20 

That's a known issue https://issuetracker.google.com/issues/190734097 and will show up until/unless android gradle plugin is bumped to 7+ - so it may be best/easiest to leave the version bumps alone or you'll need to also bump gradle to 7.2 and android gradle plugin to 7.0.3, but then you'll likely also need to update the test project to react-native 0.66.1 because I think react-native 0.64 at least (maybe 0.65 as well...) had problems with the new android gradle plugin.

Completely separately, CI everywhere else is failing on


09:54:17 > [email protected] test /home/jenkins/.jenkins/workspace/detox-android-64-x-pr/detox/test
09:54:17 > tsc && tslint types/*.ts
09:54:17 
09:54:17 node_modules/@types/node/child_process.d.ts(267,18): error TS2304: Cannot find name 'AbortSignal'.
09:54:17 

That looks unrelated. I posted a PR that could resolve that #3036

Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Formatting issues are still present
The previous issue with build tools corrupted is gone now that you've attempted 30
However react-native 0.59 appears to tickle a native compiler bug as seen in that jenkins failure.

I will humbly submit that in order to get this through, you should propose the minimal change. just the exports. That's all that is strictly required.

@jonathanmos jonathanmos marked this pull request as draft October 28, 2021 08:25
@jonathanmos jonathanmos changed the title Upgrade to android 12 Upgrade Android, Androidx-test, and RN Nov 3, 2021
@jonathanmos jonathanmos marked this pull request as ready for review December 5, 2021 15:28
@d4vidi d4vidi mentioned this pull request Dec 7, 2021
4 tasks
@d4vidi d4vidi changed the title Upgrade Android, Androidx-test, and RN Upgrades: Android 12, Androidx-test 1.4, and RN 66 Dec 7, 2021
Copy link
Collaborator

@d4vidi d4vidi left a comment

Choose a reason for hiding this comment

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

Brilliant work but unfortunately I must make our lives even harder in this case... Please go over my notes, kind sir!

Copy link
Collaborator

@d4vidi d4vidi left a comment

Choose a reason for hiding this comment

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

Almost there... @jonathanmos please review

String mCommand = Reflect.on(viewOperation).field(FIELD_MCOMMAND).get();

boolean isReactSwitch = mClassName.equals(REACT_SWITCH);
boolean hasOneRetryIncremented = numRetries == 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This indicates that the operation has been queued for retry, which happens when the reactSwitch operation fails in the rn66 bug

synchronized(Reflect.on(uiViewOperationQueueInstance).field(LOCK_OPERATIONS).get())
{
Reflect.on(uiViewOperationQueueInstance)
.field(FIELD_DISPATCH_RUNNABLES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(should have been: FIELD_NON_BATCHED_OPS_LOCK)

@d4vidi
Copy link
Collaborator

d4vidi commented Dec 15, 2021

@jonathanmos I took the liberty to push a commit (dbe0db4) here in order to try to proactively assist in pushing this forward towards a merge.

I addressed several domains:

  1. Mostly styling - tried to make things more Kotlin idiomatic
  2. A touch of architecture: made Reflected classes more tightly isolated in associated with the class they are reflecting
  3. Found a bug 🐞 ! (see previous comment)

I hate to say this but I couldn't build and test this on my local env, so I will leave that to the CI / us tomorrow (today!)

@jonathanmos jonathanmos merged commit 2b83c86 into master Dec 22, 2021
@jonathanmos jonathanmos deleted the android-12 branch December 22, 2021 08:24
@mikehardy
Copy link
Contributor

@jonathanmos & @d4vidi Exciting to see this merged! Fantastic to see Detox all ready for another mobile version year (with iOS15 and Android12 thoroughly handled). Amazing to me that it requires so much effort some times to handle the versions. Hopefully Android13 and iOS16 won't be so difficult :-). Cheers

@d4vidi
Copy link
Collaborator

d4vidi commented Dec 22, 2021

@mikehardy Thanks for the kind words, and for being involved in the process 🙏🏻
Unfortunately, as RN have been holding back the official version 1 release since forever, from time to time a version upgrade happens to be very painful. I truly hope this would not be the case in at least a couple of versions 🤞🏻

In any case, solving the worked-around issue is being handled on a separate thread, as I'm sure you already know.

@mikehardy
Copy link
Contributor

Yes, I've been a part of the RN release process as well, working hard on getting Xcode 13+ and Apple Silicon / M1 chips handled there, it is really fiddly to get the compile working and took quite a few iterations. Then in the end a Hermes upgrade resulted in rejected builds from Apple. Sometimes it just takes a while but the current RN67 release candidate has no show-stoppers reported after around a week of testing amongst our group of pre-release testers (finally!) and I've had successful TestFlight builds with it on M1 macs even, so maybe finally it's good to go :-). Cheers

@d4vidi
Copy link
Collaborator

d4vidi commented Dec 22, 2021

Best of luck! We've been reported of the hermes issues - happy to see it's coming together nonetheless. Cheers!

@@ -29,4 +29,5 @@ end

post_install do |installer|
react_native_post_install(installer)
__apply_Xcode_12_5_M1_post_install_workaround(installer)
Copy link
Contributor

@asafkorem asafkorem Dec 23, 2021

Choose a reason for hiding this comment

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

We are not using Xcode 12.5 + M1 on our agents 🙂

Copy link
Contributor

@mikehardy mikehardy Dec 23, 2021

Choose a reason for hiding this comment

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

This is part of the standard template, I worked so hard on that workaround 😅 - I'm not sure why 12.5+ (or 13+ now?) isn't in use in the agents? But if it is at all you will suffer immediate build failures without this workaround, and in fact there may be build failures just by including Swift, not just using 12.5+ or M1. Either way, it's part of the standard template (npx react-native init...) and will also be removed in react-native 0.68 (0.67 still requires it unfortunately...). Building on Xcode right now is a real mess basically, and I would remove that line at your peril...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for all your efforts on this upgrade Mike. I just saw the workaround details on RN. We are using Xcode 13, so it is relevant for our Xcode version. I was too quick to judge that it was relevant for v12.5 and not for the following versions (or specifically v13)..

I saw this error in one of our builds, and wondered why we need this call on our Podfile if we are not using M1 agents:

Generating Pods project
--
  | [!] An error occurred while processing the post-install hook of the Podfile.
  |  
  | undefined method `__apply_Xcode_12_5_M1_post_install_workaround' for #<Pod::Podfile:0x00007f7f5bd58f68>
  |  
  | /Users/builder/work/detox/test/ios/Podfile:32:in `block (2 levels) in from_ruby'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-core-1.10.2/lib/cocoapods-core/podfile.rb:179:in `post_install!'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:897:in `run_podfile_post_install_hook'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:885:in `block in run_podfile_post_install_hooks'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/user_interface.rb:145:in `message'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:884:in `run_podfile_post_install_hooks'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:329:in `block (2 levels) in create_and_save_projects'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer/xcode/pods_project_generator/pods_project_writer.rb:61:in `write!'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:328:in `block in create_and_save_projects'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/user_interface.rb:64:in `section'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:307:in `create_and_save_projects'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:299:in `generate_pods_project'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:178:in `integrate'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/installer.rb:166:in `install!'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/command/install.rb:52:in `run'
  | /Library/Ruby/Gems/2.6.0/gems/claide-1.0.3/lib/claide/command.rb:334:in `run'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/lib/cocoapods/command.rb:52:in `run'
  | /Library/Ruby/Gems/2.6.0/gems/cocoapods-1.10.2/bin/pod:55:in `<top (required)>'
  | /usr/local/bin/pod:23:in `load'
  | /usr/local/bin/pod:23:in `<main>'

However, when tried to remove this line, our build failed on xcodebuild command..

Copy link
Contributor

@asafkorem asafkorem Dec 26, 2021

Choose a reason for hiding this comment

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

OK, the issue was that we are calling this method from RN0.64 build (and this method doesn't exist yet).
We'll wrap this with a version check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Yes, that won't work :-), great catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants