Support for building Android platforms #90

editso · 2024-06-28T11:23:19Z

Add header files required for the Android platform
Manually implement libbpf_set_print instead of using bindgen to automatically generate it, to solve the va_list error
Modify build.rs to use the cc library instead of the make command for building, and support building for Android
Added Android testing in CI

danielocfb

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).

build.rs

Cargo.toml

danielocfb · 2024-08-19T20:23:50Z

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.

danielocfb · 2024-08-19T20:23:59Z

The rest looks reasonable.

…blem of platform 'va_list'

editso · 2024-08-20T04:08:31Z

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.

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

danielocfb · 2024-08-20T23:39:17Z

No worries. Didn't get a chance to take another look today, but should be able to review tomorrow.

danielocfb · 2024-08-21T16:44:34Z

build.rs

-    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();


Perhaps drop a note to reference rust-lang/cargo#1197, which would allow us to enable different features for Android specifically.

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).

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?)

Huh? But the issue is still present, right?

build.rs

danielocfb · 2024-08-21T17:02:28Z

build.rs

+        // 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",


Where are these flags coming from, all of a sudden? And what does chromium have to do with this?

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 going against this recommendation is fine, 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...

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.

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

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.

danielocfb · 2024-08-21T17:09:01Z

build.rs

+
+    let prog = "./configure";
+
+    let _ = subproc("chmod", project, &["+x", prog]);


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.

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

Isn't a configure script already executable, at least for zlib?

danielocfb · 2024-08-21T17:18:14Z

build.rs

-    cflags.push_str(" -Wno-error=stringop-overflow");
-    cflags.push_str(&format!(" -I{}/zlib/", src_dir.display()));
+    let host = if build_android() {
+        format!("")


Why is there no target triple available?

Why is there no target triple available?

When I compile for android locally, an error will be reported, so I leave it blank.

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.

danielocfb · 2024-08-21T17:25:14Z

build.rs

-        .env("BUILD_STATIC_ONLY", "y")
-        .env("PREFIX", "/")
-        .env("LIBDIR", "")
-        .env("OBJDIR", &obj_dir)
-        .env("DESTDIR", out_dir)


Are these all irrelevant or how are they picked up now?

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.

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 be important isn't really going to cut it from my perspective, we are going to need a bit more than that.

danielocfb · 2024-08-26T22:37:29Z

build.rs

@@ -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) {


If you don't use a previously present argument, can you remove it?

danielocfb · 2024-08-26T22:44:35Z

build.rs

@@ -151,10 +152,10 @@ fn main() {
        pkg_check("flex");
        pkg_check("bison");
        pkg_check("gawk");
+        pkg_check("aclocal");


What needs aclocal?

jfernandez · 2024-09-21T12:35:19Z

@editso, thank you for working on adding Android support. I found this issue because a user asked for Android support in bpftop Netflix/bpftop#91

Do you intend to continue working on the PR?

editso · 2024-09-23T16:27:29Z

@editso, thank you for working on adding Android support. I found this issue because a user asked for Android support in bpftop Netflix/bpftop#91

Do you intend to continue working on the PR?

Yes, I have had other things to do recently, so it has been delayed

danielocfb requested changes Aug 16, 2024

View reviewed changes

build.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

editso force-pushed the android branch 2 times, most recently from 95a22e1 to 0de097e Compare August 18, 2024 03:54

editso added 3 commits August 20, 2024 10:33

feat: Complete Android platform header files

2a5e9fd

feat: Manually implementing 'libbpf_set_print' to solve the error pro…

6299c59

…blem of platform 'va_list'

feat: Build support for Android

ed8b221

editso force-pushed the android branch from 0de097e to 128a62a Compare August 20, 2024 04:00

danielocfb self-requested a review August 20, 2024 16:28

danielocfb reviewed Aug 21, 2024

View reviewed changes

Re-implement the build of zlib

4a5c3bf

editso force-pushed the android branch 2 times, most recently from 263fc5e to 3348d06 Compare August 25, 2024 06:13

editso added 3 commits August 25, 2024 14:16

Re-implement the build of libelf

32f5ad8

Re-implement the build of libbpf

03ec160

feat: Add Android test job

3b80f23

editso force-pushed the android branch from 3348d06 to 3b80f23 Compare August 25, 2024 06:17

danielocfb reviewed Aug 26, 2024

View reviewed changes

jfernandez mentioned this pull request Sep 21, 2024

Android support Netflix/bpftop#91

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for building Android platforms #90

Support for building Android platforms #90

editso commented Jun 28, 2024 •

edited

Loading

danielocfb left a comment

danielocfb commented Aug 19, 2024

danielocfb commented Aug 19, 2024

editso commented Aug 20, 2024

danielocfb commented Aug 20, 2024

danielocfb Aug 21, 2024

danielocfb Aug 21, 2024

editso Aug 24, 2024

danielocfb Aug 26, 2024

danielocfb Aug 21, 2024

danielocfb Aug 21, 2024 •

edited

Loading

kxxt Aug 24, 2024

editso Aug 24, 2024

danielocfb Aug 26, 2024 •

edited

Loading

danielocfb Aug 21, 2024

editso Aug 24, 2024

danielocfb Aug 26, 2024

danielocfb Aug 21, 2024

editso Aug 24, 2024

danielocfb Aug 26, 2024

danielocfb Aug 21, 2024

editso Aug 24, 2024

danielocfb Aug 26, 2024 •

edited

Loading

danielocfb Aug 26, 2024

danielocfb Aug 26, 2024

jfernandez commented Sep 21, 2024

editso commented Sep 23, 2024


		let prog = "./configure";

		let _ = subproc("chmod", project, &["+x", prog]);

Support for building Android platforms #90

Are you sure you want to change the base?

Support for building Android platforms #90

Conversation

editso commented Jun 28, 2024 • edited Loading

danielocfb left a comment

Choose a reason for hiding this comment

danielocfb commented Aug 19, 2024

danielocfb commented Aug 19, 2024

editso commented Aug 20, 2024

danielocfb commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielocfb Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielocfb Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielocfb Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfernandez commented Sep 21, 2024

editso commented Sep 23, 2024

editso commented Jun 28, 2024 •

edited

Loading

danielocfb Aug 21, 2024 •

edited

Loading

danielocfb Aug 26, 2024 •

edited

Loading

danielocfb Aug 26, 2024 •

edited

Loading