Read Podfile dependencies from third-party-podspec #6086

mokagio · 2023-08-18T04:49:44Z

The previous approach had them hardcoded. This is better because we now have a single source of truth.

Hat tip @crazytonyli, see #5739 (comment)

Notice that the only Podfile.lock change is in the checksum, meaning that the new logic results in the same resolved dependencies as before.

To test: Ensure CI is green.

PR submission checklist:

I have considered adding unit tests where possible. – N.A.
I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary. – N.A.

mokagio · 2023-08-18T04:50:57Z

ios-xcframework/Podfile

  raise "Could not find node modules at given path #{rn_node_modules}" unless File.exist? rn_node_modules
+


RuboCop suggests leaving an empty line after raise for readability 😉

mokagio · 2023-08-18T04:51:51Z

ios-xcframework/Podfile.lock

@@ -674,6 +674,6 @@ SPEC CHECKSUMS:
  WordPress-Aztec-iOS: 7d11d598f14c82c727c08b56bd35fbeb7dafb504
  Yoga: f7decafdc5e8c125e6fa0da38a687e35238420fa

-PODFILE CHECKSUM: 1caec69a3de9c6e37113c8eb5dbdb2d2b52c5108
+PODFILE CHECKSUM: 8f2b84cf5105963b4ef56ad18d6d64ab24334789


Notice the only change is in the checksum. This mean that the resolved dependencies with this new approach are exactly the same as before 🎉

The different checksum is due to the text content of Podfile having changed.

crazytonyli · 2023-08-20T22:00:11Z

ios-xcframework/Podfile

+  podspec_extension = '.podspec.json'
+
+  Dir["../third-party-podspecs/*#{podspec_extension}"].each do |podspec_path|
+    pod_name = File.basename(podspec_path, podspec_extension)


It might be more robust to use the podspec json's name property as the pod_name, instead of the filename? We probably won't use a different name for the podspec.json file intentionally, but there might be a typo, omitting a dash (i.e. RCTFolly.podspec.json for the RCT-Folly pod), or something else, which would cause a mismatch between the filename and the pod name.

Done via f865eb5

So it's easier to find them in the `third-party-podspec` folder

I run `bundle exec pod install` to verify these removals didn't result in any change in the `Podfile.lock`, which they didn't.

@crazytonyli

The previous approach had them hardcoded. This is better because we now have a single source of truth. Hat tip @crazytonyli, see #5739 (comment) Notice that the only `Podfile.lock` change is in the checksum, meaning that the new logic results in the same resolved dependencies as before.

Co-authored-by: Tony Li <tony.li@automattic.com> See #6086 (comment)

peril-wordpress-mobile · 2023-08-23T01:35:40Z

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

peril-wordpress-mobile · 2023-08-23T01:35:40Z

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

peril-wordpress-mobile · 2023-08-23T01:35:40Z

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

mokagio · 2023-08-23T01:37:14Z

@dcalhoun @geriux @SiobhyB @fluiddot

Is there anything I'm missing with this implementation that uses the contents of third-party-specs as the source of truth for which dependencies to include?

Thanks!

geriux · 2023-08-24T17:59:20Z

Is there anything I'm missing with this implementation that uses the contents of third-party-specs as the source of truth for which dependencies to include?

Nope! We are good at using third-party-specs as the source of truth 🚀

geriux · 2023-08-24T18:06:34Z

I see React-related podspecs deleted, is this because of the XCFramework that they are not needed? We might need to update the bin/generate-podspecs.sh script to avoid adding them back whenever we update a dependency 🤔

mokagio · 2023-08-28T01:27:29Z

@geriux thank you for mentioning that. I run bin/generate-podspec.sh and it did in fact recreate the specs I removed. Clearly there's more work to do to centralize the source of truth.

geriux · 2023-08-28T07:15:44Z

@geriux thank you for mentioning that. I run bin/generate-podspec.sh and it did in fact recreate the specs I removed. Clearly there's more work to do to centralize the source of truth.

No problem! I think if we just remove the # Generate the React Native podspecs part within the bin/generate-podspecs.sh script we'd be set.

fluiddot · 2023-08-28T12:02:21Z

Is there anything I'm missing with this implementation that uses the contents of third-party-specs as the source of truth for which dependencies to include?

The approach looks good to me Gio, thanks for working on this 🙇 ! The only caveat, as @geriux shared in #6086 (comment), we'd need to prevent re-generating the React-related pods in the generate-podspecs.sh script.

mokagio · 2023-09-06T06:40:26Z

@geriux @fluiddot I updated the script to generates the third-party-podspecs to only generated the ones we currently need. The approach could do with some more work and optimization, but in my tests locally it did not generate additional or removed existing .podspecs.

If one of you can confirm, I'll proceed with merging.

mokagio · 2023-09-07T00:15:01Z

I'll take those emoji reactions as confirmation. I just merged trunk into this branch to 1) update CI to match the new requirements (Lint and Unit Tests from Buildkite) and 2) verify locally that the script didn't change the third-party-podspecs content.

Enabling auto-merge

fluiddot

Thanks @mokagio for the improvements 🙇 !

I've only added a minor comment, that shouldn't block the PR but would be interesting to include to avoid adding an unnecessary pod.

Probably something for another PR, but I noticed we still have the logic that prompts us to run the generate-podspecs. twice:

gutenberg-mobile/bin/generate-podspecs.sh

Lines 39 to 43 in 1261f7b

    
           read -r -p "Enter the commit hash of previous commit. If this is the first-time running this script, enter 0, then commit generated files and re-rerun the script and this time use the previous commit hash: " COMMIT_HASH 
        
           if [[ -z "$COMMIT_HASH" ]]; then 
        
               echo "Commit hash cannot be empty." 
        
               exit 1 
        
           fi

gutenberg-mobile/bin/generate-podspecs.sh

Lines 148 to 155 in 1261f7b

    
           # We are required to run this script twice to capture the correct target. 
        
           # 0 is the value set during the first script run to generate the podspecs. 
        
           if [[ "$COMMIT_HASH" != "0" ]]; then 
        
               echo 'Updating XCFramework Podfile.lock with these changes' 
        
               pushd ios-xcframework > /dev/null 
        
               bundle exec pod update 
        
               popd > /dev/null 
        
           fi

I understand that this is no longer needed as the FBReactNativeSpec pod is now generated in the XCFramework project, is this accurate?

ios-xcframework/Podfile

bin/generate-podspecs.sh

Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

mokagio · 2023-09-15T04:21:22Z

I understand that this [running pod install twice] is no longer needed as the FBReactNativeSpec pod is now generated in the XCFramework project, is this accurate?

I'll need to double check this and follow up if necessary.

See https://github.com/wordpress-mobile/gutenberg-mobile/pull/6086/files#r1318224598 Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

I noticed this because RuboCop barked at me for having a method with 11 SLOC instead of 10. However, I'm happy with the change. The implementation expects the file to be a JSON, so there's little value in making the extension configurable. If we changed that var to `.podspec`, the implementation would break!

mokagio · 2023-09-15T04:34:35Z

@fluiddot @geriux I addressed your new suggestions, thanks again. I'd appreciate a final look over, just in case.

# Conflicts: # ios-xcframework/Podfile.lock

mokagio · 2023-09-18T12:02:14Z

Thanks for the trunk mereg @fluiddot. CI is green. I'll take your reaction as a 👍 to merge this.

fluiddot · 2023-09-18T13:22:36Z

Thanks for the `trunk` mereg @fluiddot. CI is green. I'll take your reaction as a 👍 to merge this.

@mokagio To be honest, I couldn't fully test it last week when I shared those reactions. From the code perspective, the PR looks good to me but I was planning to generate an installable build as a double-check. Since CI is green let's keep this as-is and we'll test next week an installable build when we have more availability. Thanks 🙇 !

fluiddot · 2023-09-25T11:36:07Z

Thanks for the `trunk` mereg @fluiddot. CI is green. I'll take your reaction as a 👍 to merge this.

@mokagio To be honest, I couldn't fully test it last week when I shared those reactions. From the code perspective, the PR looks good to me but I was planning to generate an installable build as a double-check. Since CI is green let's keep this as-is
and we'll test next week an installable build when we have more availability. Thanks 🙇 !

I tested using the XCFramework and a local Gutenberg installation and haven't found any build and runtime errors 🎊.

mokagio requested a review from crazytonyli August 18, 2023 04:50

mokagio commented Aug 18, 2023

View reviewed changes

crazytonyli approved these changes Aug 20, 2023

View reviewed changes

mokagio added 4 commits August 23, 2023 11:25

Sort dependencies alphabetically in Podfile

4d91fdb

So it's easier to find them in the `third-party-podspec` folder

Removed custom podspec.json for deps not used in Podfile

60b8095

I run `bundle exec pod install` to verify these removals didn't result in any change in the `Podfile.lock`, which they didn't.

Read XCFramework dependencies name from podspec content, not file name

f865eb5

Co-authored-by: Tony Li <tony.li@automattic.com> See #6086 (comment)

mokagio force-pushed the mokagio/compute-podfile-dependencies branch from a4762b3 to f865eb5 Compare August 23, 2023 01:35

mokagio marked this pull request as draft August 28, 2023 01:27

mokagio added 3 commits September 6, 2023 16:16

