-
Notifications
You must be signed in to change notification settings - Fork 585
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
RJS-2101: "Build optimizations" for iOS and Android #6650
Conversation
e0482ca
to
0b3a7e7
Compare
943dabc
to
29868fe
Compare
- uses: actions/setup-java@v3 | ||
if: ${{ matrix.variant.os == 'android' }} | ||
with: | ||
distribution: 'zulu' # See 'Supported distributions' for available options | ||
java-version: '17' | ||
|
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.
- name: Setup Java | ||
if: ${{ matrix.variant.os == 'android' }} | ||
uses: actions/setup-java@v3 | ||
with: | ||
distribution: 'zulu' # See 'Supported distributions' for available options | ||
java-version: '11' | ||
|
||
- name: Setup Android SDK | ||
if: ${{ matrix.variant.os == 'android' }} | ||
uses: android-actions/setup-android@v2 | ||
|
||
- name: Install NDK | ||
if: ${{ matrix.variant.os == 'android' }} | ||
run: sdkmanager --install "ndk;${{ env.NDK_VERSION }}" |
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.
These were moved into a separate Android specific prebuild-android
job.
"pod-install:ci": { | ||
"command": "pod-install", | ||
"clean": "if-file-deleted", | ||
"files": [ | ||
"ios/Podfile", | ||
"../../../packages/realm/RealmJS.podspec" | ||
], | ||
"output": [ | ||
"ios/Pods", | ||
"ios/Podfile.lock", | ||
"ios/RealmTests.xcworkspace" | ||
], |
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.
No need for dependencies on other scripts when testing on CI, since that's already handled by other jobs uploading artifacts.
@@ -1,111 +0,0 @@ | |||
|
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.
Deleting this entirely, since almost all the information was outdated and would have to be re-tested anyway.
|
||
s.frameworks = uses_frameworks ? ['React'] : [] | ||
|
||
s.library = 'c++', 'z', 'compression' | ||
|
||
s.pod_target_xcconfig = { | ||
# Setting up clang | ||
'CLANG_CXX_LANGUAGE_STANDARD' => 'c++17', | ||
'CLANG_CXX_LANGUAGE_STANDARD' => 'c++20', |
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're using C++20 features in the binding.
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.
Should we merge or close #5053?
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.
Perhaps 🤔 I haven't double checked the current state of the Node.js binding.
#pragma GCC diagnostic push | ||
#pragma GCC diagnostic ignored "-Wswitch" |
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.
Adding this to silence warnings that might be confusing to our users.
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.
But will we accidentally overlook something?
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 say very unlikely, since it's pop'ed just after the code that relies on it.
const { submitAnalytics } = require("../dist/scripts/submit-analytics"); | ||
submitAnalytics().catch(console.error); | ||
} catch (err) { | ||
console.error(err instanceof Error ? err.message : err); |
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 only prints if loading the analytics file fails, since submitAnalytics
returns a rejected promise if failing. This is mostly to avoid any uncaught exceptions resulting in an unclean exit code.
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.
Your comment could be added to the code as well if you'd like 🙂
@@ -0,0 +1,172 @@ | |||
//////////////////////////////////////////////////////////////////////////// |
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 is the build script for the Android prebuild.
@@ -0,0 +1,181 @@ | |||
//////////////////////////////////////////////////////////////////////////// |
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 is the build script for the Apple / iOS prebuild.
@@ -42,7 +42,6 @@ | |||
// the other information on. |
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 I moved more scripts into TypeScript I took the time to "translate" the submit-analytics script as well.
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.
TypeScript first team 😄
const __filename = fileURLToPath(import.meta.url); | ||
const __dirname = path.dirname(__filename); | ||
const realmPackagePath = path.resolve(__dirname, ".."); | ||
const realmPackagePath = path.resolve(__dirname, "../.."); |
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.
It moved up a level because of the src
directory.
function isAnalyticsDisabled() { | ||
let isDisabled = false; | ||
|
||
// NODE_ENV is commonly used by JavaScript framework | ||
if ("NODE_ENV" in process.env) { | ||
isDisabled |= process.env["NODE_ENV"] === "production" || process.env["NODE_ENV"] === "test"; | ||
if ("REALM_DISABLE_ANALYTICS" in process.env || "CI" in process.env) { | ||
return true; | ||
} else if ( | ||
// NODE_ENV is commonly used by JavaScript framework | ||
process.env["NODE_ENV"] === "production" || | ||
process.env["NODE_ENV"] === "test" | ||
) { | ||
return true; | ||
} | ||
|
||
// If the user has specifically opted-out or if we're running in a CI environment | ||
isDisabled |= "REALM_DISABLE_ANALYTICS" in process.env || "CI" in process.env; | ||
|
||
return isDisabled; | ||
} |
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.
Refactored this to use returns instead of the more exotic |=
operator.
@@ -175,15 +170,15 @@ function getInstallationMethod() { | |||
if (userAgent) { | |||
return userAgent.split(" ")[0].split("/"); | |||
} else { | |||
return "unknown"; | |||
return ["unknown", "?.?.?"]; |
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 was actually a bug I discovered during the refactor.
const payload = { | ||
webHook: { | ||
event: "install", | ||
properties: data, | ||
}, | ||
event: "install", | ||
properties: data, |
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.
No need to wrap this in a webHook
object, since it was always used as payload.webHook
.
if (process.argv[1] === fileURLToPath(import.meta.url)) { | ||
submitAnalytics().catch(console.error); | ||
} |
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 remains in the calling <root>/script/submit-analytics.js
.
…s of the "realm" package
Co-authored-by: Kenneth Geisshirt <kenneth.geisshirt@mongodb.com>
Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>
60275c7
to
48e8eaa
Compare
What, How & Why?
Important
With this change, our SDK is no longer pinned to a specific ABI of JSI and we expect the next release of our SDK (after merging this PR) to be backwards and forwards compatible with many past versions of React Native.
This is my friendly takeover of #6396, by reverting the compilation of Realm Core from source and instead distributing prebuilds via our NPM package:
npm run prebuild-apple --workspace realm
):npm run prebuild-android --workspace realm
):npm run prebuild-node --workspace realm
).This also moves all binding related code (prebuilt or not), including that previously in
packages/realm/react-native
intopackages/realm/binding
in platform specific directories.We won't support compiling our prebuilds from from the NPM archive as users still need to
git clone
our mono-repository to produce a build from source.It's worth mentioning that this PR introduce an internal CLI incapsulating the complexity of invoking the CMake scripts for Android and Apple platforms. I imagine we could extend this to wrap cmake-js to provide a single cohesive experience for building our prebuilds in the future: This is a replacement for the
packages/realm/scripts
for Android and iOS that we currently use.This is the resulting size of our tarball:
Here's a bit more context on this change and why we choose the approach we did:
As we started implementing out "React Native SDK Build Optimizations" project we realized a few things:
In the context of the above, if we choose to include prebuild binaries for iOS and Android of Realm Core in the NPM package, which will be ABI independent on the React Native version (except for the caveat of NDK version, which has a default that I'd expect 95+% to leave unchanged) ... do we actually need to support compiling from source when installing the realm package via NPM as the original project scope suggests? I'd expect the need for that to be very rare (as it is right now) and it does come with drawbacks:
The alternative is to ask developers that want to consume Realm JS and Realm Core from source to perform a checkout of the repository, run the relevant build script and npm pack the SDK package themselves.
Another (now closed) PR has some more context on the problem this PR is solving: #4027
Notes for the reviewer:
packages/realm/binding/CMakeLists.txt
and instead either refer to theCMakeLists.txt
inbindgen/vendor/realm-core
directly orCMakeLists.txt
files for the individual platforms, instead of having an shared CMake project used by all bindings. This, because it felt the requirements for each platform was too different across platforms to play for the complexity of having a shared project.☑️ ToDos
files
to ensure all files needed to compile is included.Compatibility
label is updated or copied from previous entryCOMPATIBILITY.md
package.json
s (if updating internal packages)Breaking
label has been applied or is not necessary