Skip to content

Conversation

@ryantrem
Copy link
Member

@ryantrem ryantrem commented Jul 7, 2020

This change supports building an NPM package with pre-compiled binaries. Notable pieces of this change:

  • There is a new Package directory, which contains a few static support files, and a Gulp based build script. I decided to use Gulp because it is cross platform, commonly used for web-tech based projects, and makes it easy to break the build script into separable pieces. My plan is to have a PR build that does gulp buildIOS and gulp buildAndroid in parallel, and have the CI build that does gulp (invokes the default function) to build both iOS and Android and then publish the package. These PR/CI build changes will come in a later PR.
  • The build script builds Android by just building the Playground app and then copying over the libBabylonNative.so files for different architectures, plus some Android specific interop source code files that are compiled into the consuming app.
  • For Android, we additionally include a static build.gradle that is exactly what is generated by the React Native CLI tools for new packages (which is compatible with React Native consuming apps), plus two additions: 1) It produces a build error if the min SDK version is less than 18 (required for GLES3), 2) it adds a dependency on ARCore.
  • The build script builds iOS through a CMakeLists.txt that includes the regular iOS CMakeLists.txt from the source package, and then adds a few extra steps to scoop up all the static libs we care about into a single directory. These libs are run through the lipo tool to combine the different architectures for each lib into a set of "universal libs" for all supported architectures. This makes them compatible with CocoaPods, where we do not have the ability to specify different static libs for different architectures. The "universal libs" plus some iOS specific interop source code files are copied over to the precompiled package directory.
  • For iOS, we additionally include a static podspec that includes all the static libs, plus the loose source files that the consuming app needs to compile.
  • The PackageTest app's @babylonjs/react-native dependency now points to the locally built binary package (rather than the source package).
  • The PackageTest app's xcworkspace no longer references a ReactNativeBabylon xcodeproj since it is now pulling in precompiled binaries.

Other smaller changes included in this PR:

  • Update to the BabylonNative submodule to the latest master, which includes fixes for warnings as errors on iOS and Android. This pulled in an updated version of glslang, which produces two additional libs, which needed to be added to the list of libs to link in the source-based package's podspec.
  • I updated the iOS deployment target of the PackageTest app to be 12 instead of 9. This just got missed on the initial checkin, and actually results in build errors for Release (since the Babylon React Native package declares a minimum deployment target of 12).
  • The iOS CMakeLists.txt now explicitly uses ${CMAKE_CURRENT_LIST_DIR} since paths are relative to the CMakeLists.txt file itself. Without specifying this, paths are relative to the "top level" CMakeLists.txt (in this case, the one in the Package directory that includes this CMakeLists.txt.
  • Updates to the README.md to reflect the changes around the binary packaging.

Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

Can't say I understand everything that is going on in this PR, but looks good to me.

await exec('npm pack', {cwd: 'Assembled'});
};

const copyFiles = gulp.parallel(copyCommonFiles, copyIOSFiles, copyAndroidFiles);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the difference between gulp.parallel and Promise.all. Why use one vs. the other?

Copy link
Member Author

Choose a reason for hiding this comment

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

Gulp functions/tasks can return Promises, but they can also return a variety of other objects representing asynchronous work (such as those using the src/pipe/dest stuff). gulp.parallel understands all these, and also makes it so gulp understands the task hierarchy and can display reasonable command line output for the tasks that it is running.

Comment on lines +82 to +94
await new Promise(resolve => {
gulp.src('Android/build.gradle')
.pipe(gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/src**/main/AndroidManifest.xml'))
.pipe(gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/src**/main/java/**/*'))
.pipe(gulp.dest('Assembled/android'))
.on('end', resolve);
});

await new Promise(resolve => {
gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/build/intermediates/library_and_local_jars_jni/release/jni/**/*')
.pipe(gulp.dest('Assembled/android/src/main/jniLibs/'))
.on('end', resolve);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is using new Promise really necessary here? Seems like you can use gulp.parallel like below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, if i use gulp.parallel, then Gulp treats these as individual atomic "tasks." I think it makes more sense to have these grouped together in a single "logical task."

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean use gulp.parallel on the "tasks" returned by each section of the gulp.pipe.pipe:

Suggested change
await new Promise(resolve => {
gulp.src('Android/build.gradle')
.pipe(gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/src**/main/AndroidManifest.xml'))
.pipe(gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/src**/main/java/**/*'))
.pipe(gulp.dest('Assembled/android'))
.on('end', resolve);
});
await new Promise(resolve => {
gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/build/intermediates/library_and_local_jars_jni/release/jni/**/*')
.pipe(gulp.dest('Assembled/android/src/main/jniLibs/'))
.on('end', resolve);
});
gulp.parallel(
gulp.src('Android/build.gradle')
.pipe(gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/src**/main/AndroidManifest.xml'))
.pipe(gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/src**/main/java/**/*'))
.pipe(gulp.dest('Assembled/android')),
gulp.src('../Apps/Playground/node_modules/@babylonjs/react-native/android/build/intermediates/library_and_local_jars_jni/release/jni/**/*')
.pipe(gulp.dest('Assembled/android/src/main/jniLibs/'))
);

Does this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that will create two nodes in the task hierarchy named "anonymous function" (or something like that). Talked offline, leaving as is.

@ryantrem ryantrem merged commit 4bbfb84 into BabylonJS:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants