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

[ios][prebuild] fixed inclusion of resources in swift package #50050

Closed

Conversation

chrfalch
Copy link
Contributor

@chrfalch chrfalch commented Mar 17, 2025

Summary:

We had some issues with the Swift package build step where we saw an error message when we included resources and couldn't find out why this was happening.

After systematically going through the generated swift package file and looking for a reason I found a mistake.

When we generate the Package.swift file we pass all compilerFlags from the configuration of the target to both cpp/c flags - which in the case of the folly target ends up being passed to the dependency scanner which isn't too happy about this c++ flag.

The solution was to split compilerFlags into cCompilerFlags and cxxCompilerFlags.

This commit fixes this by:

  • split compilerFlags into cCompilerFlags and cxxCompilerFlags.
  • Updated configuration with correct settings
  • Updated Package.swift generation to use these new flags
  • Fixed issue with the copy bundles step that didn't copy the directory in some cases.

Changelog:

[INTERNAL] - Fixed processing resources in the generated swift package for the RN Dependencies/prebuild

Test-plan

Test by prebuilding RNDependencies, include the XCFramework in a new app and try to load resource bundles:

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
  std::string input = "3.1416 xyz ";
  double_conversion::DoubleToStringConverter::EcmaScriptConverter();
  LOG(INFO) << "Hello from GLOG";
  fmt::print("Hello, world from FMT!\n");
  BOOST_ASSERT(100 == 100);
  double result;
  fast_float::from_chars(input.data(), input.data() + input.size(), result);
  LOG(INFO) << "Answer :" << result;
  
  NSArray *frameworks = [NSBundle allFrameworks];

  for (NSBundle *framework in frameworks) {
    NSString *frameworkName = framework.bundleURL.lastPathComponent;
    if ([frameworkName isEqualToString: @"ReactNativeDependencies.framework"]) {
      [self loadBundle:framework bundleName:@"ReactNativeDependencies_glog"];
      [self loadBundle:framework bundleName:@"ReactNativeDependencies_boost"];
      [self loadBundle:framework bundleName:@"ReactNativeDependencies_folly"];
      break;
    }
  }
  return YES;
}

- (void) loadBundle:(NSBundle*)framework bundleName: (NSString*)bundleName {
  NSBundle *bundle = [NSBundle bundleWithURL:[framework bundleURL]];
  NSURL *bundleURL = [bundle URLForResource:bundleName withExtension:@"bundle"];
  NSBundle *resourceBundle = [NSBundle bundleWithURL:bundleURL];
  NSURL* url = [resourceBundle URLForResource:@"PrivacyInfo" withExtension:@"xcprivacy"];
  if (url == nil) {
    LOG(ERROR) << "Could not find PrivacyInfo.xcprivacy in the " << [bundleName UTF8String] << " bundle";
  } else {
    LOG(INFO) << "Found PrivacyInfo.xcprivacy in " << [bundleName UTF8String] << ".";
  }
}

We had some issues with the Swift package build step where we saw an error message when we included resources and couldn't find out why this was happening.

After systematically going through the generated swift package file and looking for a reason I found a mistake.

When we generate the Package.swift file we pass all compilerFlags from the configuration of the target to both cpp/c flags - which in the case of the folly target ends up being passed to the dependency scanner which isn't too happy about this c++ flag.

The solution is to break out the c++ stdlib version as a separate setting in the configuration and pass it only to the `cxxSettings` section in our final Package.swift file.

This commit fixes this by:
- Adding a new field in the dependency settings struct called `cppVersion`
- Updated configuration with correct resources and cppVersion for relevant targets
- Now emits the correct cppVersion to the cxxSettings section
- Fixed issue with the copy bundles step that didn't copy the directory in some cases.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 17, 2025
@react-native-bot
Copy link
Collaborator

react-native-bot commented Mar 17, 2025

Warnings
⚠️ 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against f771711

@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 Mar 17, 2025
Instead of passing the compilerFlags settings to both c and cxx these are now split in two so that it is easy to provide different flags to the cxx compiler (setting the correcct std-lib version f.ex).
- Added c++ standard to all targets
- Removed -Wno-everything from all targets
@chrfalch chrfalch requested a review from cipolleschi March 17, 2025 10:41
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Thanks for applying all the feedback!

@facebook-github-bot
Copy link
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 Mar 18, 2025
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 9cf1383.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @chrfalch in 9cf1383

When will my fix make it into a release? | How to file a pick request?

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. 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.

4 participants