-
Notifications
You must be signed in to change notification settings - Fork 47
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
Support for building Android platforms #90
base: master
Are you sure you want to change the base?
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.
Thanks for your work on this feature! Android support would be nice to have. I think there is now a conflict with existing code, could you resolve it so that we can have CI run on the changes? (there may be a race with #95 which is likely to land sooner, so perhaps wait that one out first... :-( )
Commit Support for building Android platforms
does a bunch of things and it's not really clear to me how it's semantic preserving with respect to the existing functionality for !Android. Can you split out refactorings and renames to make it more understandable what is going on? Also, generally commits should have a description unless it is a two line tops change and the summary line explains it sufficiently.
Please also merge in fix up commits and sort in random unrelated changes to the proper commit to keep history understandable (e.g., in Add android feature
).
95a22e1
to
0de097e
Compare
Sorry, but the third commit is unreviewable. There is no explanation why anything is being done. Is it so plainly obvious why we are suddenly changing out half the implementation? It certainly isn't to me. And again, this commit does a thousand things, all in one go, without any obvious moves or anything. Perhaps someone has the time to match up each removed line with each addition, I don't. Please split it to allow people to understand individual changes, not just replace one huge blob with another. |
The rest looks reasonable. |
This is my first time submitting code to the community. Thank you for your guidance. I have split it into multiple commits. Please review it again |
No worries. Didn't get a chance to take another look today, but should be able to review tomorrow. |
let vendored_libbpf = cfg!(feature = "vendored-libbpf"); | ||
let vendored_libelf = cfg!(feature = "vendored-libelf"); | ||
let vendored_zlib = cfg!(feature = "vendored-zlib"); | ||
let android = build_android(); |
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.
Perhaps drop a note to reference rust-lang/cargo#1197, which would allow us to enable different features for Android specifically.
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.
And thinking about this more...I think this will break the header embedding, among potentially others. So at the very least we need to adjust that as well.
A better solution may be something along the lines of what is suggested here in the last answer: https://stackoverflow.com/questions/57972355/enable-a-cargo-feature-by-default-when-the-target-arch-is-wasm
We don't care about dependency resolution because all dependencies are vendored in the repository, so I think this should work well (well, assuming it does, I haven't tried 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.
I think it is not necessary now. In my daily use, I always need these dependencies. zlib has static libraries in android ndk (maybe I need to add this?)
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.
Huh? But the issue is still present, right?
// We do support hidden visibility, so turn that on. | ||
"-DHAVE_HIDDEN", | ||
// We do support const, so turn that on. | ||
"-DZLIB_CONST", | ||
// Enable -O3 as per chromium. | ||
"-O3", | ||
// "-Wall", | ||
// "-Werror", | ||
// "-Wno-deprecated-non-prototype", | ||
// "-Wno-unused", | ||
// "-Wno-unused-parameter", |
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.
Where are these flags coming from, all of a sudden? And what does chromium have to do with this?
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.
In general, I kind of fail to understand what we gain from building our own build logic. The general guideline I am aware of is that the original build system should be used. If it provided a value, perhaps, but what value is that? Is the existing zlib
build system incapable, somehow? Perhaps if you finally wrote a proper commit description I wouldn't have to inquire about every single detail and it would even be available for future developers...
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.
FYI Android keeps forks of elfutils(https://android.googlesource.com/platform/external/elfutils) and zlib(https://android.googlesource.com/platform/external/zlib/+/refs/heads/main) that uses the soong blueprint + ninja build system.
Clearly some flags are copied from there: https://android.googlesource.com/platform/external/zlib/+/refs/heads/main/Android.bp#20 . (That Android.bp file doesn't have a license header.)
Some of the flags used in the soong blueprint are known to be supported by the compiler prebuilt that the AOSP project distributes, but might not be supported by all compilers. I don't think it is appropriate to set those flags for non Android targets.
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.
FYI Android keeps forks of elfutils(https://android.googlesource.com/platform/external/elfutils) and zlib(https://android.googlesource.com/platform/external/zlib/+/refs/heads/main) that uses the soong blueprint + ninja build system.
Clearly some flags are copied from there: https://android.googlesource.com/platform/external/zlib/+/refs/heads/main/Android.bp#20 . (That Android.bp file doesn't have a license header.)
Some of the flags used in the soong blueprint are known to be supported by the compiler prebuilt that the AOSP project distributes, but might not be supported by all compilers. I don't think it is appropriate to set those flags for non Android targets.
Yes, these flags are only used in Android
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.
The larger question seems unanswered.
In general, I kind of fail to understand what we gain from building our own build logic. The general guideline I am aware of is that the original build system should be used. If it provided a value, perhaps, but what value is that? Is the existing
zlib
build system incapable, somehow? Perhaps if you finally wrote a proper commit description I wouldn't have to inquire about every single detail and it would even be available for future developers...
Putting that aside, please remove unused flags as well as references to Chromium. Perhaps these make some kind of sense in an Android build system context, but they have no business here.
build.rs
Outdated
|
||
let prog = "./configure"; | ||
|
||
let _ = subproc("chmod", project, &["+x", prog]); |
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.
Isn't a configure
script already executable, at least for zlib
? Why would it ever be checked in but not executable?
If this is truly necessary then please use Rust API for changing permissions, instead of shelling out.
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.
Isn't a script already executable, at least for ? Why would it ever be checked in but not executable? If this is truly necessary then please use Rust API for changing permissions, instead of shelling out.
configure``zlib
Ok, I will use the api to achieve this
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.
Isn't a
configure
script already executable, at least forzlib
?
build.rs
Outdated
cflags.push_str(" -Wno-error=stringop-overflow"); | ||
cflags.push_str(&format!(" -I{}/zlib/", src_dir.display())); | ||
let host = if build_android() { | ||
format!("") |
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.
Why is there no target triple available?
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.
Why is there no target triple available?
When I compile for android locally, an error will be reported, so I leave it blank.
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.
That doesn't really answer or explain anything. aarch64-linux-android
is a valid triple for Android, among others. We should probably understand what the issue is instead of working around it from the get-go.
.env("BUILD_STATIC_ONLY", "y") | ||
.env("PREFIX", "/") | ||
.env("LIBDIR", "") | ||
.env("OBJDIR", &obj_dir) | ||
.env("DESTDIR", out_dir) |
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.
Are these all irrelevant or how are they picked up now?
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.
Are these all irrelevant or how are they picked up now?
Now, all the generated files will be in the project directory. I think these environment variables are not important, so I deleted them.
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.
Well, BUILD_STATIC_ONLY
sounds as if it should be set if we are building a static library, for one reason or another. Just removing it because you think it may not important isn't really going to cut it from my perspective, we are going to need a bit more than that.
263fc5e
to
3348d06
Compare
@@ -213,45 +214,73 @@ fn main() { | |||
} | |||
} | |||
|
|||
fn make_zlib(compiler: &cc::Tool, src_dir: &path::Path, out_dir: &path::Path) { | |||
let src_dir = src_dir.join("zlib"); | |||
fn make_zlib(compiler: &cc::Tool, src_dir: &path::Path, _: &path::Path) { |
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.
If you don't use a previously present argument, can you remove it?
@@ -151,10 +152,10 @@ fn main() { | |||
pkg_check("flex"); | |||
pkg_check("bison"); | |||
pkg_check("gawk"); | |||
pkg_check("aclocal"); |
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.
What needs aclocal
?
Android
platformlibbpf_set_print
instead of usingbindgen
to automatically generate it, to solve theva_list
errorbuild.rs
to use thecc
library instead of themake
command for building, and support building forAndroid
Android
testing inCI