Use the same version as `package.json` in XCFramework #6101

mokagio · 2023-08-23T07:33:56Z

See @crazytonyli suggestion here: #5769 (comment)

To test: Check the value in the generated Info.plist, either by running npm run xcframework:build locally or by downloading and inspecting the CI artifact:

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.

crazytonyli · 2023-08-25T02:04:12Z

Thanks for taking my suggestion! What do you think about passing the version configurations to the xcodebuild command here?

dcalhoun

The test plan succeeded for me. 🎉 Thanks!

mokagio · 2023-08-29T02:32:34Z

What do you think about passing the version configurations to the xcodebuild command here?

Thanks @crazytonyli !

I think that would work and it looks better that using PlistBuddy after the file has been created. At the same time, having the setting in the build script alone means we might lose it in case we change the automation (eg by moving to Fastlane). If it's as a build phase in the target, it will always run as long as we build the target.

Am I missing something?

crazytonyli · 2023-08-29T23:52:49Z

The current solution looks good to me. But I'm not sure what responsibilities should the "scaffold" project holds. We now have build related tasks in both the build.sh shell script and the Xcode project's build phases. I just thought maybe we should consolidate the build related tasks in one place. The build.sh seems to be a good place for them. Also, we get to keep the scaffold Xcode project as lean as possible, which could be beneficial too.

BTW, I believe you can also pass build settings to fastlane actions too. Fastlane is usually backwards compatible with xcodebuild.

mokagio · 2023-08-31T04:54:17Z

Also, we get to keep the scaffold Xcode project as lean as possible, which could be beneficial too.

Okay. I can stand behind that 👍

BTW, I believe you can also pass build settings to fastlane actions too. Fastlane is usually backwards compatible with xcodebuild.

Sure. I'm pretty sure that would be possible out of the box. My point was that with the settings in the xcodeproj they would automatically apply to a build call made from Fastlane.

Previously it was done via a build phase. See this GitHub conversation for the rationale: #6101 (comment)

peril-wordpress-mobile · 2023-08-31T05:54:35Z

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

peril-wordpress-mobile · 2023-08-31T05:54:35Z

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

peril-wordpress-mobile · 2023-08-31T05:54:35Z

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

mokagio · 2023-08-31T05:55:07Z

@crazytonyli addressed your suggestion in e277e38

Thanks! 🙇‍♂️

crazytonyli

Thanks for taking my suggestions! I have left a couple of comments which may help further simplify the solution.

crazytonyli · 2023-08-31T21:24:54Z

ios-xcframework/BuildPhases/set_current_project_version.sh

+    /usr/libexec/PlistBuddy -c "Set :CFBundleVersion $version" "$plist"
+    /usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString $version" "$plist"
+  fi
+done


This script is not used anymore, I think?

Thanks! I had forgotten about that one.

Done via 6d5a033

crazytonyli · 2023-08-31T21:30:01Z

ios-xcframework/build.sh

@@ -63,6 +66,9 @@ if [ ! -d $HERMES_XCFRAMEWORK ]; then
  exit 1
 fi

+# Fail early if cannot fetch version
+VERSION=$(./bin/get_gutenberg_version)


I think we can use jq to replace this custom JSON parsing script: jq -e -r '.version' ./package.json?

I considered it, but decided against it to avoid having to ensure the tool is available locally.

Having said that... I guess it's safe to assume that to be the case and it doesn't take much to fail the script early if that's not the case 🤔

Addressed in 8779e2e

crazytonyli · 2023-08-31T21:30:36Z

ios-xcframework/bin/get_gutenberg_version

+
+# get the path to the parent folder using Dir methods
+package_name = 'package.json'
+path_to_package_json = File.join(File.dirname(File.dirname(File.dirname(File.absolute_path(__FILE__)))), package_name)


What do think using File.expand_path, instead of multiple dirname calls?

Can you elaborate on this suggestion?

The code uses File.dirname to have a platform-independent ... That is go up three parent folders.

There's no need for this to be platform-independent to be fair, but I find it a good habit to get into 🤷‍♂️

BTW, addressing this is no longer necessary since the automation uses jq as of 8779e2e

@crazytonyli

See conversation with @crazytonyli at #6101 (comment)

crazytonyli

Previously it was done via a build phase. See this GitHub conversation for the rationale: #6101 (comment)

Via RuboCop: Style/StderrPuts: Use `warn` instead of `$stderr.puts` to allow such output to be disabled.

It became unused in e277e38

@crazytonyli

See conversation with @crazytonyli at #6101 (comment)

mokagio marked this pull request as ready for review August 23, 2023 10:52

mokagio requested review from crazytonyli, dcalhoun, SiobhyB, geriux and fluiddot August 23, 2023 10:52

mokagio enabled auto-merge August 23, 2023 10:52

dcalhoun reviewed Aug 25, 2023

View reviewed changes

mokagio added a commit that referenced this pull request Aug 31, 2023

Move XCFramework version setting to build script

e277e38

Previously it was done via a build phase. See this GitHub conversation for the rationale: #6101 (comment)

crazytonyli requested changes Aug 31, 2023

View reviewed changes

mokagio added a commit that referenced this pull request Sep 1, 2023

Use jq instead of dedicated script to read Gutenberg version

8779e2e

See conversation with @crazytonyli at #6101 (comment)

mokagio requested a review from crazytonyli September 1, 2023 04:05

crazytonyli approved these changes Sep 4, 2023

View reviewed changes

mokagio added 6 commits September 5, 2023 11:10

Prefix App.js checker build phase in XCFramework with "Gutenberg"

5246c0a

Add build phase that sets XCFramework version based on Gutenberg version

398ee63

Move XCFramework version setting to build script

73e0a43

Previously it was done via a build phase. See this GitHub conversation for the rationale: #6101 (comment)

Use warn over $stderr.puts

cdec4b0

Via RuboCop: Style/StderrPuts: Use `warn` instead of `$stderr.puts` to allow such output to be disabled.

Remove unused set_current_project_version.sh build phase

802f922

It became unused in e277e38

Use jq instead of dedicated script to read Gutenberg version

9cb9689

See conversation with @crazytonyli at #6101 (comment)

mokagio force-pushed the mokagio/xcframework-info-plist-version branch from 8779e2e to 9cb9689 Compare September 5, 2023 01:13

mokagio merged commit 85151fd into trunk Sep 5, 2023

mokagio deleted the mokagio/xcframework-info-plist-version branch September 5, 2023 02:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same version as `package.json` in XCFramework #6101

Use the same version as `package.json` in XCFramework #6101

mokagio commented Aug 23, 2023 •

edited

Loading

crazytonyli commented Aug 25, 2023

dcalhoun left a comment

mokagio commented Aug 29, 2023

crazytonyli commented Aug 29, 2023

mokagio commented Aug 31, 2023

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

edited

Loading

peril-wordpress-mobile bot commented Aug 31, 2023

peril-wordpress-mobile bot commented Aug 31, 2023

mokagio commented Aug 31, 2023

crazytonyli left a comment

crazytonyli Aug 31, 2023

mokagio Sep 1, 2023

mokagio Sep 1, 2023

crazytonyli Aug 31, 2023

mokagio Sep 1, 2023

mokagio Sep 1, 2023

crazytonyli Aug 31, 2023

mokagio Sep 1, 2023

mokagio Sep 1, 2023

crazytonyli left a comment

Use the same version as package.json in XCFramework #6101

Use the same version as package.json in XCFramework #6101

Conversation

mokagio commented Aug 23, 2023 • edited Loading

crazytonyli commented Aug 25, 2023

dcalhoun left a comment

Choose a reason for hiding this comment

mokagio commented Aug 29, 2023

crazytonyli commented Aug 29, 2023

mokagio commented Aug 31, 2023

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

peril-wordpress-mobile bot commented Aug 31, 2023

peril-wordpress-mobile bot commented Aug 31, 2023

mokagio commented Aug 31, 2023

crazytonyli 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

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

crazytonyli left a comment

Choose a reason for hiding this comment

Use the same version as `package.json` in XCFramework #6101

Use the same version as `package.json` in XCFramework #6101

mokagio commented Aug 23, 2023 •

edited

Loading

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

edited

Loading