Skip to content

fix: [Codegen] log supported apple platforms if there are any#42819

Closed
okwasniewski wants to merge 1 commit into
react:mainfrom
okwasniewski:fix/codegen-message
Closed

fix: [Codegen] log supported apple platforms if there are any#42819
okwasniewski wants to merge 1 commit into
react:mainfrom
okwasniewski:fix/codegen-message

Conversation

@okwasniewski

Copy link
Copy Markdown
Contributor

Summary:

This PR adds check if there are any supported platforms to log.

For built-in modules this was logging empty line (as some of them doesn't contain podspecs):

CleanShot 2024-02-02 at 15 54 42@2x

Changelog:

[GENERAL] [FIXED] - Log Codegen supported platforms if any are available

Test Plan:

Run Codegen and check if it prints empty Supported Apple platforms

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Feb 2, 2024
@okwasniewski

Copy link
Copy Markdown
Contributor Author

cc: @dmytrorykun

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 2, 2024
@analysis-bot

Copy link
Copy Markdown
Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,197,357 -15
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 20,576,236 -1
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 069c244
Branch: main

@dmytrorykun

Copy link
Copy Markdown

@okwasniewski This got me thinking. According to the Podspec Syntax Reference:

Leaving this blank means the Pod is supported on all platforms.

Is it a valid case when both platform and deployment_target are not defined in the podspec? How do we interpret that?

@okwasniewski

Copy link
Copy Markdown
Contributor Author

I think this case is happening for core modules that don't have Podspec defined in the same directory as the rest of the code.

Because for built-in modules we have:

packages/react-native/Libraries/<Module> (which has Podspec) but the actual codegen specs are sitting in packages/react-native/src/private/ so I think for normal libraries this won't happen

@okwasniewski

okwasniewski commented Feb 2, 2024

Copy link
Copy Markdown
Contributor Author

Ah okay I miss read your question.. Is it possible for RN libraries to not specify a target? This would go against the docs and soon against the "Golden library template".

Edit: I think if they are not defined we should do nothing (don't add any compiler conditionals) as it means that it supports every platform. I can create a follow up PR for this on Monday and test this edge case

@dmytrorykun

Copy link
Copy Markdown

@okwasniewski I just did some local tests with my test library.
The podspec looks like this:

require 'json'

package = JSON.parse(File.read(File.join(__dir__, 'package.json')))

Pod::Spec.new do |s|
  s.name            = 'OSSLibraryExample'
  s.version         = package['version']
  s.summary         = package['description']
  s.description     = package['description']
  s.homepage        = package['homepage']
  s.license         = package['license']
  s.author          = 'Meta Platforms, Inc. and its affiliates'
  s.source          = { :git => package['repository'], :tag => '#{s.version}' }

  s.source_files = 'ios/**/*.{h,m,mm,cpp}'

  install_modules_dependencies(s)
end

Note that there is neither platform nor deployment_target in the podspec.

pod install finishes successfully. But the generated code looks like this:

#if !TARGET_OS_IOS && !TARGET_OS_OSX && !TARGET_OS_TV && !TARGET_OS_VISION

    {"SampleNativeComponent", SampleNativeComponentCls}, // 1
#endif

Which means that SampleNativeComponent is not available on any platform.

I know that this issue is not directly related to this PR, but could you please submit a followup PR were this case would be "all platforms are supported"?

@okwasniewski

Copy link
Copy Markdown
Contributor Author

@dmytrorykun Sure, I will handle this case with a followup PR next week. Thanks for the review 🙌

@okwasniewski

Copy link
Copy Markdown
Contributor Author

Hey @dmytrorykun, anything more I can do to get this PR merged?

@cipolleschi

Copy link
Copy Markdown
Contributor

So @dmytrorykun is on PTO until the 12th. He asked me to take care of this PR, but the import is failing. I'll try again tomorrow morning to see if the system recovers!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 8, 2024
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in 2ca7bec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants