-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Refactored prebuild of react native dependencies #49614
base: main
Are you sure you want to change the base?
Refactored prebuild of react native dependencies #49614
Conversation
- Refactored prebuild into separate folder with simple cli interface - Added scheme parameter to be able to work with multiple frameworks - Refactored steps so that cleaning happens before building (easier to see / debug the output) - Added support for more configuration settings - Using glob for setting files (easier to convert from podspec) - Added more flexible configuration settings for c/c++ compiler - Added support for including resources (copied to Resources in framework) - Changed `copyHeaderRule` to `headerSkipFolderNames` which takes a partial path that will be removed from the target directory when copying headers - Added support for prepareScript to be a direct command (if it is not a file) - Added code generation of Package.swift - Added prebuild of: - glog - double-conversion - fast_float - fmt - boost - socket-rocket - Added support for embedding resources - Added support for copying resource bundles - Updated gitignore. Tested in this `AppDelegate.mm`: Imports: ```obj-c #import "AppDelegate.h" #import <glog/logging.h> #import <double-conversion.h> #include <fmt/core.h> #include <boost/assert.hpp> #include <fast_float/fast_float.h> #include <string> #import <SocketRocket/SRWebSocket.h> ``` Code: ```obj-c - (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"]) { NSBundle *bundle = [NSBundle bundleWithURL:[framework bundleURL]]; NSURL *bundleURL = [bundle URLForResource:@"ReactNativeDependencies_glog" withExtension:@"bundle"]; NSBundle *resourceBundle = [NSBundle bundleWithURL:bundleURL]; NSURL* url = [resourceBundle URLForResource:@"PrivacyInfo" withExtension:@"xcprivacy"]; if (url == nil) { @throw [NSException exceptionWithName:@"ResourceNotFoundException" reason:@"Could not find PrivacyInfo.xcprivacy in ReactNativeDependencies_glog bundle" userInfo:nil]; } break; } } return YES; } ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some time and I went through the code as I was too curious!
Great job, really, the code is much cleaner and it reads much better.
I left some questions here and there and also some suggestions. There are a couple of bits that I'd rather we change, but we can discuss them on Monday.
`${scheme}.framework`, | ||
'Headers', | ||
); | ||
if (fs.existsSync(headerFolder)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably skip this step. We can always delete the folder, if it's not there, nothing happens.
*/ | ||
|
||
//TODO: Add support for mac, Mac (catalyst), tvOS, xros and xrsimulator | ||
const platforms /*: $ReadOnlyArray<Platform> */ = ['iOS', 'iOS Simulator']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle all the OOT platforms here. In the script in main, I already listed them. I'd also use all lowercase letters and I'd reuse what is there in the script in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, glog now builds for all the platforms. Can we verify that this is true also for all the other dependencies?
const argv = await cli.argv; | ||
|
||
// Verify that the platforms argument is valid | ||
const invalidPlatforms = arrayLike(argv.platforms).filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather extract all the validation in a separate function.
} | ||
|
||
// Prepare platforms and dependencies | ||
const resolved_platforms = platforms.filter(p => argv.platforms.includes(p)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const resolved_platforms = platforms.filter(p => argv.platforms.includes(p)); | |
const resolvedPlatforms = platforms.filter(p => argv.platforms.includes(p)); |
|
||
// Prepare platforms and dependencies | ||
const resolved_platforms = platforms.filter(p => argv.platforms.includes(p)); | ||
const resolved_dependencies = dependencies.filter(d => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const resolved_dependencies = dependencies.filter(d => | |
const resolvedDependencies = dependencies.filter(d => |
headerSkipFolderNames?: string, | ||
}>; | ||
export type Define = $ReadOnly<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we mean with Defines
? To which field does this correspond?
const targetDir = path.dirname(targetPath); | ||
|
||
fs.mkdirSync(targetDir, {recursive: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be lifted up from the sourceFiles.forEach
. We try to create a folder for each file, otherwise!
if (fs.existsSync(output)) { | ||
fs.rmSync(output, {recursive: true, force: true}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably just try and delete it
/** | ||
* Copies the bundles in the source frameworks to the target xcframework inside the xcframework's Resources folder | ||
*/ | ||
function copyBundles( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we copy the headers to the .framework
folders, before creating the XCFrameworks, wouldn't be easier if we do the same for the resources?
Perhaps we can just reuse part fo the code for copying the headers. WDYT?
function copyDirectory(source /*: string*/, target /*: string*/) { | ||
if (!fs.existsSync(target)) { | ||
fs.mkdirSync(target, {recursive: true}); | ||
} | ||
// Read all files in source | ||
const files = fs.readdirSync(source); | ||
files.forEach(file => { | ||
const sourceFile = path.join(source, file); | ||
const targetFile = path.join(target, file); | ||
if (fs.statSync(sourceFile).isDirectory()) { | ||
copyDirectory(sourceFile, targetFile); | ||
} else { | ||
fs.copyFileSync(sourceFile, targetFile); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitiely not a JS expert, but why implementing the copy like this, instead of using the execSync(
mv -p src dest)` command?
This is one line that relies on a well known working code. While reimplementing the move might bring in some bugs.
Perhapst there are some benefits that I'm not aware of...
Summary:
Refactored, extended and implemented almost all (except for Folly) dependencies:
copyHeaderRule
toheaderSkipFolderNames
which takes a partial path that will be removed from the target directory when copying headersChangelog:
[INTERNAL] - Changed and extended prebuild of React Native Dependencies
Test Plan:
Tested in this
AppDelegate.mm
:Imports:
Code: