Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [webauthn], )

Attachments

(3 attachments, 7 obsolete attachments)

90.37 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
20.47 KB, patch
ted
: review+
Details | Diff | Splinter Review
143.47 KB, patch
gerv
: review+
qdot
: review+
Details | Diff | Splinter Review
Kyle started the u2f-hid-rs library a while ago, JC and I finished it. We think it's ready to land in Firefox so we don't have to share a pile of patches to work on U2F, and to also start giving builds to QA and partners.
Depends on: 1380270
Blocks: 1388851
This is a straight copy of everything in [1], except the .git directory. I think that's what "mach vendor" would do, correct me if I'm wrong.

The library supports Windows, Linux, and macOS. Linux needs libudev, that's what bug 1380270 is for.

The PlatformManager and DeviceMap for each platform looks very similar. We intentionally did not combine those as we're not confident it would let us easily add Bluetooth support in the future.

[1] https://github.com/jcjones/u2f-hid-rs
Attachment #8895501 - Attachment description: 0003-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch → 0001-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch
This patch has the necessary Cargo.toml changes in toolkit/library/rust to build u2f-hid-rs. It also places the override for "libudev-sys:0.1.3" (see bug 1380270).
The last of three patches, "vendoring" the u2f-hid-rs dependencies libudev, libudev-sys, and boxfnonce.

Please not that we don't actually need libudev-sys as we're using the one from bug 1380270, but "mach vendor" seems ignore [replace] directives and downloads the library anyway. We're not building it though, so I'd like to talk to build folks and just fix this later.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
Comment on attachment 8895501 [details] [diff] [review]
0001-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch

Kyle, I think it makes sense to have you review this library even though you started it originally. It's changed quite a bit :) Can you suggest an additional reviewer to look at Rust code?
Attachment #8895501 - Flags: review?(kyle)
Comment on attachment 8895502 [details] [diff] [review]
0002-Bug-1388843-Part-2-Add-u2fhid-to-the-shared-rust-lib.patch

This seems like another one for Ted, it's mainly build system integration with Firefox' shared Rust library.
Attachment #8895502 - Flags: review?(ted)
Comment on attachment 8895504 [details] [diff] [review]
0003-Bug-1388843-Part-3-Vendor-u2f-hid-rs-dependencies.patch

Ted, would you be a good reviewer for this as well? These are the vendored Rust dependencies. Including the unused libudev-sys.
Attachment #8895504 - Flags: review?(ted)
Update with some rust-clippy fixes from Manish.
Attachment #8895501 - Attachment is obsolete: true
Attachment #8895501 - Flags: review?(kyle)
Attachment #8896185 - Flags: review?(kyle)
See Also: → 1391438
(In reply to Tim Taubert [:ttaubert] from comment #1)
> Created attachment 8895501 [details] [diff] [review]
> 0001-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch
> 
> This is a straight copy of everything in [1], except the .git directory. I
> think that's what "mach vendor" would do, correct me if I'm wrong.

`mach vendor rust` copies the contents of crates from crates.io. In practice this is basically the same thing, but I'm curious as to what the plan for support is going forward--is the github repository going to be the canonical repo, or is the copy in m-c going to be canonical? Either way, are you going to wind up double-landing patches or something? So far we've been keeping Rust crates that are developed elsewhere in `third_party/rust` via the existing vendoring mechanism, and only things that are actively developed in-tree live elsewhere in the tree (which mostly winds up being glue code for Gecko).

I'd like to know what the plan is around that before we land this in m-c. If it's going to be actively developed there, then that seems fine. You can still release to crates.io from within m-c, it just means that anyone else wanting to contribute will have to clone m-c and submit patches via bugzilla instead of GitHub.
Comment on attachment 8895504 [details] [diff] [review]
0003-Bug-1388843-Part-3-Vendor-u2f-hid-rs-dependencies.patch

Review of attachment 8895504 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine for now. File a followup on `mach vendor rust` ignoring [replace] and we'll sort it out there.
Attachment #8895504 - Flags: review?(ted) → review+
Comment on attachment 8895502 [details] [diff] [review]
0002-Bug-1388843-Part-2-Add-u2fhid-to-the-shared-rust-lib.patch

Review of attachment 8895502 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/library/rust/Cargo.toml
@@ +44,5 @@
>  debug-assertions = false
>  panic = "abort"
> +
> +[replace]
> +"libudev-sys:0.1.3" = { path = "../../../dom/webauthn/libudev-sys" }

So you've got this here, but not in toolkit/library/gtest/rust/Cargo.toml. In practice this means that gtests will wind up linking the original version of libudev-sys, which is probably not what you want.
Attachment #8895502 - Flags: review?(ted)
Thanks for taking a look, Ted!

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> I'd like to know what the plan is around that before we land this in m-c. If
> it's going to be actively developed there, then that seems fine. 

We're open to both, and were kind of hoping you'd have some advice as to which we should do. :) If you were producing what's effectively a rust lib for U2F, I guess it'd make sense to leave it on Github and publish it to crates.io, then?
That's certainly friendlier to outside contributors, but it means more friction for updating the copy in mozilla-central. If you think you're going to be doing a lot of development on this crate and it's going to be tied to work in Firefox, you might want to just host it in m-c.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)
> That's certainly friendlier to outside contributors, but it means more
> friction for updating the copy in mozilla-central. If you think you're going
> to be doing a lot of development on this crate and it's going to be tied to
> work in Firefox, you might want to just host it in m-c.

Hmm. Maybe we can split the difference a bit, and start with it being purely in m-c, and in a follow-up, once everything is pretty stable, move it back out.
My original plan was to possibly split usage of this between m-c and servo, but for right now the concentration should definitely be on m-c. After that's integrated we can work from there to adapt as needed/as we have time. It's not like it's a ton of code anyways.
See Also: → 1395293
Tiny update after rebasing.

r=ted
Attachment #8895504 - Attachment is obsolete: true
Attachment #8903515 - Flags: review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> ::: toolkit/library/rust/Cargo.toml
> @@ +44,5 @@
> >  debug-assertions = false
> >  panic = "abort"
> > +
> > +[replace]
> > +"libudev-sys:0.1.3" = { path = "../../../dom/webauthn/libudev-sys" }
> 
> So you've got this here, but not in toolkit/library/gtest/rust/Cargo.toml.
> In practice this means that gtests will wind up linking the original version
> of libudev-sys, which is probably not what you want.

Good point, I think we already saw this as Linux build failures on try.
Attachment #8895502 - Attachment is obsolete: true
Attachment #8903516 - Flags: review?(ted)
* A small update to the README file.
* Added a blank Android platform template.
* Fix for #39, properly unpack APDU status codes

As JC and you already said, the plan is to get this landed in m-c, wait for it be tested and shipped, and then transition it back to GitHub.
Attachment #8896185 - Attachment is obsolete: true
Attachment #8896185 - Flags: review?(kyle)
Attachment #8903519 - Flags: review?(kyle)
(In reply to Tim Taubert [:ttaubert] from comment #17)
> As JC and you already said, the plan is to get this landed in m-c, wait for
> it be tested and shipped, and then transition it back to GitHub.

BTW, that's tracked by bug 1395293.
Ok, working on code review now, but that may take a day or two. Glaring omission on Patch 1 though: there's no license. We definitely need one.

I'm honestly not sure how this should licensed, since it's an external library we're bringing in. I guess we could go MPL as we do with gecko code? Bringing in :gerv for guidance here.

gerv: This is a rust library that can work outside of gecko and may get integrated into servo. We need to integrate it into the gecko tree for now. Should we just call it MPL and add headers like it's gecko code?
Flags: needinfo?(gerv)
Comment on attachment 8903519 [details] [diff] [review]
0001-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch, v3

Review of attachment 8903519 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I've done what I can in terms of code review. This is a massive chunk of code across multiple specific platforms, so I can't really say with certainty that my review is gonna catch everything, but it is what it is I guess.

I'm r-'ing right now mostly for the licensing issues. I can't r+ until we know how that's going to be handled, and I don't like just clearing reviews. 

All of my comments are mostly nits and code style issues, though the massive chunks of repeated code in the mod.rs files is also worrisome, but I may be missing something there?

Also, since this is its own library that will also live in a github repo, should this go in third_party/rust instead of in the dom tree?

::: dom/webauthn/u2f-hid-rs/src/android/mod.rs
@@ +16,5 @@
> +        &mut self,
> +        timeout: u64,
> +        challenge: Vec<u8>,
> +        application: Vec<u8>,
> +        callback: OnceCallback<Vec<u8>>,

Here and pretty much everywhere else:

I haven't seen rust code using trailing commas on lists and parens on next line before, and I can't find any other code in our tree that uses this style (though that's just a cursory search through a few random files). On other functions where we have multiple arguments on the same line, there's no trailing comma. It feels a little inconsistent and hard to read?

::: dom/webauthn/u2f-hid-rs/src/capi.rs
@@ +32,5 @@
> +}
> +
> +#[no_mangle]
> +pub unsafe extern "C" fn rust_u2f_mgr_free(mgr: *mut U2FManager) {
> +    if !mgr.is_null() {

nit: Here and throughout read of code: usually prefer early return style, as happens below for is_null checks, for consistency.

::: dom/webauthn/u2f-hid-rs/src/linux/hidraw.rs
@@ +73,5 @@
> +            return None;
> +        }
> +
> +        let key = self.desc.value[self.pos];
> +        if key & 0xf0 == 0xf0 {

nit: there's an awful lot of magic numbers around here. Can these be turned into defines?

::: dom/webauthn/u2f-hid-rs/src/linux/mod.rs
@@ +18,5 @@
> +    // Handle to the thread loop.
> +    thread: Option<RunLoop>,
> +}
> +
> +impl PlatformManager {

This struct looks almost exactly the same between all platforms. Why is it repeated 3 different times in the code?

@@ +114,5 @@
> +                                    device,
> +                                    &challenge,
> +                                    &application,
> +                                    key_handle,
> +                                )

nit: Consistency, should be on one line.

::: dom/webauthn/u2f-hid-rs/src/macos/device.rs
@@ +181,5 @@
> +        if let Err(e) = tx.send(data) {
> +            // TOOD: This happens when the channel closes before this thread
> +            // does. This is pretty common, but let's deal with stopping
> +            // properly later.
> +            warn!("Problem returning read_new_data_cb data for thread: {}", e);

Where are messages like this going to come out? Are we able to push them to gecko logs?

::: dom/webauthn/u2f-hid-rs/src/macos/monitor.rs
@@ +62,5 @@
> +                    trace!("OSX Runloop running, handle={:?}", thread::current());
> +
> +                    if unsafe { CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.1, 0) } ==
> +                        kCFRunLoopRunStopped
> +                    {

nit: style

::: dom/webauthn/u2f-hid-rs/src/manager.rs
@@ +121,5 @@
> +    ) -> io::Result<()>
> +    where
> +        F: FnOnce(io::Result<(Vec<u8>, Vec<u8>)>),
> +        F: Send + 'static,
> +    {

nit: style
Attachment #8903519 - Flags: review?(kyle) → review-
(In reply to Kyle Machulis [:qdot] [:kmachulis]  (if a patch has no decent commit message, automatic r-) from comment #19)
> Ok, working on code review now, but that may take a day or two. Glaring
> omission on Patch 1 though: there's no license. We definitely need one.
> 
> I'm honestly not sure how this should licensed, since it's an external
> library we're bringing in. I guess we could go MPL as we do with gecko code?
> Bringing in :gerv for guidance here.
> 
> gerv: This is a rust library that can work outside of gecko and may get
> integrated into servo. We need to integrate it into the gecko tree for now.
> Should we just call it MPL and add headers like it's gecko code?

I've added MPL licensing in https://github.com/jcjones/u2f-hid-rs/commit/c02a80e08565fabc077b3867142d37c1dc218dbf ; though I'll leave the needinfo for Gerv to check me. But since Servo is all MPL, it seems like that's a reasonable default.
Thanks for the review! Replies below:

(In reply to Kyle Machulis [:qdot] [:kmachulis]  (if a patch has no decent commit message, automatic r-) from comment #20)
> I'm r-'ing right now mostly for the licensing issues. I can't r+ until we
> know how that's going to be handled, and I don't like just clearing reviews. 

I agree, let's wait on Gerv. I'll upload a new patch with JC's MPL changes.

> All of my comments are mostly nits and code style issues, though the massive
> chunks of repeated code in the mod.rs files is also worrisome, but I may be
> missing something there?

You're right in that we actually only need a single PlatformManager as the state machine. We could also likely merge DeviceMaps.

We decided to not do this at the moment because we don't want to have to revert all of this if we discover that U2F via Bluetooth requires a slightly different setup. If you think that's not reasonable we can merge as much code as possible and use the git/hg history later if needed.

> Also, since this is its own library that will also live in a github repo,
> should this go in third_party/rust instead of in the dom tree?

I'd like to put it there but "mach vendor rust" removes everything in there and vendors 3rd-party libs back in. So as long as we only develop it on m-c I'd leave it in dom/webauthn and move it to third_party/rust when we "release" it as a standalone lib.

> I haven't seen rust code using trailing commas on lists and parens on next
> line before, and I can't find any other code in our tree that uses this
> style (though that's just a cursory search through a few random files). On
> other functions where we have multiple arguments on the same line, there's
> no trailing comma. It feels a little inconsistent and hard to read?

That's actually not something we prefer, it's what rustfmt (via cargo fmt) does. We used mostly the default settings, that seemed the right thing to do. Citing from the rustfmt repo:

"By default, Rustfmt uses a style which conforms to the Rust style guide. For details that have not yet been formalized through the style RFC process, we try to adhere to a style similar to that used in the Rust repo."

> ::: dom/webauthn/u2f-hid-rs/src/capi.rs
> @@ +32,5 @@
> > +}
> > +
> > +#[no_mangle]
> > +pub unsafe extern "C" fn rust_u2f_mgr_free(mgr: *mut U2FManager) {
> > +    if !mgr.is_null() {
> 
> nit: Here and throughout read of code: usually prefer early return style, as
> happens below for is_null checks, for consistency.

I'm a fan of early return as well, but it seems not a big win if we have `if (cond) return; action();`? OTOH it would make adding more early return conditions easier in the future.

> ::: dom/webauthn/u2f-hid-rs/src/linux/hidraw.rs
> @@ +73,5 @@
> > +            return None;
> > +        }
> > +
> > +        let key = self.desc.value[self.pos];
> > +        if key & 0xf0 == 0xf0 {
> 
> nit: there's an awful lot of magic numbers around here. Can these be turned
> into defines?

Good point, will clean this up.

> > +impl PlatformManager {
> 
> This struct looks almost exactly the same between all platforms. Why is it
> repeated 3 different times in the code?

(see answer above)

> @@ +114,5 @@
> > +                                    device,
> > +                                    &challenge,
> > +                                    &application,
> > +                                    key_handle,
> > +                                )
> 
> nit: Consistency, should be on one line.

(rustfmt)

> ::: dom/webauthn/u2f-hid-rs/src/macos/device.rs
> @@ +181,5 @@
> > +        if let Err(e) = tx.send(data) {
> > +            // TOOD: This happens when the channel closes before this thread
> > +            // does. This is pretty common, but let's deal with stopping
> > +            // properly later.
> > +            warn!("Problem returning read_new_data_cb data for thread: {}", e);
> 
> Where are messages like this going to come out? Are we able to push them to
> gecko logs?

I honestly don't know much about our logging integration yet. Would you be fine exploring this in a follow-up? This should probably be a requirement before we pref it on on Nightly.

> ::: dom/webauthn/u2f-hid-rs/src/macos/monitor.rs
> @@ +62,5 @@
> > +                    trace!("OSX Runloop running, handle={:?}", thread::current());
> > +
> > +                    if unsafe { CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.1, 0) } ==
> > +                        kCFRunLoopRunStopped
> > +                    {
> 
> nit: style

(rustfmt)

> ::: dom/webauthn/u2f-hid-rs/src/manager.rs
> @@ +121,5 @@
> > +    ) -> io::Result<()>
> > +    where
> > +        F: FnOnce(io::Result<(Vec<u8>, Vec<u8>)>),
> > +        F: Send + 'static,
> > +    {
> 
> nit: style

(rustfmt)
r? gerv for the MPL license question

Kyle, this patch contains the license file and headers (to be signed off by Gerv) and fixes the magic numbers in linux/hidraw.rs. I seized the chance to cleanup hidraw.rs some more and add support for skipping long items, in case that's a thing out there.
Attachment #8903519 - Attachment is obsolete: true
Attachment #8908095 - Flags: review?(kyle)
Attachment #8908095 - Flags: review?(gerv)
Still r? gerv for the MPL license question

Updated the C API type names for bug 1388851.
Attachment #8908095 - Attachment is obsolete: true
Attachment #8908095 - Flags: review?(kyle)
Attachment #8908095 - Flags: review?(gerv)
Attachment #8908109 - Flags: review?(kyle)
Attachment #8908109 - Flags: review?(gerv)
Comment on attachment 8908109 [details] [diff] [review]
0001-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch, v5

r=gerv. MPL2 is a fine choice.

Gerv
Flags: needinfo?(gerv)
Attachment #8908109 - Flags: review?(gerv) → review+
Attachment #8903516 - Flags: review?(ted) → review+
Comment on attachment 8908109 [details] [diff] [review]
0001-Bug-1388843-Part-1-Copy-u2f-hid-rs-into-dom-webauthn.patch, v5

Review of attachment 8908109 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, followups are fine, and my nits were more questions/recommendations than requirements. LGTM.
Attachment #8908109 - Flags: review?(kyle) → review+
Lemme land this.
Keywords: checkin-needed

Comment 28

2 years ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/435c255e8718
Part 1: Copy u2f-hid-rs into dom/webauthn/ r=gerv,qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ca504303b8
Part 2: Add u2fhid to the shared rust library r=ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0cba1eef91
Part 3: Vendor u2f-hid-rs dependencies r=ted

Updated

2 years ago
Depends on: 1400066
Did you use `mach vendor rust` to do the vendoring of the rust dependencies? When I run this command on current m-c it removes the following files:

third_party/rust/libudev-sys/target/build/check_udev_hwdb_new
third_party/rust/libudev-sys/target/build/check_udev_hwdb_new.rs

I don't know where these files came from (in your patch), because they're not in the libudev-sys package hosted on crates.io:

wget -O - https://crates.io/api/v1/crates/libudev-sys/0.1.3/download | tar tz
[... wget output omitted ...]
libudev-sys-0.1.3/.gitignore
libudev-sys-0.1.3/.travis.yml
libudev-sys-0.1.3/build.rs
libudev-sys-0.1.3/Cargo.toml
libudev-sys-0.1.3/examples/hwdb_query.rs
libudev-sys-0.1.3/examples/list_devices.rs
libudev-sys-0.1.3/examples/monitor.rs
libudev-sys-0.1.3/LICENSE
libudev-sys-0.1.3/README.md
libudev-sys-0.1.3/src/lib.rs
libudev-sys-0.1.3/cross-build.sh
Flags: needinfo?(ttaubert)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> Did you use `mach vendor rust` to do the vendoring of the rust dependencies?
> When I run this command on current m-c it removes the following files:
> 
> third_party/rust/libudev-sys/target/build/check_udev_hwdb_new
> third_party/rust/libudev-sys/target/build/check_udev_hwdb_new.rs
> 
> I don't know where these files came from (in your patch), because they're
> not in the libudev-sys package hosted on crates.io:

They're generated by build.rs:

https://github.com/dcuddeback/libudev-sys/blob/master/build.rs

This is a little unfortunate as we don't actually need libudev-sys itself. We have our own copy but "mach vendor rust" doesn't respect the [replace] directives in the Cargo.toml files.

Also, "mach vendor rust" on Linux yields no working copy modifications. I assume you're on macOS because then it too wants to remove the check_udev* files. libudev is Linux-only anyway...
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #31)
> They're generated by build.rs:
> 
> https://github.com/dcuddeback/libudev-sys/blob/master/build.rs

Hm, but they shouldn't be getting generated at vendoring time. They should be getting generated at build time, into the objdir. AFAIK we don't run build.rs at vendoring time.

> This is a little unfortunate as we don't actually need libudev-sys itself.
> We have our own copy but "mach vendor rust" doesn't respect the [replace]
> directives in the Cargo.toml files.

I'm not sure if this is intentional or something that can be changed.

> Also, "mach vendor rust" on Linux yields no working copy modifications. I
> assume you're on macOS because then it too wants to remove the check_udev*
> files. libudev is Linux-only anyway...

Nope, I was running this on Linux. I'm quite surprised that it doesn't result in working copy modifications for you. I'm wondering if maybe somehow the vendoring process is picking up the [replace] incorrectly, and vendoring it from somewhere else for you, perhaps a copy which has those files already built? Since I never built that crate I don't have those files anywhere else on my system. Can you try doing `cargo clean` wherever else you have this crate and see if that makes a difference?

/cc :froydnj also
(In reply to Tim Taubert [:ttaubert] from comment #31)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> > Did you use `mach vendor rust` to do the vendoring of the rust dependencies?
> > When I run this command on current m-c it removes the following files:
> > 
> > third_party/rust/libudev-sys/target/build/check_udev_hwdb_new
> > third_party/rust/libudev-sys/target/build/check_udev_hwdb_new.rs
> > 
> > I don't know where these files came from (in your patch), because they're
> > not in the libudev-sys package hosted on crates.io:
> 
> They're generated by build.rs:
> 
> https://github.com/dcuddeback/libudev-sys/blob/master/build.rs
> 
> This is a little unfortunate as we don't actually need libudev-sys itself.
> We have our own copy but "mach vendor rust" doesn't respect the [replace]
> directives in the Cargo.toml files.

I read through the bug after reading this, and I'm still not clear what's going on here:

* We don't need libudev-sys (?)
* But we still have it vendored (?)
* And the vendoring includes files that aren't actually in the released package?

Can you clarify for me, please?

The build shouldn't need stuff in $crate_srcdir/target/build, because those files should get regenerated.  If the crate doesn't work correctly in our build system, then that's a bug in the crate...
Flags: needinfo?(ttaubert)
(In reply to Nathan Froyd [:froydnj] from comment #33)
> I read through the bug after reading this, and I'm still not clear what's
> going on here:
> 
> * We don't need libudev-sys (?)

We do need libudev-sys on Linux. The original version however links against libudev.so.2, and we need a dlopen() version that can silently fail at runtime if libudev isn't installed. Basically similar to what we did with the Gamepad code [1].

So I took the original crate and rewrote it. libudev-sys is a dependency of libudev, so we're replacing that with a [replace] directive.

[1] http://searchfox.org/mozilla-central/source/dom/gamepad/linux/udev.h

> * But we still have it vendored (?)

Yeah, we have our copy in dom/webauthn/libudev-sys but cargo update/vendor still pulls in the original version from crates.io that we're not using.

> * And the vendoring includes files that aren't actually in the released
> package?

That seems like it might be solved by me running cargo clean, I'll give that a try.

> The build shouldn't need stuff in $crate_srcdir/target/build, because those
> files should get regenerated.  If the crate doesn't work correctly in our
> build system, then that's a bug in the crate...

That might be a bug too, yeah.
Flags: needinfo?(ttaubert)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> on my system. Can you try doing `cargo clean` wherever else you have this
> crate and see if that makes a difference?

I removed everything in ~/.cargo and now I have a diff that removes *every* .cargo-ok file and modifies every cargo-checksum.json file. Whatever that's about.

BUT, it also removes the two check_udev*.rs files that were probably somehow cached on my system.
(In reply to Tim Taubert [:ttaubert] from comment #35)
> I removed everything in ~/.cargo and now I have a diff that removes *every*
> .cargo-ok file and modifies every cargo-checksum.json file. Whatever that's
> about.

Weird, no idea why that would happen.

> BUT, it also removes the two check_udev*.rs files that were probably somehow
> cached on my system.

Ok, I'll put up a patch for this.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)
> (In reply to Tim Taubert [:ttaubert] from comment #35)
> > I removed everything in ~/.cargo and now I have a diff that removes *every*
> > .cargo-ok file and modifies every cargo-checksum.json file. Whatever that's
> > about.
> 
> Weird, no idea why that would happen.

I actually think there was a cargo-vendor update that started ignoring .cargo-ok files.
Comment on attachment 8908663 [details] [diff] [review]
Remove generated files from libudev-sys vendoring

Review of attachment 8908663 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8908663 - Flags: review?(ttaubert) → review+
Depends on: 1400513
Comment on attachment 8908663 [details] [diff] [review]
Remove generated files from libudev-sys vendoring

Patch is obsolete, the servo vcs-sync bot already took care of this in caet aa02aa853d37
Attachment #8908663 - Attachment is obsolete: true
Either the second or third part of this patch is causing a build failure when building Fennec on MacOS:

> 0:25.19 error: native frameworks are only available on macOS targets
> 0:25.19 
> 0:25.35 error: aborting due to previous error
> 0:25.35 
> 0:25.38 error: Could not compile `u2fhid`.

Tim, any ideas?
Flags: needinfo?(ttaubert)
Depends on: 1400927
droeh: can we get more of the log?

ted: is this supposed to be enabled on all platforms?  Is it possible to make Rust modules conditional on target OS?

ttaubert: same question about whether this is supposed to support Android?

We should probably back this out if it's not very easy to fix the Mac OS build failures, since I think most of the relevant Android devs are running macOS.
Flags: needinfo?(ted)
Flags: needinfo?(droeh)
(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #42)
> ted: is this supposed to be enabled on all platforms?  Is it possible to
> make Rust modules conditional on target OS?

I don't know about whether this is supposed to be enabled on all platforms. If not, the most straightforward way to support that would probably be adding a cargo feature and conditionally enabling it via moz.build:
https://dxr.mozilla.org/mozilla-central/source/toolkit/library/rust/gkrust-features.mozbuild
Flags: needinfo?(ted)
(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #42)
> droeh: can we get more of the log?

https://gist.github.com/anonymous/5bb38867f3cc8b7890fc00f026398b55
Flags: needinfo?(droeh)
(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #42)
> droeh: can we get more of the log?
> 
> ted: is this supposed to be enabled on all platforms?  Is it possible to
> make Rust modules conditional on target OS?
> 
> ttaubert: same question about whether this is supposed to support Android?

Answering at least partially: Yes, this is supposed to build and do no harm on Android; we have a stub in place, and try builds from Linux for Android are fine. 

I'm setting myself up to build for Android now from OSX so I can try to debug this.
(In reply to J.C. Jones [:jcj] from comment #45)
> (In reply to Nick Alexander :nalexander [parental leave until September
> 15th] from comment #42)
> > droeh: can we get more of the log?
> > 
> > ted: is this supposed to be enabled on all platforms?  Is it possible to
> > make Rust modules conditional on target OS?
> > 
> > ttaubert: same question about whether this is supposed to support Android?
> 
> Answering at least partially: Yes, this is supposed to build and do no harm
> on Android; we have a stub in place, and try builds from Linux for Android
> are fine. 

Great!

> I'm setting myself up to build for Android now from OSX so I can try to
> debug this.

It looks like the Rust "target=" invocation isn't quite right for cross compilation; first glance suggests the target is really the host in the failing logs.

froydnj: rillian: I think you two know the most about the Rust integration.  Can you take a look at this ticket and tell me whether we expect Rust's target= to correctly understand cross compilation?  (I would expect the answer is yes, or else Fennec would burn, but perhaps not.)
Flags: needinfo?(nfroyd)
Flags: needinfo?(giles)
Let me lead by saying this is a terrible hack, but changing http://searchfox.org/mozilla-central/source/dom/webauthn/u2f-hid-rs/build.rs#6 to

#[cfg(all(target_os = "macos", target_env = "gnu"))]

seems to fix this, since target_env is not "gnu" when building with the NDK on OS X.

My best guess as to what's happening here is that Cargo evaluates build.rs to decide what compiler flags to toss in, but when it's doing that for a cross-compile, it's not setting the target_os flag, as from its' perspective the target of the compiler flag setting script is the compile host.

I'll make a patch just in case this is the real fix.
I've put a patch up on Bug 1400927.
Flags: needinfo?(ttaubert)
(In reply to Nick Alexander :nalexander from comment #46)
> (In reply to J.C. Jones [:jcj] from comment #45)
> > I'm setting myself up to build for Android now from OSX so I can try to
> > debug this.
> 
> It looks like the Rust "target=" invocation isn't quite right for cross
> compilation; first glance suggests the target is really the host in the
> failing logs.
> 
> froydnj: rillian: I think you two know the most about the Rust integration. 
> Can you take a look at this ticket and tell me whether we expect Rust's
> target= to correctly understand cross compilation?  (I would expect the
> answer is yes, or else Fennec would burn, but perhaps not.)

The long and short of it is that #[cfg(target_os)] tests for the OS that the *current file* is built for.  For build.rs, that is the OS that the entire build is running on, since we need to run the resulting program to generate files, print directives for cargo to parse, etc. etc.  In the cross-compilation case `target_os` in build.rs is *not* the OS that Firefox is going to run on.  It so happens that this is broken for Android, as you have noticed; it is also broken for our OS X cross-compilation builds in automation.  We don't notice the breakage on OS X, though, because we link in the needed IOKit to libxul anyway.

Rust's #[cfg(target_os)] does cooperate with cross-compilation, but not in the way it's being used here.  If you were building a normal crate (no build.rs), for instance, and you consulted #[cfg(target_os = "Android")] (or whatever the incantation is), that *would* work correctly.  There is a documented-in-an-RFC way for build scripts to tell what the actual "target" of interest, i.e. not the place the build script will be running, but the sort of system that we're building this entire crate for, is:

https://github.com/rust-lang/rfcs/blob/master/text/1721-crt-static.md#lowering-cfg-values-to-cargo-build-script-environment-variables

These variables should be consulted instead of the #[cfg(target_os)] dance currently there.
Flags: needinfo?(nfroyd)
I agree with Nick's assessment. You can't use #[cfg(target_os=foo)] in build.rs. The build script is intended for generating code, so target=host when it's compiled. That generates an unconditional link dependency on IOKit when the host is macOS which kills the final build when the target is not macOS.

I'm not sure what the correct fix here is, though. The framework is only needed for final link, and gecko will pull that in anyway, so you could just remove build.rs. That will break `cargo test` but we don't run that for in-tree crates currently.

If you were importing these symbols directly, you could hang a #[link(name = "IOKit", kind = "framework")] attribute on the extern block, but it looks like you're getting the symbols from corefoundation-sys. Perhaps it's a bug upstream then, if they're exporting symbols which are really in IOKit but there's no transitive link to that framework.
I submitted a fix in bug 1400927.
Flags: needinfo?(giles)
I'm glad I asked, 'cuz I learned something from both froydnj and rillian.  Thanks, team!
Depends on: 1404556
Comment on attachment 8903515 [details] [diff] [review]
0003-Bug-1388843-Part-3-Vendor-u2f-hid-rs-dependencies.patch, v2

Review of attachment 8903515 [details] [diff] [review]:
-----------------------------------------------------------------

::: third_party/rust/boxfnonce/src/macros.rs
@@ +1,2 @@
> +macro_rules! impl_trait_n_args {
> +	( $($var:ident: $typevar:ident),* ) => (

This file uses 8 space indents. It should use 4 space indents.

::: third_party/rust/boxfnonce/src/no_send.rs
@@ +49,5 @@
> +/// ```
> +pub struct BoxFnOnce<Arguments, Result = ()> (Box<FnBox<Arguments, Result>>);
> +
> +impl<Args, Result> BoxFnOnce<Args, Result> {
> +	/// call inner function, consumes the box.

ditto

::: third_party/rust/boxfnonce/src/send.rs
@@ +40,5 @@
> +/// ```
> +pub struct SendBoxFnOnce<Arguments, Result = ()> (Box<FnBox<Arguments, Result> + Send>);
> +
> +impl<Args, Result> SendBoxFnOnce<Args, Result> {
> +	/// call inner function, consumes the box.

ditto
Nick, we unfortunately can't do anything about the formatting here. Those resources are pulled into the tree via "mach vendor rust" and are maintained by a third party.
Depends on: 1413598
Depends on: 1418234
Depends on: 1418242
Depends on: 1419070
Depends on: 1419685
Depends on: 1419907
You need to log in before you can comment on or make changes to this bug.