switch to consuming bindgen from realm-core repo #5769

RedBeard0531 · 2023-04-26T11:01:00Z

Now we are tracking https://github.com/realm/realm-core/tree/bindgen which has what used to be packages/realm/bindgen/vendor/bindgen-lib at bindgen. There is still some separating to do, such as moving the wireit tasks out of realm-core's bindgen package.json, and making that package.json freestanding, but I want to lock in this progress first.

(Adding Yavor and Nikola mostly as an FYI)

RedBeard0531 · 2023-04-26T11:02:17Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "docs/jsdoc-template"]
 	path = docs/jsdoc-template
 	url = https://github.com/realm/realm-jsdoc.git
-[submodule "vendor/realm-core"]
-	path = vendor/realm-core
+[submodule "packages/realm/bindgen/vendor/realm-core"]


git mv didn't change this line and I wasn't 100% sure if I should, but I decided to do it manually. Let me know if I shouldn't have.

It worked wonderfully when I pulled and did a git submodule update --init --recursive locally.

fealebenpae · 2023-04-26T11:04:04Z

packages/realm/bindgen/CMakeLists.txt

@@ -78,6 +78,7 @@ if(DEFINED CMAKE_JS_VERSION)
    include(NodeJSTargets)
 endif()

+# TODO is this option dead? Do we want to support compiling with it set to OFF?


We never ended up implementing building from Core prebuilds so we might as well kill it.

I'd like to keep that option open as a potential optimization when we'd eventually start exploring moving back to deferring compilation to the developer's machine, instead of shipping a prebuilt binary for RN (to remove our strong dependency on the jsi.h ABI)

I think this setup is wrong either way. This (used to) let you import a binary realm-core lib, but then still build the realm-js C++ code. I think we need to defer building both, rather than just realm-core. So I propose removing this option for now*, to make room for reintroducing deferred building correctly later

*Not right now though. I'd rather land this PR, and have separate work to clean up the CML.txt file now that we no longer need to be concerned with merging changes with the non-bindgen version.

I'm fine with that 👍 Prebuilding Core with a particular NDK does introduce a dependency on the stdlib ABI as we've noticed before, so I'm fine with doing this correctly later.

RedBeard0531 · 2023-04-26T11:15:05Z

@kraenhansen How do I tell GHA to not lint every js file in realm-core? https://github.com/realm/realm-js/actions/runs/4807896465/jobs/8557223717?pr=5769#step:5:26. I think we either want it to lint nothing, or only lint the bindgen dir.

kraenhansen · 2023-04-26T11:27:03Z

How do I tell GHA to not lint every js file in realm-core?

@RedBeard0531 I've pushed a commit adding a .lintignore file to the realm/bindgen directory 👍

RedBeard0531 · 2023-04-26T11:30:09Z

I pulled this back to a draft. I'm going to move the package.json up to the root of realm-core, and that will have some effects on how we consume it from realm-js.

RedBeard0531 · 2023-04-27T10:55:23Z

Out of draft now that realm/realm-core#6546 has been merged into the realm-core:bindgen branch. There is still a weird linting issue that @kraenhansen said he would look into, but I think this is generally ready to review.

kraenhansen · 2023-04-27T12:17:37Z

packages/realm/bindgen/package.json

@@ -3,6 +3,6 @@
  "private": true,
  "type": "module",
  "workspaces": [
-    "packages/realm/bindgen/vendor/realm-core/bindgen/"
+    "packages/realm/bindgen/vendor/realm-core/"


Interesting. Any particular reason this needed to be moved to the root?

Without that, npm run foo and npx bar would only work when run from a subdirectory of bindgen. I thought it would be best if they worked when run from anywhere in the realm-core repo. realm/realm-core@3bad9ef should solve this for commands run by cmake in the $repo_root/build directory, but it seemed better to just make those commands work from anywhere in the repo.

In theory, I could have made a root package.json, with bindgen as a subworkspace, but that seems silly for realm-core which isn't really a JS project.

npx bar in particular would have the odd behavior of "working", but using the latest version of bar and its deps, rather than considering the versions from package.json

Yeah. When not invoked through an NPM script in the package.json, npx use a "globally" installed package.

kneth

Good to see that we are transferring it to Core 😌

RedBeard0531 · 2023-05-09T10:07:57Z

I was going to delete the whole /vendor directory, but then I noticed the /vendor/.npmignore file. Does anyone know if we need something like this in /packages/realm/bindgen/vendor?

kraenhansen · 2023-05-09T10:22:41Z

Does anyone know if we need something like this in /packages/realm/bindgen/vendor?

Running a pack in packages/realm (npm pack --dry-run) yields:

npm notice === Tarball Contents === 
npm notice 3.4kB  RealmJS.podspec                                                         
npm notice 8.5kB  package.json                                                            
npm notice 977B   react-native.config.js                                                  
npm notice 1.8kB  react-native/android/build.gradle                                       
npm notice 52.3kB react-native/android/gradle/wrapper/gradle-wrapper.jar                  
npm notice 230B   react-native/android/gradle/wrapper/gradle-wrapper.properties           
npm notice 5.1kB  react-native/android/gradlew                                            
npm notice 2.4kB  react-native/android/gradlew.bat                                        
npm notice 38B    react-native/android/settings.gradle                                    
npm notice 127B   react-native/android/src/main/AndroidManifest.xml                       
npm notice 4.6kB  react-native/android/src/main/java/io/realm/react/RealmReactModule.java 
npm notice 2.4kB  react-native/android/src/main/java/io/realm/react/RealmReactPackage.java
npm notice 6.6kB  react-native/android/src/main/java/io/realm/react/util/SSLHelper.java   
npm notice 109B   react-native/android/src/main/java/io/realm/react/Version.java          
npm notice 8.8MB  react-native/android/src/main/jniLibs/arm64-v8a/librealm.so             
npm notice 5.6MB  react-native/android/src/main/jniLibs/armeabi-v7a/librealm.so           
npm notice 9.8MB  react-native/android/src/main/jniLibs/x86_64/librealm.so                
npm notice 9.4MB  react-native/android/src/main/jniLibs/x86/librealm.so                   
npm notice 886B   react-native/ios/RealmReact/RealmReact.h                                
npm notice 5.2kB  react-native/ios/RealmReact/RealmReact.mm                               
npm notice 150B   react-native/ios/RealmReact/RealmReact.xcconfig                         
npm notice 829B   scripts/submit-analytics.mjs

Which doesn't include anything from the realm/bindgen directory, because it isn't listed in files. I believe this is fine for now, but we probably have to add it back into the files (as well as an .npmignore file) once we start supporting compiling from source again.

* main: switch to consuming bindgen from realm-core repo (#5769) # Conflicts: # packages/realm/bindgen/vendor/bindgen-lib/spec.yml

RedBeard0531 requested review from fealebenpae, kneth, kraenhansen and nirinchev April 26, 2023 11:01

cla-bot bot added the cla: yes label Apr 26, 2023

github-actions bot assigned RedBeard0531 Apr 26, 2023

RedBeard0531 commented Apr 26, 2023

View reviewed changes

fealebenpae reviewed Apr 26, 2023

View reviewed changes

RedBeard0531 marked this pull request as draft April 26, 2023 11:27

RedBeard0531 marked this pull request as ready for review April 27, 2023 10:53

kraenhansen reviewed Apr 27, 2023

View reviewed changes

kraenhansen approved these changes Apr 27, 2023

View reviewed changes

kneth approved these changes Apr 27, 2023

View reviewed changes

elle-j mentioned this pull request May 9, 2023

Added support for full text search #5797

Merged

2 tasks

RedBeard0531 and others added 8 commits May 9, 2023 12:09

switch to consuming bindgen from realm-core repo

2ca0510

Create .eslintignore to ignore vendor/realm-core

81d0382

move wireit config for schema generation to realm-js package.json

c5195af

Bump to ms/bindgen-work branch of core

b56f876

update remaining references to $repo_root/vendor/realm-core

fd30117

upgrade to latest merged realm-core:bindgen

b442598

Moved the eslintignore entry one level up

60e0ef0

pull in bindgen bump to v13.10.1

1e84910

RedBeard0531 force-pushed the ms/bindgen-from-core branch from 8dbdcc5 to 1e84910 Compare May 9, 2023 10:09

delete vendor/ directory in repo root

2eefb07

RedBeard0531 mentioned this pull request May 9, 2023

Binding resolution refactored #5802

Merged

pull in change to bindgen submodule's tsconfig

2ef9b6c

RedBeard0531 merged commit 63c2fcb into main May 10, 2023

RedBeard0531 deleted the ms/bindgen-from-core branch May 10, 2023 07:24

papafe added a commit that referenced this pull request May 12, 2023

Merge branch 'main' into fp/minimal-fts

d1c3bb9

* main: switch to consuming bindgen from realm-core repo (#5769) # Conflicts: # packages/realm/bindgen/vendor/bindgen-lib/spec.yml

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to consuming bindgen from realm-core repo #5769

switch to consuming bindgen from realm-core repo #5769

RedBeard0531 commented Apr 26, 2023

RedBeard0531 Apr 26, 2023

kraenhansen Apr 27, 2023

fealebenpae Apr 26, 2023

kraenhansen Apr 27, 2023 •

edited

Loading

RedBeard0531 Apr 27, 2023

kraenhansen Apr 27, 2023

RedBeard0531 commented Apr 26, 2023

kraenhansen commented Apr 26, 2023

RedBeard0531 commented Apr 26, 2023

RedBeard0531 commented Apr 27, 2023

kraenhansen Apr 27, 2023

RedBeard0531 Apr 27, 2023 •

edited

Loading

RedBeard0531 Apr 27, 2023

kraenhansen Apr 27, 2023

kneth left a comment

RedBeard0531 commented May 9, 2023

kraenhansen commented May 9, 2023

switch to consuming bindgen from realm-core repo #5769

switch to consuming bindgen from realm-core repo #5769

Conversation

RedBeard0531 commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 commented Apr 26, 2023

kraenhansen commented Apr 26, 2023

RedBeard0531 commented Apr 26, 2023

RedBeard0531 commented Apr 27, 2023

Choose a reason for hiding this comment

RedBeard0531 Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneth left a comment

Choose a reason for hiding this comment

RedBeard0531 commented May 9, 2023

kraenhansen commented May 9, 2023

kraenhansen Apr 27, 2023 •

edited

Loading

RedBeard0531 Apr 27, 2023 •

edited

Loading