Debugger update broke the Android build

[mmatyas]

A few days ago rust-websocket was replaced with ws-rs in the debugger, and this change breaks the Android build currently.

ws-rs depends on mio, which in turn depends on nix, a dependency that wants to use the kernel signal definitions. It tries to create a sigaction structure here; the problem is that the type of sa_flags is architecture-dependent (int, unsigned int or unsigned long, see here), but nix defines it as an int here. (Note: we're using an older nix currently, but the problem still exists upstream).

In addition, it seems the type of the flags are defined incorrectly in libc too, as for example some of the possible flags are defined globally for notbsd targets as int here, despite that for other values ulong is used for notbsd/ android, int for notbsd/linux/other, and the remaining flags are not even defined for notbsd/linux/musl.

Related/blocks: #13154
cc @larsbergstrom

[larsbergstrom]

@alexcrichton Do you have opinions on the right fix here? Should we push the ulong into the global notbsd target in the Android case, too, and then fix nix to do something similar?

@ejpbruel @jdm Is it OK if we disable the debugger crate on Android for now? I'm a little afraid that making a change like this might break some theoretically-stable APIs (it did the last time we ran into this int vs. ulong Android issue), which can take a little while to push through :-)

[alexcrichton]

Unfortunately we probably can't change the type in libc itself, so the fix probably needs to go into nix

[larsbergstrom]

Makes sense - they have an issue to get working on Android already: nix-rust/nix#313 (comment)

[nox]

@alexcrichton Why can't android be its own thing instead of 'notbsd' in libc?

[ejpbruel]

Are we talking about the debugger crate or the devtools crate? If the
former, that is a work in progress, and not actively used by anything, so
it's fine to disable it.

On Thu, Nov 17, 2016 at 11:16 PM, Lars Bergstrom notifications@github.com
wrote:

@alexcrichton https://github.com/alexcrichton Do you have opinions on
the right fix here? Should we push the ulong into the global notbsd
target in the Android case, too, and then fix nix to do something similar?

@ejpbruel https://github.com/ejpbruel @jdm https://github.com/jdm Is
it OK if we disable the debugger crate on Android for now? I'm a little
afraid that making a change like this might break some theoretically-stable
APIs (it did the last time we ran into this int vs. ulong Android issue),
which can take a little while to push through :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14171 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AALW_6XMGrwFrx8_CqaxQPho-CEWDT43ks5q_NI6gaJpZM4Kvsf0
.

[nox]

Unfortunately we probably can't change the type in libc itself, so the fix probably needs to go into nix

Unfortunately someone stabilised stuff in libc that is just Plain Wrong(tm) on Android. Can we fix this? The symbols in libc are basically useless on Android.

[alexcrichton]

@nox the libc crate can be reorganized at will, and no a constant cannot change. If it's wrong a bug should be filed for the next major version of libc. In the meantime it needs to be worked around in downstream crates as we're not ready to release a new major version of the libc crate.

[nox]

@alexcrichton When something was Plain Wrong(tm) in Rust itself, it was changed back. Something was merged that is absolutely wrong on Android, and you say it cannot be changed on Android? Why?

[nox]

I don't understand why e.g. SO_DEBUG cannot be made a c_ulong on Android, when it currently cannot be useful as a c_int anyway on that platform.

[larsbergstrom]

@nox I think Alex isn't saying that it can't be changed, but just that it needs to be fixed in the next semver-breaking rev of libc.

The more interesting question, to me, is how to handle such changes. Ideally, we'd just bump the version and land it, but that would make it quite challenging to make any other fixes to libc between now and when they want to publish the next breaking version of libc.

This is a problem we frequently run into in Servo's dependencies, and often have fixed in pretty ad-hoc ways (e.g., just making a random commit that also has a version bump, publishing that, leaving around a random old-version branch in case more are needed, and then merging the real thing onto master).

[nox]

@larsbergstrom I understand that. I just differ on the opinion that fixing something that is broken is a breaking change.

[larsbergstrom]

@nox As a counterpoint, I'd like to refer you to the inimitable nokusu:
https://twitter.com/nokusu/status/788285254322774016

i.e., if we did land the fix w/o a breaking change, it would break nix (and presumably others). That's exactly what happened in the libc 0.2 days with a semver-stealth-Android change of a similar nature and the ripple on the ecosystem was far worse for Servo, imo.

[nox]

@larsbergstrom I think that the only way of using e.g. SO_DEBUG currently on Android means you have to do SO_DEBUG as c_ulong for it to be useful, and that wouldn't break if we made it a c_ulong in libc in a patch bump. If I'm wrong about this, disregard me.

Edit: but touché quoting my own tweet at me, that made me chuckle.

[larsbergstrom]

@nox Yeah, the breakage I've seen in the past is two forms:

1. people are using/wrapping/passing around the types and compiling on Android but not actually running the code at all
2. they bleed the brokenness all the way down to the FFI layer and just YOLO-pass a rust c_int into a C function that has something of type unsigned long

So admittedly this is a bit of an ad-hoc statement w/o a crater run but based on previous Rust Android fun, I can almost guarantee you'd break some widely-used packages if you changed it, even from its incorrect definition today.