-
Notifications
You must be signed in to change notification settings - Fork 84
Added more build options for 3d party users #75
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
base: main
Are you sure you want to change the base?
Conversation
csmartdalton
left a comment
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 are awesome changes @JCash ! Thank you for taking this on!
| newoption({ | ||
| trigger = 'with-libs-only', | ||
| description = 'only builds the libraries', | ||
| }) |
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 don't think this should be necessary. If you want to build just a library, you can already say:
build_rive.sh -- rive_pls_renderer
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.
While I just verified that it is indeed possible to do, there are still major flaws with that approach:
- Documentation. There is no clear documentation as to which libraries are needed in order to link an executable.
- Robustness. There have been numerous updates (breaking changes) where the list (and names) of these libraries change.
So, in all fairness, I think the notion of a "build libraries only" target is still very much needed here.
| local host_arch = os.outputof('uname -m') or '' | ||
|
|
||
| -- Apply target triple if host arch doesn't match requested arch | ||
| if _OPTIONS['arch'] == 'x64' and host_arch ~= 'x86_64' then |
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.
Thanks for handling these. Can we add x86, just for completeness?
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'll try adding it.
build/rive_build_config.lua
Outdated
| end | ||
|
|
||
| -- Optional sysroot | ||
| if _OPTIONS['sysroot'] ~= nil then |
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 apply the sysroot outside of the linux branch? It might be confusing if someone tries to specify a sysroot for a non-linux build.
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.
Hmm, looking at my code, I don't think I even use it anymore. I'll verify removing it. 🤔
| -- Detect the NDK. | ||
| local EXPECTED_NDK_VERSION = 'r27c' | ||
| local NDK_LONG_VERSION_STRING = "27.2.12479018" | ||
| local NDK_LONG_VERSION_STRING = "25.2.9519653" |
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.
Thanks for calling attention to this! @ErikUggeldahl can probably suggest how we should handle it.
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.
Hmm, looks like this wasn't fully handled. Right now it's 27 to match with Rive Android's expected build. By changing this, we will likely clash. I could see adding a parameter to change it if necessary, but just changing it like this is likely to cause issues.
Additionally:
- This no longer matches the
EXPECTEDvariable above. - This version does not support 16KB page sizes by default. By dropping <26, you would need to ensure the advised build flags are enabled.
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.
Yeah, this entire PR is highlighting customer hot spots. So, what we likely need is a good way to specify the NDK/SDK version. I'm not sure what is the best way to do it, but basically something like this is a common approach:
export ANDROID_SDK_ROOT=/path/to/android/sdk
export ANDROID_NDK_ROOT=/path/to/android/ndk
export ANDROID_ABI=arm64-v8a
export ANDROID_PLATFORM=android-21
export ANDROID_TOOLCHAIN=clang
export ANDROID_STL=c++_shared
And then you will of course have your own defaults.
ErikUggeldahl
left a comment
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.
Just reviewing the NDK version change, but I think it needs some additional thought.
| -- Detect the NDK. | ||
| local EXPECTED_NDK_VERSION = 'r27c' | ||
| local NDK_LONG_VERSION_STRING = "27.2.12479018" | ||
| local NDK_LONG_VERSION_STRING = "25.2.9519653" |
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.
Hmm, looks like this wasn't fully handled. Right now it's 27 to match with Rive Android's expected build. By changing this, we will likely clash. I could see adding a parameter to change it if necessary, but just changing it like this is likely to cause issues.
Additionally:
- This no longer matches the
EXPECTEDvariable above. - This version does not support 16KB page sizes by default. By dropping <26, you would need to ensure the advised build flags are enabled.
build/rive_build_config.lua
Outdated
| end | ||
|
|
||
| -- Optional sysroot | ||
| if _OPTIONS['sysroot'] ~= nil then |
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.
Hmm, looking at my code, I don't think I even use it anymore. I'll verify removing it. 🤔
| local host_arch = os.outputof('uname -m') or '' | ||
|
|
||
| -- Apply target triple if host arch doesn't match requested arch | ||
| if _OPTIONS['arch'] == 'x64' and host_arch ~= 'x86_64' then |
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'll try adding it.
|
|
||
| newoption({ trigger = 'with-skia', description = 'use skia' }) | ||
| if _OPTIONS['with-skia'] then | ||
| if _OPTIONS['with-skia'] and not _OPTIONS['with-libs-only'] then |
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 approach may be obsolete with the workaround Chris suggested.
| newoption({ | ||
| trigger = 'with-libs-only', | ||
| description = 'only builds the libraries', | ||
| }) |
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.
While I just verified that it is indeed possible to do, there are still major flaws with that approach:
- Documentation. There is no clear documentation as to which libraries are needed in order to link an executable.
- Robustness. There have been numerous updates (breaking changes) where the list (and names) of these libraries change.
So, in all fairness, I think the notion of a "build libraries only" target is still very much needed here.
| -- Always build position independent code on Linux | ||
| -- Ensures static libs can link into shared objects without relocation errors. | ||
| filter({ 'system:linux' }) | ||
| do | ||
| pic('on') | ||
| buildoptions({ '-fPIC' }) | ||
| linkoptions({ '-fPIC' }) | ||
| end |
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 tried using the "--with-pic" option, but it seems it didn't apply to everything required 🤷
sysroot- for telling the build where the sysroot actually iswith_pthread- to be able to build WASM with pthread supportNDK_LONG_VERSION_STRING- highlight the need for configurable NDK valueswith-libs-only- to be able to just build the runtime libraries and skip everything else.