Remove unused RN_DIR var definition

5cacf8f

Remove unnecessary pods from generate-podspecs script

90601d3

DRY the logic to annotate the custom podspecs with a warning

52d4196

mokagio marked this pull request as ready for review September 6, 2023 06:38

mokagio requested review from geriux and fluiddot September 6, 2023 06:38

Merge 'origin/trunk' into mokagio/compute-podfile-dependencies

1261f7b

fluiddot reviewed Sep 7, 2023

View reviewed changes

ios-xcframework/Podfile Outdated Show resolved Hide resolved

bin/generate-podspecs.sh Show resolved Hide resolved

bin/generate-podspecs.sh Show resolved Hide resolved

Remove unused HERMES_ENABLED from generate-podspecs.sh

5039d5c

Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

mokagio and others added 2 commits September 15, 2023 14:28

Fix JSC vs Hermes pod inclusion logic

5fe3b77

See https://github.com/wordpress-mobile/gutenberg-mobile/pull/6086/files#r1318224598 Co-authored-by: Carlos Garcia <fluiddot@gmail.com>

mokagio force-pushed the mokagio/compute-podfile-dependencies branch from c52c16c to 8f9fb1c Compare September 15, 2023 04:30

Merge branch 'origin/trunk' into mokagio/compute-podfile-dependencies

ba402c3

mokagio requested review from fluiddot and crazytonyli September 15, 2023 04:32

Merge branch 'trunk' into mokagio/compute-podfile-dependencies

37058de

# Conflicts: # ios-xcframework/Podfile.lock

mokagio merged commit 2e52634 into trunk Sep 18, 2023

mokagio deleted the mokagio/compute-podfile-dependencies branch September 18, 2023 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read Podfile dependencies from third-party-podspec #6086

Read Podfile dependencies from third-party-podspec #6086

mokagio commented Aug 18, 2023

mokagio Aug 18, 2023

mokagio Aug 18, 2023

crazytonyli Aug 20, 2023

mokagio Aug 23, 2023

peril-wordpress-mobile bot commented Aug 23, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Aug 23, 2023

peril-wordpress-mobile bot commented Aug 23, 2023

mokagio commented Aug 23, 2023

geriux commented Aug 24, 2023

geriux commented Aug 24, 2023

mokagio commented Aug 28, 2023

geriux commented Aug 28, 2023

fluiddot commented Aug 28, 2023

mokagio commented Sep 6, 2023

mokagio commented Sep 7, 2023

fluiddot left a comment

mokagio commented Sep 15, 2023

mokagio commented Sep 15, 2023

mokagio commented Sep 18, 2023

fluiddot commented Sep 18, 2023

fluiddot commented Sep 25, 2023

		raise "Could not find node modules at given path #{rn_node_modules}" unless File.exist? rn_node_modules

	read -r -p "Enter the commit hash of previous commit. If this is the first-time running this script, enter 0, then commit generated files and re-rerun the script and this time use the previous commit hash: " COMMIT_HASH
	if [[ -z "$COMMIT_HASH" ]]; then
	echo "Commit hash cannot be empty."
	exit 1
	fi

	# We are required to run this script twice to capture the correct target.
	# 0 is the value set during the first script run to generate the podspecs.
	if [[ "$COMMIT_HASH" != "0" ]]; then
	echo 'Updating XCFramework Podfile.lock with these changes'
	pushd ios-xcframework > /dev/null
	bundle exec pod update
	popd > /dev/null
	fi

Read Podfile dependencies from third-party-podspec #6086

Read Podfile dependencies from third-party-podspec #6086

Conversation

mokagio commented Aug 18, 2023

mokagio Aug 18, 2023

Choose a reason for hiding this comment

mokagio Aug 18, 2023

Choose a reason for hiding this comment

crazytonyli Aug 20, 2023

Choose a reason for hiding this comment

mokagio Aug 23, 2023

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Aug 23, 2023 • edited Loading

peril-wordpress-mobile bot commented Aug 23, 2023

peril-wordpress-mobile bot commented Aug 23, 2023

mokagio commented Aug 23, 2023

geriux commented Aug 24, 2023

geriux commented Aug 24, 2023

mokagio commented Aug 28, 2023

geriux commented Aug 28, 2023

fluiddot commented Aug 28, 2023

mokagio commented Sep 6, 2023

mokagio commented Sep 7, 2023

fluiddot left a comment

Choose a reason for hiding this comment

mokagio commented Sep 15, 2023

mokagio commented Sep 15, 2023

mokagio commented Sep 18, 2023

fluiddot commented Sep 18, 2023

fluiddot commented Sep 25, 2023

peril-wordpress-mobile bot commented Aug 23, 2023 •

edited

Loading