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.

Sign up for GitHub

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

[iOS] Use relative paths for pod spec to keep checksum stable #31195

Closed
wants to merge 4 commits into from
Closed

[iOS] Use relative paths for pod spec to keep checksum stable #31195

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2badc6f
Use relative paths for pod spec to keep checksum stable
thaapasa Mar 19, 2021
328bdec
Fix path to generate-specs.sh to be relative
thaapasa Mar 19, 2021
513501e
Simplify
thaapasa Mar 19, 2021
c2211d5
Adjust working directory to match relative path
thaapasa Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions scripts/react_native_pods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def use_react_native_codegen!(spec, options={})
return if ENV['DISABLE_CODEGEN'] == '1'

# The path to react-native (e.g. react_native_path)
prefix = options[:path] ||= File.join(__dir__, "..")
prefix = options[:path] ||= "../../node_modules/react-native"
Copy link

@martintreurnicht martintreurnicht Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is the correct path for the script_phase but not for prepare_command, prepare_command is relative to the podspec, and not PODS_ROOT like script_phase. Not sure how this is working for you, All your generated code should be in the wrong directory. It might work but is not the expected result. The generated code should exist under node_modules/react-native/React/

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @martintreurnicht, you are correct. The prepare command was run for the wrong path; after running pod install an extra node_modules directory was created under node_modules/react-native.

The was working for me because apparently the prepare command is a bit redundant; after checking out react-native the empty files are already there so just the timestamps were not updated.

I've fixed the script now. The end result is a hack that I do not like, but at least it should work now. If someone knows better how the Pod build works and how this problem should be properly addressed please let me know (or make another PR and this one can be closed).

For reference (TIL): the prepare_command is run with pod install; you can check the working directory by adding pwd && exit 1 at the beginning of it (spoiler: it's /local/dev/path/node_modules/react-native/React/FBReactNativeSpec). Also, the script_phase is run with the actual app build (yarn run-ios).

Copy link
Contributor

Choose a reason for hiding this comment

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

First off, thanks for bringing this issue to my attention! I agree this needs to be fixed.
I am not sure at this moment that hard-coding this prefix to "../../node_modules/react-native" will work in every scenario. While this path is valid for standalone apps that consume react-native from npm, the path will not exist if this script is invoked when pod install is run within packages/rn-tester (e.g. when building RNTester as a contributor to React Native).

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prefix = options[:path] ||= "../../node_modules/react-native"
prefix = options[:path] ||= Pathname.new(File.join(__dir__, "..")).relative_path_from(File.join(__dir__))


# The path to JavaScript files
srcs_dir = options[:srcs_dir] ||= File.join(prefix, "Libraries")
Expand Down Expand Up @@ -192,7 +192,7 @@ def use_react_native_codegen!(spec, options={})
:name => 'Generate Specs',
:input_files => [srcs_dir],
:output_files => ["$(DERIVED_FILE_DIR)/codegen-#{codegen_modules_library_name}.log"].concat(generated_files),
:script => "set -o pipefail\n\nbash -l -c '#{env_vars} CODEGEN_MODULES_LIBRARY_NAME=#{codegen_modules_library_name} #{File.join(__dir__, "generate-specs.sh")}' 2>&1 | tee \"${SCRIPT_OUTPUT_FILE_0}\"",
:script => "set -o pipefail\n\nbash -l -c '#{env_vars} CODEGEN_MODULES_LIBRARY_NAME=#{codegen_modules_library_name} ../../node_modules/react-native/scripts/generate-specs.sh' 2>&1 | tee \"${SCRIPT_OUTPUT_FILE_0}\"",
Copy link

Choose a reason for hiding this comment

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

This is just a question to showcase my ignorance: I have fixed this locally by using SRCROOT which will resolve to an absolute path at runtime, because I was under the impression that would be the more 'canonical' way. Do you think it is correct to do so (my build passes, so that's at least 1 criterion) or is it better to have it relative like you have done?

Copy link
Author

Choose a reason for hiding this comment

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

Is this SRCROOT something that is already defined by the build process (and does it point to the same absolute directory in the prepare command and script phase)? If so, that would definitely be better in my opinion; I don't like this relative addressing at all (I just don't know enough about this build system to suggest a good fix for the issue).

Copy link

Choose a reason for hiding this comment

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

It's pretty obscure to me too, luckily in the logs on our build server all environment variables are printed at one point. Here is a small excerpt of ones that I think could be relevant in this context:

export PODS_ROOT\=/Users/runner/work/1/s/ios/Pods
export PODS_TARGET_SRCROOT\=/Users/runner/work/1/s/ios/Pods/../../node_modules/react-native/React/FBReactNativeSpec
export SOURCE_ROOT\=/Users/runner/work/1/s/ios/Pods
export SRCROOT\=/Users/runner/work/1/s/ios/Pods

There is some overlap here and my initial googling hasn't exactly given me any pointers as to which ones are future proof. I actually ended up using PODS_ROOT for the prefix, because on my machine SRCROOT turned out to be pointing to /, which gave me permission errors.

Anyway, here is some documentation as well, which could potentially proof helpful: https://developer.apple.com/library/archive/documentation/DeveloperTools/Reference/XcodeBuildSettingRef/1-Build_Setting_Reference/build_setting_ref.html.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to test this myself and verified that I have similar variables properly set in the script phase. However, when prepare command is run (pod install) all the variables I tested were unset, so the paths would be incorrect in that phase if the paths are based on those variables.

:execution_position => :before_compile,
:show_env_vars_in_log => true
Copy link

Choose a reason for hiding this comment

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

This setting is probably the reason those variables are printed.

}
Expand Down