android: Support custom build profiles#44182
Conversation
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
|
mach try android-production: https://github.com/jschwe/servo/actions/runs/24358852088/job/71132964376 |
| val fallback = basePath + "/target/" + getSubTargetDir(debug, arch) | ||
| // Prefer the value provided by `mach`, which supports custom cargo profiles. | ||
| // The fallback is for packaging in Android studio, after a mach build (only debug / release). | ||
| val target_dir = System.getenv("SERVO_TARGET_DIR") ?: fallback |
There was a problem hiding this comment.
Do we still need the SERVO_TARGET_DIR variable? Wouldn't it be equivalent to fallback and if not, can it be derived from the build type itself?
Also, I feel it is a bit "hacky" to have functions ignore the parameters and just use the environment variable. I wonder if we do this in a clean way, so that these functions can simply be responsible for constructing the path correctly and the caller at the top-level reads the environment variable, like we do for getSigningConfig.
There was a problem hiding this comment.
Do we still need the SERVO_TARGET_DIR variable?
Users can override the target directory (and for example I need to do that when using devcontainers on macos, since the workspace FS is mounted as case-insensitive, which breaks the build in mozjs, which lead me to move the target directory outside the workspace mount and into a dedicated volume, that is case-sensitive). I don't currently build android in the devcontainer, but I'm in the process of migrating more and more of my work into containers, since it significantly reduces the attack surface to my machine (since currently any rogue build.rs or proc macro could run arbitrary code).
I did a bit of a modification though, and got rid of the second environment variable, since we can construct that from SERVO_TARGET_DIR.
Also, I feel it is a bit "hacky" to have functions ignore the parameters
Oh, the entire packaging code here feels very hacky to me. Just take a look at this beauty:
servo/support/android/apk/servoview/build.gradle.kts
Lines 206 to 209 in cc6f7c5
What I want to unblock with this PR, is being able to build servoshell for android in production mode in CI for the release. That currently fails since the apk is not placed in the <profile_name> folder.
After cherry-picking the changes from this PR, CI passes: https://github.com/jschwe/servo/actions/runs/24516098869/job/71660434228
There was a problem hiding this comment.
Oh, the entire packaging code here feels very hacky to me. Just take a look at this beauty:
That is true, but it was also probably written back when servo just had support for debug and release profiles. We are bolting on custom profile support without accounting for all these places where the code assumes that only 'debug' and 'release' profiles are possible.
What I want to unblock with this PR, is being able to build servoshell for android in production mode in CI for the release.
Is it just to ensure consistency with other platforms, or we think we need the additional performance improvements on Android?
There was a problem hiding this comment.
Yeah, it would be really great if some contributor with android app experience could come along and clean up our android build code.
Is it just to ensure consistency with other platforms, or we think we need the additional performance improvements on Android?
One thing is the consistency, but the other (surprising) thing is that the production build apk is just 45MB, which is 1/3 of the release build!
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
mukilan
left a comment
There was a problem hiding this comment.
I'm fine with merging this PR as it is simpler now with a single environment variable rather than two where one can override the other.
| val fallback = basePath + "/target/" + getSubTargetDir(debug, arch) | ||
| // Prefer the value provided by `mach`, which supports custom cargo profiles. | ||
| // The fallback is for packaging in Android studio, after a mach build (only debug / release). | ||
| val target_dir = System.getenv("SERVO_TARGET_DIR") ?: fallback |
There was a problem hiding this comment.
Oh, the entire packaging code here feels very hacky to me. Just take a look at this beauty:
That is true, but it was also probably written back when servo just had support for debug and release profiles. We are bolting on custom profile support without accounting for all these places where the code assumes that only 'debug' and 'release' profiles are possible.
What I want to unblock with this PR, is being able to build servoshell for android in production mode in CI for the release.
Is it just to ensure consistency with other platforms, or we think we need the additional performance improvements on Android?
Since #44182 production builds for android are now possible in CI. Let's unify this, since all other platforms already use production. The production build is also ~45MB small, which is a huge reduction compared to the ~159MB our nightly builds have. I didn't investigate further why the difference is that large, but I installed the production version on my phone and didn't spot any obvious issues (on servo.org). [Mach try android-production](https://github.com/jschwe/servo/actions/runs/24525178123) Testing: We don't test CI workflows themselves. Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Pass the build directory to gradle via an environment variable, to support custom profiles.
Building the custom profiles already works, but follow-up commands expect the artifact at the location of the profile.
This allows switching the android release build to production (in a follow-up PR).
Testing: We don't run any runtime tests for android in CI