Allow nix to compile on android #631

roblabla · 2017-06-27T21:46:24Z

Fixes #313

Susurrus · 2017-06-28T15:42:33Z

src/sys/signal.rs

@@ -210,6 +211,19 @@ libc_bitflags!{
    }
 }

+#[cfg(target_os = "android")]


These need doc comments. Also, if you search the libc crate you'll see these are available on Linux and all BSD platforms. We shouldn't artificially restrict them to just android here.

They are still available on other platforms, just defined as libc::c_int above ? CF line 201

Maybe I should use cfg_if! to make it clearer.

It's worth noting that this change is necessary because of rust-lang/libc#511

They're defined as c_int for all platforms.

Since you mentioned the upstream issue, please also add a FIXME that links to the upstream issue and states what can be done when it's resolved. We need to track these tasks in the code, not just here in the PRs.

roblabla · 2017-06-28T15:42:44Z

So there's another symbol that's apparently missing from android : __errno_location. Instead, it stores it in tread local storage, and has a function to retrieve a pointer to it called __errno : https://github.com/LineageOS/android_bionic/blob/cm-14.1/libc/bionic/__errno.cpp

errno is simply defined as dereferencing the symbol there : https://github.com/LineageOS/android_bionic/blob/cm-14.1/libc/include/errno.h#L44

When we try to compile something for android, we get a linker error saying that __errno_location is undefined.

Now, because the bionic __errno is defined as volatile in bionic, I don't know if it's a good idea to simply make __errno_location return it ? I'm pretty illiterate when it comes to what volatile does.

roblabla · 2017-06-28T17:04:59Z

More problems, this time for the target aarch64-linux-android. For some reason, libc::SO_PASSCRED, libc::SO_PEERCRED, libc::SO_SNDBUFFORCE are not defined on android in libc (in addition to being undefined on linux arm). I believe this is a bug, so I'm pushing it upstream. In the meantime, I'll also #cfg them out.

https://github.com/nix-rust/nix/blob/master/src/sys/socket/consts.rs#L43 is marked as not(target_arch="arm").

roblabla · 2017-06-28T17:36:42Z

It should now be possible to compile binaries using nix for both arm-linux-androideabi and aarch64-linux-android.

Should I document every #[cfg(not(target_os = "android"))] ? I'm not too sure what I'm supposed to put in there :/.

Susurrus · 2017-06-29T04:29:11Z

@roblabla There are basically two categories of things here you're asking about, one is things that currently fail when testing on Android that shouldn't and things that shouldn't be tested on Android. I'd prefer to wait until upstream is fixed so that we only have cases of the latter. Those don't need to be documented. If we have things that can't currently be tested because they're blocking on upstream, those need to be documented. But like I said, I'd rather wait until upstream is fixed to merge this PR so that we only have "good" fixes here.

roblabla · 2017-06-29T18:34:45Z

The only change that'll ever be fixed by libc is the use of 'as' in the bitflags though, along with missing symbols. I'll try digging what symbols are missing in libc compared to the most recent version of android, so we have a clearer picture of the situation.

…

On June 29, 2017 6:29:12 AM GMT+02:00, Susurrus ***@***.***> wrote: @roblabla There are basically two categories of things here you're asking about, one is things that currently fail when testing on Android that shouldn't and things that shouldn't be tested on Android. I'd prefer to wait until upstream is fixed so that we only have cases of the latter. Those don't need to be documented. If we have things that can't currently be tested because they're blocking on upstream, those need to be documented. But like I said, I'd rather wait until upstream is fixed to merge this PR so that we only have "good" fixes here. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #631 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

Susurrus · 2017-06-29T18:38:40Z

Yes, well libc isn't going to add symbols that don't exist on Android. For things that don't exist on Android, cfg-ing them out of tests and builds is correct and doesn't need explicit documentation.

roblabla · 2017-06-29T19:28:45Z

Linking rust-lang/libc#632 since it seems to contain some items of interest.

Susurrus · 2017-07-03T00:01:00Z

@roblabla We now have all android targets being built in CI, so if you rebase this you can see how well things will compile on those targets. Probably not worth doing so until rust-lang/libc#632 settles down, however.

ndusart · 2017-07-03T17:40:05Z

@roblabla rust-lang/libc#632 is fixed, we could remove some of the #[cfg()] in the first commit now :)

roblabla · 2017-07-03T17:41:35Z

With rust-lang/libc#634 now merged, most of the constants are now defined. The only ones left are :

SO_PASSCRED
SO_PEERCRED
SO_SNDBUFFORCE

They should be defined though, as a quick grep -R reveals they are in the unified headers, under asm-generic. Time for a second libc PR I guess

Susurrus · 2017-07-03T17:44:25Z

@roblabla I added those constants in rust-lang/libc#639 already as I saw they were needed for Android and I was already adding socket constants as part of #636.

Susurrus · 2017-07-03T17:44:43Z

So give this thing a rebase and remove some of those cfgs and we'll see what happens!

roblabla · 2017-07-03T20:39:21Z

rust-lang/libc#642 is necessary now

Susurrus · 2017-07-03T22:55:21Z

I'd also like to move Android builds to Tier 2 as part of this. Could you modify the README and also .travis.yml to do that as well?

roblabla · 2017-07-03T23:09:32Z

Should be fine now

Susurrus · 2017-07-03T23:50:10Z

Could you better document your changes to the libc_bitflags macro? munch an ident isn't very helpful to someone who doesn't use the macro system in Rust. It'd be better if you described exactly what it allowed, especially if you added an example use-case. The module comment at the top could use some additions for the two use cases you added as well.

I'd also prefer your changes to macros.rs be removed into a separate commit before the others if you're comfortable with git rebase, otherwise it's not a big deal.

roblabla · 2017-07-04T12:18:28Z

Is this better ? I basically only added one use-case, just split accross 2 arms to support the trailing comma on the last ident.

roblabla · 2017-07-04T12:35:18Z

Right, so this change caused the build to fail on arm(v7)-unknown-linux-gnueabi because libc doesn't define those flags on arm. rust-lang/libc#644 will fix this failure.

For the i686 and x86_64 android, there are a bunch of type failures. Which is surprising because there isn't much in the arch-specific files of libc (https://github.com/rust-lang/libc/blob/master/src/unix/notbsd/android/b32/arm.rs). I'll investigate.

ndusart · 2017-07-04T14:51:26Z

The failures on Android with Intel-cpu are due because the test suite is using rustc 1.13 which has no support at all for x86_64-linux-android and for which std was broken for i686-linux-android (see fix), this fix was introduced in version 1.18 of rust.

So the explanation was that std::os::raw::c_char returned from CStr::as_ptr is different than libc::c_char for older stdlib.

I think x86_64-linux-android is not considered stable yet, libc still use the beta channel for testing it.
We should consider using at least rustc 1.18 for testing i686-linux-android, and at least stable for x86_64-linux-android.

I could also advise you to test (at least the build phase) locally on your machine with the latest rustc for the different targets. You won't have to commit just to test and you'd have seen that difference.

roblabla · 2017-07-04T15:23:48Z

Ah I see. So I bumped the minimum version to 1.18 for i686/x86_64. I also updated the libc PR to also define MAP_32BIT on x86/x86_64 android targets.

I think this should be everything necessary.

Susurrus · 2017-07-04T17:31:02Z

@roblabla Love the separate macro commit, thanks for that. That let me cherry-pick it into #527, as I needed that functionality as well.

This is pretty close to being mergeable:

The requirement for Rust 1.18 for the x86 Android targets needs to be documented. I suggest adding it to the README like (requires Rust >= 1.18) to the end of the lines stating they have Tier 2 support.
We need some CHANGELOG entries. Under Changed add that Android is now a Tier 2 target and under Fixed bind and errno_location now work correctly on Android.

Susurrus · 2017-07-04T17:32:51Z

Please add those CHANGELOG entries in with the corresponding commits.

If you're feeling particularly diligent, you could also reorder the 2 last commits. I like to have commits ordered such that building things will always succeed. This means fixing the errors and then requiring builds to pass for the fixed target.

roblabla · 2017-07-04T17:33:58Z

@Susurrus actually, the commits are in the right order. Github interface is just confused, probably because of all the rebase and push --force ? https://github.com/roblabla/nix/commits/feature-androidWorks

This is necessary because certain flags in libc have different types, generally due to a mistake when adding the flags to the libc. See rust-lang/libc#511 for an example of such a discrepency.

roblabla · 2017-07-04T17:44:11Z

Done.

Susurrus · 2017-07-04T18:39:20Z

LGTM, thanks for all your hard work!

bors r+

631: Allow nix to compile on android r=Susurrus Fixes #313

bors · 2017-07-04T19:22:52Z

Build succeeded

roblabla force-pushed the feature-androidWorks branch 4 times, most recently from f82dbf1 to 3513dad Compare June 28, 2017 12:36

Susurrus reviewed Jun 28, 2017

View reviewed changes

roblabla force-pushed the feature-androidWorks branch from 7ac2dc7 to 7c609de Compare June 28, 2017 17:21

Susurrus mentioned this pull request Jul 2, 2017

Add all Android targets as Tier 3 platforms #639

Merged

roblabla force-pushed the feature-androidWorks branch 2 times, most recently from c890644 to b63494d Compare July 3, 2017 20:38

roblabla force-pushed the feature-androidWorks branch from b63494d to 9936533 Compare July 3, 2017 22:41

roblabla force-pushed the feature-androidWorks branch from ff15228 to f75f03b Compare July 3, 2017 23:09

roblabla force-pushed the feature-androidWorks branch from f75f03b to eceada4 Compare July 4, 2017 12:15

roblabla force-pushed the feature-androidWorks branch from eceada4 to 51aae0f Compare July 4, 2017 15:19

roblabla force-pushed the feature-androidWorks branch 2 times, most recently from 51aae0f to 55bec60 Compare July 4, 2017 16:25

roblabla force-pushed the feature-androidWorks branch 2 times, most recently from f8a6951 to 552fccf Compare July 4, 2017 17:42

roblabla added 2 commits July 4, 2017 19:43

Allow casting a flag to a type in libc_bitflags!

26a0a48

This is necessary because certain flags in libc have different types, generally due to a mistake when adding the flags to the libc. See rust-lang/libc#511 for an example of such a discrepency.

Allow nix to compile on android targets

11dae33

Make Android a Tier 2 target

552fccf

bors bot added a commit that referenced this pull request Jul 4, 2017

Merge #631

3912a20

631: Allow nix to compile on android r=Susurrus Fixes #313

bors bot merged commit 552fccf into nix-rust:master Jul 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow nix to compile on android #631

Allow nix to compile on android #631

roblabla commented Jun 27, 2017

Susurrus Jun 28, 2017

roblabla Jun 28, 2017 •

edited

Loading

roblabla Jun 28, 2017

Susurrus Jun 28, 2017

roblabla commented Jun 28, 2017 •

edited

Loading

roblabla commented Jun 28, 2017

roblabla commented Jun 28, 2017

Susurrus commented Jun 29, 2017

roblabla commented Jun 29, 2017 via email

Susurrus commented Jun 29, 2017

roblabla commented Jun 29, 2017

Susurrus commented Jul 3, 2017

ndusart commented Jul 3, 2017 •

edited

Loading

roblabla commented Jul 3, 2017

Susurrus commented Jul 3, 2017

Susurrus commented Jul 3, 2017

roblabla commented Jul 3, 2017 •

edited

Loading

Susurrus commented Jul 3, 2017

roblabla commented Jul 3, 2017

Susurrus commented Jul 3, 2017

roblabla commented Jul 4, 2017 •

edited

Loading

roblabla commented Jul 4, 2017 •

edited

Loading

ndusart commented Jul 4, 2017 •

edited

Loading

roblabla commented Jul 4, 2017

Susurrus commented Jul 4, 2017 •

edited

Loading

Susurrus commented Jul 4, 2017

roblabla commented Jul 4, 2017

roblabla commented Jul 4, 2017

Susurrus commented Jul 4, 2017

bors bot commented Jul 4, 2017

Allow nix to compile on android #631

Allow nix to compile on android #631

Conversation

roblabla commented Jun 27, 2017

Susurrus Jun 28, 2017

Choose a reason for hiding this comment

roblabla Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

roblabla Jun 28, 2017

Choose a reason for hiding this comment

Susurrus Jun 28, 2017

Choose a reason for hiding this comment

roblabla commented Jun 28, 2017 • edited Loading

roblabla commented Jun 28, 2017

roblabla commented Jun 28, 2017

Susurrus commented Jun 29, 2017

roblabla commented Jun 29, 2017 via email

Susurrus commented Jun 29, 2017

roblabla commented Jun 29, 2017

Susurrus commented Jul 3, 2017

ndusart commented Jul 3, 2017 • edited Loading

roblabla commented Jul 3, 2017

Susurrus commented Jul 3, 2017

Susurrus commented Jul 3, 2017

roblabla commented Jul 3, 2017 • edited Loading

Susurrus commented Jul 3, 2017

roblabla commented Jul 3, 2017

Susurrus commented Jul 3, 2017

roblabla commented Jul 4, 2017 • edited Loading

roblabla commented Jul 4, 2017 • edited Loading

ndusart commented Jul 4, 2017 • edited Loading

roblabla commented Jul 4, 2017

Susurrus commented Jul 4, 2017 • edited Loading

Susurrus commented Jul 4, 2017

roblabla commented Jul 4, 2017

roblabla commented Jul 4, 2017

Susurrus commented Jul 4, 2017

bors bot commented Jul 4, 2017

Build succeeded

roblabla Jun 28, 2017 •

edited

Loading

roblabla commented Jun 28, 2017 •

edited

Loading

ndusart commented Jul 3, 2017 •

edited

Loading

roblabla commented Jul 3, 2017 •

edited

Loading

roblabla commented Jul 4, 2017 •

edited

Loading

roblabla commented Jul 4, 2017 •

edited

Loading

ndusart commented Jul 4, 2017 •

edited

Loading

Susurrus commented Jul 4, 2017 •

edited

Loading