-
Notifications
You must be signed in to change notification settings - Fork 168
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
Fix tools build #6475
Fix tools build #6475
Conversation
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.
LGTM once CI is passing 👍
src/realm/exec/CMakeLists.txt
Outdated
add_custom_target(build_all DEPENDS | ||
RealmImporter RealmTrawler RealmEnumerate RealmDecrypt | ||
RealmEncrypt RealmBrowser RealmDump) | ||
add_dependencies(build_all ${ExecTargetsToInstall}) |
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.
CMake Error at src/realm/exec/CMakeLists.txt:75 (add_dependencies):
add_dependencies called with incorrect number of arguments
I think you might want to disable building these tools on Android
would the change log entry be needed for such internal thing? |
I think excluding these tools from the default build was intentional to save compile time. (the downside as you discovered is that they can easily bitrot). Instead of making them part of the default build, how about explicitly building them on one select CI machine?
Sure, you can add a note in the internals section of the change log. |
87bcddc
to
5e83928
Compare
aac0ee0
to
733c298
Compare
4f4a6a0
to
5a74adb
Compare
5a74adb
to
74ce044
Compare
I've excluded a few configurations, but it's less than one second on a laptop, practically it saves not much, i think it should be fine. |
What, How & Why?
☑️ ToDos
[ ] 🚦 Tests (or not relevant)[ ] C-API, if public C++ API changed.