CI build for Android #3
Conversation
.github/workflows/build.yml
Outdated
| - name: install ndk | ||
| if: startsWith(matrix.config.compiler, 'ndk-') | ||
| run: echo "y" | sudo ${ANDROID_HOME}/cmdline-tools/latest/bin/sdkmanager --install `echo "${{ matrix.config.compiler }}" | tr - ";"` | ||
| - name: expand ANDROID_HOME env var in conan profile |
There was a problem hiding this comment.
Couldn't find a way to put $ANDROID_HOME in conan profile. Don't want to hardcode the path because it would be unusable on my development machine
There was a problem hiding this comment.
I think we had a similar problem in the android repo. Hardcoding would be last resort but I don't think it is terrible because it is an github specific profile. Locally we could emulate the github ci with https://github.com/nektos/act
There was a problem hiding this comment.
I should file an enhancement request to conan, because I don't believe we are the only ones who want to put $ANDROID_HOME env var in conan profile
|
Hey @andiwand , could you review this? Do we still support odrcore 1.0.0 and 2.0.0? Those two versions have build issues on Android. Also, currently we rebuild each package, each version on all platforms and all ABIs. This will get huge pretty fast once I start adding extra ports. The |
ViliusSutkus89
force-pushed
the
android-ci
branch
from
0c431bc to
b0580fc
Compare
|
@ViliusSutkus89 thanks! As you said, this can explode quite quickly, this is why initially only used a single compiler and a single OS. I am not sure if we expect that the build does not work on a different OS. That said the old versions seem not being able to build as you discovered but I think this is an upstream package issue? I don't think we should model an automatic version update chain if one of the upstream package version changes. I would rather do this manually by bumping the version of the dependency in the conanfile directly which should then trigger a correct rebuild. |
.github/workflows/build.yml
Outdated
| - { os: ubuntu-22.04, compiler: clang-15 } | ||
| - { os: ubuntu-22.04, compiler: clang-15, host-profile: host } | ||
| - { os: ubuntu-22.04, compiler: ndk-26.3.11579264, host-profile: armv8 } | ||
| - { os: ubuntu-22.04, compiler: ndk-26.3.11579264, host-profile: x86_64 } | ||
| - { os: ubuntu-22.04, compiler: ndk-26.3.11579264, host-profile: armv7 } | ||
| - { os: ubuntu-22.04, compiler: ndk-26.3.11579264, host-profile: x86 } |
There was a problem hiding this comment.
I am not sure yet if Android makes sense here but I would say at maximum one? Because otherwise we launch a very big amount of jobs.
I think compiler should stick with the clang rather than ndk-26.3.11579264. I guess we could have build OS and host OS to make this a bit cleaner?
There was a problem hiding this comment.
Well it depends on how we want to use this odr-index repo.
Small matrix here means faster builds here and our artifactory is more like a recipe index, but this means slow builds for downstream users, because most of the time, downstream users would have to build these conan packages from source.
Big matrix here means slower builds here, but fast builds for downstream users, because downstream users would be able to find binary packages that were built here. Mind you, downstream users (odr.droid) need binaries from all 4 Android ABIs.
How is artifactory charging us for the repository? Does the amount of binaries that we upload have an actual cost? Or is it in gigabytes and we don't actually have to care? Big matrix here does not have that much of an impact on GitHub side, the repo is open source, so we don't have to worry about GitHub charges, only the run time, but if all the dependencies are already available as binaries, each of them wouldn't take that long, it's not in the hours as it was when I was building pdf2htmlEX dependencies from source each time :D
As for the compiler, clang and ndk-26.3.11579264, I don't really care about the actual naming, what I need is a way to specify build and host profiles. And I also need some kind of a variable to specify NDK version, because NDK needs to be installed manually
.github/workflows/build.yml
Outdated
| - name: install ndk | ||
| if: startsWith(matrix.config.compiler, 'ndk-') | ||
| run: echo "y" | sudo ${ANDROID_HOME}/cmdline-tools/latest/bin/sdkmanager --install `echo "${{ matrix.config.compiler }}" | tr - ";"` | ||
| - name: expand ANDROID_HOME env var in conan profile |
There was a problem hiding this comment.
I think we had a similar problem in the android repo. Hardcoding would be last resort but I don't think it is terrible because it is an github specific profile. Locally we could emulate the github ci with https://github.com/nektos/act
|
@ViliusSutkus89 for Initially I also thought that this repo could build for our common target platforms but I am not sure about this anymore. Right now I distribute only the recipe and the ubuntu binaries which will not be too useful. But on the platform builds I upload the binaries so we should get sort of both - keep the matrix small here and have accelerated builds for platform. About the artifactory: we currently host it on a VM which should have enough resources to hold all the binaries we need so we do not need to worry about it. |
|
Could you explain what do you mean by "common target platforms" and "platform builds"? odr.core is the platform, right? All the other packages that I want to dump here would eventually end up as dependencies of odr.core (pdf2htmlEX and others), so they are kinda platform too |
|
btw, conan is actually cool about dependency versioning. In ndkports whenever I had to update a deep dependency, I had to do commits and change version numbers in downstream users, mind you, there were like 5 layers between pdf2htmlEX and the deepest dependency. If I changed anything in my packaging scripts, a version increment was needed and a whole downstream version change commit and rebuild rigamarole. With conan we can change recipes but keep the same version numbers, because here version numbers represent library version, not our package version. And when there's a new version of a deep dependency, we can just [replace_requires] in the conan profile, that's extra cool. |
|
Sorry, I didn't do a great job explaining what I meant. With With Btw here is the upload mechanism for the Android CI build https://github.com/opendocument-app/OpenDocument.droid/blob/ef42f76f2822222cabf4502af01e514a26b06113/.github/workflows/android_main.yml#L63-L64 |
|
Working Ubuntu build is nice to keep in case there's a problem with program output, so we could easily compare it between different platforms. As for the upload mechanism. I see what you mean. Uploading binaries from odr.droid has the upside that we don't have to match NDK versions, Android API levels and profile configs between odr.droid and here. The downside is that we have two places with write access to the artifactory. I assume there's no need to |
|
Sorry for chiming into this discussion so late. I have to ask the obvious question: what's the goal of this PR exactly? Adding more binaries in order to speed up builds for other users? I think that's not worth it considering that 1. it adds code complexity and 2. it consumes GitHub action minutes (which we get 2k for free per month). If the goal of this PR is somehow related to getting pdf2htmlEX into the core I'm all up for it though! 🎉 @ViliusSutkus89 |
|
Hello, 2k minutes is news to me. I know about that limit, but I was under the impression that open source projects don't count towards that limit. I've checked my billings summary and I don't see anything counted. It was staying at 0 even when I was doing ndkports. harfbuzz library alone is like half an hour, but that's just for one NDK version and library type (shared/static). Full build run was at least 2 hours. Just for harfbuzz. And I used to run it multiple times, just to be sure it compiled right. Emulator instrumented tests are also in the hourly range. pdf2htmlEX-Android has instrumented tests workflow, which runs tests on 24 different emulators. Those emulators take forever to boot up. And they are extra wonky, sometimes they glitch and stay up for hours. Could you check the usage counter on the organisation account? We need to have at least one Android build to have an actual CI, just to be sure that the dependencies are building properly for Android. I don't mind keeping a fork of this repo and running a proper build matrix there. I may eventually look into running tests on emulator for these packages. That would turn up the minutes counter real fast. |
You're right, free for public repositories: https://github.com/pricing
I kind of like that argument, but I wonder if there's a way we can achieve this without adding a lot of code complexity / duplication? I guess we could trigger a build with the new core version on https://github.com/opendocument-app/OpenDocument.droid as an alternative? Not sure if that fits my "no code complexity" argument though 😄 |
|
To me it would be good enough to test the recipe locally against Android and then just check it in this repo testing against Ubuntu. We can think about also building against Android but I would not use the full build matrix. The reason being that this repo is rather meant to bootstrap recipes and upload them into our artifactory. The build cash will be filled from the platform builds. In principle we can also do all the platform builds here but in that case I would like to rethink a bit how we can scale this best. Opinions @TomTasche @ViliusSutkus89 ? |
|
The pricing definition is very ambiguous. Definitely written by a marketer. Instrumented tests for these packages would be a life saver. In ndkports I've had a crash in GLib ( ViliusSutkus89/ndkports#20 ), but that crash had different affected APIs for different ABIs. Of course my real devices were unaffected, why would they be affected? I only found out about it during pdf2htmlEX testing, and only because I had a big test matrix. I would prefer to see these crashes during conan test phase, while building the offending package, and not somewhere down the line in odr.droid, because the later down the line we see the issue, the harder it is to track down the changes which caused it. Of course, instrumented tests during the build would be way more complex than the current setup of just building it, but just building it on each ABI gives a bit more confidence that the library will at least link properly. What could be the actual scaling issue that we could face? Due to size constraints, ccache would be unusable, but that's pretty much it. Oh, and a lot of failed build spam in the inbox. That's a big one. |
|
Testing is a very good point. Would this be the pdf2htmlex tests or some of our own? I think the conan tests are meant for small scale checks of the binaries actually do something. In that case it would definitely be beneficial to have it after we built for each platform. But that would have to run in an emulator right? So that might actually be out of scope. On the other hand we can run these tests on the platform after integration and catch the problem there. That might be a bit easier? |
|
Conan tests usually is a minimal program that uses the freshly built library. This way you test if the library can be linked against and at least loads properly. Although currently conan doesn't run the minimal program if it's cross compiled. So currently we only get the benefit of a proper linking check. I don't think that setting up the emulator would be that much of an effort. self.run would be replaced by a few lines of adb push and adb shell execute and that's pretty much it
Ideally I would want to run whatever the test suite that the package provides, not just linking, but that may not be easy to set up for every package. If the package uses gtest, than it's just a matter of pushing the compiled binary, same as with the linking test. But some packages have other test frameworks that may or may not run on Android. Running that test suite is important, even if it's not our package, because for some packages, we are the only ones who actually uses them on Android. Before our work, GLib wouldn't even build for Android. That crash also means that other users are also either stuck on that particular version or they have unmainstreamed patches. Or there is no other users and we may just be the only ones using that package. Running upstream tests would uncover a lot of that. |
|
There's additional problem with just running the build locally. I keep contaminating my local conan cache, so some dependencies sometimes compile, sometimes not, and I keep clearing the local |
|
Ok but that can also be solved with a script right? And while developing / debugging the recipe I would avoid the CI anyways because of the delay |
|
Ideally I wouldn't be cleaning the cache locally that often, because a full rebuild takes a lot of time. Personally I trust CI build more than I trust my local build. I understand the delay, but it gives me peace of mind that it builds properly on all defined ABIs, all defined min versions and et cetera. Anyways, CI vs local build is personal preference. No point in us discussing that. I've added a separate deep test workflow, but you won't have to deal with it. It's disabled in GitHub website and also if-guarded using As you can see, I've added some extra Android conan profiles. As of right now, odr.droid requires Android-23, but it may still be possible to ship pdf2htmlEX-Android for Android-21, once I sort out OpenLibm build. If I do some testing with previously deprecated NDKs, I might be able to ship pdf2htmlEX-Android even for older devices. Multiple min SDKs was a pain in ndkports, but it's pretty easy with Conan. But to do that I need testing pipelines on all different min API levels. Like I've mentioned previously, those pipelines are running only on my fork, so you don't need to worry about them. Publishing them to artifactory would be extra cool, because it would drastically reduce my CI times. But we need to agree if that's ok, because there will be bunch of different configurations that I would publish and all that would take up some space. |
There was a problem hiding this comment.
Looks much cleaner already!
One general question: Why do we need to build against two android versions?
Potentially we could generate the conan profiles with a script and enumerate the cases there and make it configurable in the workflow via CLI args. But that can be done in another PR.
Left a few other comments
.github/workflows/deep-test.yml
Outdated
| jobs: | ||
| find-all-packages: | ||
| # Deep testing limited to personal fork | ||
| if: ${{ github.repository_owner == 'ViliusSutkus89' }} |
There was a problem hiding this comment.
I don't think we have to do this otherwise you will always have to keep your repo up to date for testing if somebody else changes something.
There was a problem hiding this comment.
We don't have to keep this. It's here to prevent a lot of extra runs on the organization's GitHub profile
| arch=x86_64 | ||
| build_type=Release | ||
| build_type=RelWithDebInfo |
There was a problem hiding this comment.
What is the reason for changing this?
There was a problem hiding this comment.
Can you explain this?
"-s", "build_type=Release",
"-s", "&:build_type=RelWithDebInfo",
"-s", "odrcore/*:build_type=RelWithDebInfo",
I assume this means a request to build everything as Release, but only odr.core as RelWithDebInfo ? I assume that if we build it like that, there will be less debug info in dependencies. Most of pdf crashes happens not in odr.core, but somewhere deep in libpng, called by either fontforge or poppler, because pdf2htmlEX supplied the wrong argument somewhere. I assume Release instead of RelWithDebInfo for deep dependencies would give less info during crashes.
There was a problem hiding this comment.
Yes my intention here was to only have debug symbols for odr.core and rely on other packages to be stable. That paradigm might shift with pdf2htmlEX.
I think I was also facing some multi config issues with Android.
Ultimately this should not necessarily drive our decision here. I think Release and RelWithDebInfo is fine while Debug might be a bit too big.
There was a problem hiding this comment.
Debug is completely unoptimized, let's leave RelWithDebInfo. We could also test if RelWithDebInfo actually produces binaries that are slower or larger than Release
There was a problem hiding this comment.
Shouldn't the android config tools.android:ndk_path also set the build environment via the toolchain? Why do we need the build tools set manually?
There was a problem hiding this comment.
We need to keep default/ubuntu profile as the build profile, it's not the host profile. We could get rid of default and just keep ubuntu and then explicitly specify it as --profile:build ubuntu near --profile:host ${{ matrix.config.host_profile }} when calling conan
.github/workflows/deep-test.yml
Outdated
There was a problem hiding this comment.
This looks mostly duplicated to the other workflow. Can we merge this somehow?
There was a problem hiding this comment.
Yes, it's the same pipeline, but with much larger matrix. I'll merge it back. I'll also change the trigger to on: push instead of filtering by branch
|
You want to build everything from scratch if we trigger the CI here right? The argument being that we want to build all packages with their dependencies in case something broke down the line. What I do not see in the CI is that we respect the dependency tree but build in the order given by the enumeration script. Doesn't this result in building some new packages against some old packages? And depending on the position in the list this might even vary over time. |
|
I need two Android versions - 23 and 21, because pdf2htmlEX-Android Java library is still minsdk 21, while odr.droid is minsdk 23. Fontforge has some complex math code, Android's libm supports complex math only from api 23 (complex.h)[https://android.googlesource.com/platform/bionic/+/pie-dev/libc/include/complex.h]. To get the same complex math functionality I've used Julia's OpenLibm, but only when compiling for pre-23. Long story short - 23 and 21 have different dependency trees. Min sdk 19 and 16 may require even more different libraries. Haven't looked back to those versions yet. As you already have noticed, conan profiles have a lot of build env vars, which are supposed to be set by I know that we can supply arch and api level as parameters to Conan, but those arguments don't reach buildenv. See, Conan profiles are interpreted twice, once by jinja2 template engine, and then by Conan. The In pdf2htmlEX_and_deps branch I have a export_all_packages script, which exports all packages from the recipes folder to the local cache. Once all packages are in the local cache, build order doesn't matter for correct dependencies. The only downside is that a change in for example pdf2htmlEX would trigger a rebuild of both pdf2htmlEX and odr.core and if they run at the same time, both would have to build pdf2htmlEX from source, because the pdf2htmlEX binary is not in artifactory yet. I'll pull that script to this branch too. As for the goals of different pipelines - I'll merge deep-test into build workflow and then later I'll add the emulator pipeline. I need to experiment with conan graph a bit to see if it's easy to calculate which packages are affected by the commits and only rebuild those. Once that's done, the ideal CI pipeline would be two stage:
Emulators got changed recently, we don't need to run Macos anymore, they run on Ubuntu. Check this out - https://github.com/ViliusSutkus89/conan-odr-index/actions/runs/9627754472/job/26574964995 . Conan install is 15 minutes, actual emulator is just 2 minutes. Most of those 15 conan install minutes will be skipped, because build stage will provide the needed binary artifact. That linked Actions run is actually running pdf2htmlEX's link_test on the emulator - test_package/conanfile . No conversion is being done, but |
…h requirements.txt
… been wrong about ccache
|
Updated the PR to merge deep-test into regular build workflow, added the recipe exporter script and modified workflow trigger to run on any push, not just main branch |
…d from somewhere else than the root repository dir
|
Sounds good - Thanks for the detailed explanations @ViliusSutkus89 ! I think one open question is still if we need to model the dependencies also in the build workflow here. Since you want to pick up on any changes upstream we would have to have some defined build order to make this happen. Also, do you think we could have some comments on the profile for the env and pointing to the issues? I think that will make it easier in the future to remove them as soon as they are not needed anymore. Otherwise great work! I will go through one more time and wait for your answers and then this should be ready to be merged. |
| {% set cc = { | ||
| "armv7": "armv7a-linux-androideabi" + api_level + "-clang", | ||
| "armv8": "aarch64-linux-android" + api_level + "-clang", | ||
| "x86": "i686-linux-android" + api_level + "-clang", | ||
| "x86_64": "x86_64-linux-android" + api_level + "-clang", | ||
| }[arch] %} |
There was a problem hiding this comment.
nitpick: can this be moved up to the other set section?
similar below
| arch=x86_64 | ||
| build_type=Release | ||
| build_type=RelWithDebInfo |
There was a problem hiding this comment.
Yes my intention here was to only have debug symbols for odr.core and rely on other packages to be stable. That paradigm might shift with pdf2htmlEX.
I think I was also facing some multi config issues with Android.
Ultimately this should not necessarily drive our decision here. I think Release and RelWithDebInfo is fine while Debug might be a bit too big.
There was a problem hiding this comment.
Ah I see makes sense! I might have picked bash for that but it does not matter.
So the idea is to export all recipes and let the package pick up the dependencies. This might result in building packages multiple times but saves us from forcing an order on the build.
There was a problem hiding this comment.
I thought about doing something like find ./recipes -name 'conanfile.py' | grep --invert-match test_package | xargs ... conan export {}, but I think that we need to export every version of each conanfile, not just the conanfile itself. Iterating over versions in bash is too cumbersome
There was a problem hiding this comment.
I thought list_package_versions.py might fit in here but I guess it would need some more tweaking. Anyways since the current approach does the job no need to refactor this
|
I'm nearly there with the conan dependency graph parsing, so if you can wait a couple of days, there will be another monstrosity to review :D |
|
Do you think we should get this one in first? |
|
Sure, we can do incremental updates, will be easier to merge
|
No description provided.