Regarding CSC 517 OSS project

[Sruthi Kannan]

Hello,

I am a NC state graduate student working in the Refactoring Bluetooth
support project with Niveditha.
We went went through the documentation on traits. We arrived at the
following approach and we just want to make sure we are doing it right.

1) Create an adapter.rs file. Define a trait object using a vector. Create
a structure, say DeviceType for the same.

2) Create individual structures for each component(like Android,Mac..) and
implement BluetoothAdapter function in each.

3) Make changes in the servo crate to call adapter.rs

Are we right in assuming that the current BluetoothAdapter function has to
be broken for each component and put in the respective structure?

Thank you.

[Josh Bowman-Matthews]

That sounds like the right idea. I'm not quite sure what this question
is asking, however:

> Are we right in assuming that the current BluetoothAdapter function
has to
> be broken for each component and put in the respective structure?

If you are asking if code like
https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L276-L305
should be distributed across the various trait implementations, then the
answer is yes.

Cheers,
Josh

[Niveditha Shankar]

Thank you!
We meant to ask if code in
https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L276-L305
should
be split and the functions corresponding to say linux should be put in the
structure we created for implementing BluetoothAdapter for linux.

Sincerely
Niveditha Shankar

On Sat, Mar 30, 2019 at 12:26 AM Josh Bowman-Matthews <jo...@joshmatthews.net>
wrote:

> That sounds like the right idea. I'm not quite sure what this question
> is asking, however:
>

> > Are we right in assuming that the current BluetoothAdapter function
> has to
> > be broken for each component and put in the respective structure?
>

> If you are asking if code like
>
> https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L276-L305
> should be distributed across the various trait implementations, then the
> answer is yes.
>
> Cheers,
> Josh
>
> On 3/28/19 8:23 PM, Sruthi Kannan wrote:

> _______________________________________________
> dev-servo mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-servo
>

[Josh Bowman-Matthews]

The init functions are actually a bit special, because they can't be
turned into static methods on the new trait without running into compile
errors. I recommend doing this instead:

trait BluetoothAdapter {
// ...
}

impl BluetoothAdapter {
#[cfg(all(target_os = "macos", feature = "bluetooth"))]
pub fn new() -> Result<Box<BluetoothAdapter>, Box<Error>> {
// ...
}

#[cfg(all(target_os = "linux", feature = "bluetooth"))]
pub fn new() -> Result<Box<BluetoothAdapter>, Box<Error>> {
// ...
}
}

This will allow callers to use `BluetoothAdapter::new()` to create a new
instance.

Cheers,
Josh

[Niveditha Shankar]

Okay I think I get it. Thank you!

Is there anyway I can check if the changes I make to the devices crate is
right?

Our submission is on Monday and we are going to continue working with
Webbluetooth for our final project too. So if our implementation of our
adapter.rs file is right, we can go ahead and do something similar for the
rest of the enum values. It would be really helpful if there is a way to
check that.


Niveditha Shankar



On Sat, Mar 30, 2019, 12:25 PM Josh Bowman-Matthews <jo...@joshmatthews.net
wrote:

> The init functions are actually a bit special, because they can't be
> turned into static methods on the new trait without running into compile
> errors. I recommend doing this instead:
>
> trait BluetoothAdapter {
> // ...
> }
>
> impl BluetoothAdapter {
> #[cfg(all(target_os = "macos", feature = "bluetooth"))]
> pub fn new() -> Result<Box<BluetoothAdapter>, Box<Error>> {
> // ...
> }
>
> #[cfg(all(target_os = "linux", feature = "bluetooth"))]
> pub fn new() -> Result<Box<BluetoothAdapter>, Box<Error>> {
> // ...
> }
> }
>
> This will allow callers to use `BluetoothAdapter::new()` to create a new
> instance.
>
> Cheers,
> Josh
>
> On 3/30/19 11:40 AM, Niveditha Shankar wrote:

[Niveditha Shankar]

We meant to ask if code in
https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L276-L305
should
be split and the functions corresponding to say linux should be put in the
structure we created for implementing BluetoothAdapter for linux.

For example,

pub struct Bluez(Arc<BluetoothAdapterBluez>){
pub target_os: String,
pub feature: String
}
impl BluetoothAdapter for Bluez(Arc<BluetoothAdapterBluez){
fn BluetoothAdapter(&self){
pub fn new() -> Result<BluetoothAdapter, Box<Error>> {
.......
}
}
pub fn get_id(&self) -> String {
get_inner_and_call!(self, BluetoothAdapter, get_id)
}
// code for just BluetoothaAdapterBluez
}

//Similarly for Mac/Android/Empty/BluetoothTest

-Niveditha Shankar

On Sat, Mar 30, 2019 at 11:40 AM Niveditha Shankar <nsha...@ncsu.edu>

wrote:

> Thank you!
> We meant to ask if code in
> https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L276-L305 should
> be split and the functions corresponding to say linux should be put in the
> structure we created for implementing BluetoothAdapter for linux.
>
> Sincerely
> Niveditha Shankar
>
> On Sat, Mar 30, 2019 at 12:26 AM Josh Bowman-Matthews <
> jo...@joshmatthews.net> wrote:
>
>> That sounds like the right idea. I'm not quite sure what this question
>> is asking, however:
>>

>> > Are we right in assuming that the current BluetoothAdapter function
>> has to
>> > be broken for each component and put in the respective structure?
>>

>> If you are asking if code like
>>
>> https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L276-L305
>> should be distributed across the various trait implementations, then the
>> answer is yes.
>>
>> Cheers,
>> Josh
>>
>> On 3/28/19 8:23 PM, Sruthi Kannan wrote:

[Josh Bowman-Matthews]

Yes - use a Cargo override to build Servo with your local modified
version of the devices crate. You can read more about overrides at
https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#overriding-dependencies
and
https://doc.rust-lang.org/cargo/reference/manifest.html#the-patch-section
and you can see where to add this at
https://github.com/servo/servo/blob/master/Cargo.toml#L17-L26 . Once
Servo builds with the modified crate, you can run the automated
bluetooth tests to verify that the behaviour is the same: `./mach
test-wpt tests/wpt/mozilla/tests/bluetooth`.

Cheers,
Josh

[Niveditha Shankar]

Hello Josh

We are trying to fix the errors we got in adapter.rs. We fixed most of
them.
The errors that are left are due to the changes we made to the init
functions.

We are not sure about the following errors, could you help us with this?

error[E0407]: method `get_modalias` is not a member of trait
`BluetoothAdapter`
--> src\adapter.rs:335:6
|
335 | / fn get_modalias(&self) -> Result<(String, u32, u32, u32),
Box<Error>> {
336 | | get_inner_and_call!(self, BluetoothAdapter, get_modalias)
337 | | }
| |_____^ not a member of trait `BluetoothAdapter`

error[E0277]: the size for values of type `(dyn adapter::BluetoothAdapter +
'static)` cannot be known at compilation time
--> src\adapter.rs:156:6
|
156 | / fn new() -> Result<BluetoothAdapter, Box<Error>> {
157 | | let adapter = try!(BluetoothAdapterEmpty::init());
158 | | Ok(BluetoothAdapter::Empty(Arc::new(adapter)))
159 | | }
| |_____^ doesn't have a size known at compile-time


-Niveditha Shankar


On Sat, Mar 30, 2019 at 1:09 PM Josh Bowman-Matthews <jo...@joshmatthews.net>
wrote:

> Yes - use a Cargo override to build Servo with your local modified
> version of the devices crate. You can read more about overrides at
>
> https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#overriding-dependencies
> and
> https://doc.rust-lang.org/cargo/reference/manifest.html#the-patch-section
> and you can see where to add this at
> https://github.com/servo/servo/blob/master/Cargo.toml#L17-L26 . Once
> Servo builds with the modified crate, you can run the automated
> bluetooth tests to verify that the behaviour is the same: `./mach
> test-wpt tests/wpt/mozilla/tests/bluetooth`.
>
> Cheers,
> Josh
>
> On 3/30/19 12:50 PM, Niveditha Shankar wrote:

> > Okay I think I get it. Thank you!
> >
> > Is there anyway I can check if the changes I make to the devices crate is
> > right?
> >
> > Our submission is on Monday and we are going to continue working with
> > Webbluetooth for our final project too. So if our implementation of our
> > adapter.rs file is right, we can go ahead and do something similar for
> the
> > rest of the enum values. It would be really helpful if there is a way to
> > check that.
> >
> >
> > Niveditha Shankar

[Josh Bowman-Matthews]

Those errors are difficult to diagnose without seeing the rest of the
code. However, given that BluetoothAdapter is supposed to be a trait
now, the second function does not look like it is using trait objects
correctly. As for the first function, the error message appears to be
describing a concrete problem that it should be possible to figure out
independently.

In general in the new trait-based code, we should not be using the
`get_inner_and_call` macro any more:
https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L188-L256

It was written to effectively perform the same work as the new traits,
so I would expect the resulting trait implementations to directly call
the appropriate platform-specific code, rather than using a macro.

[Niveditha Shankar]

Thank you , I will look into it and see if I can fix them.

We referred to the links you sent us on overriding dependancies and tried
them. We are unable to run it. We get the error

*error:* failed to resolve patches for `https://github.com/servo/devices`


Caused by:

patch for `devices` in `https://github.com/servo/devices` did not resolve
to any crates. If this is unexpected, you may wish to consult:
https://github.com/rust-lang/cargo/issues/4678

Build FAILED in 0:00:00


The path we added was,


[patch."https://github.com/servo/devices"]

devices = { path = "/Users/jayaharsha/Desktop/devices" }



-Niveditha Shankar





On Sun, Mar 31, 2019 at 5:27 PM Josh Bowman-Matthews <jo...@joshmatthews.net>
wrote:

> Those errors are difficult to diagnose without seeing the rest of the
> code. However, given that BluetoothAdapter is supposed to be a trait
> now, the second function does not look like it is using trait objects
> correctly. As for the first function, the error message appears to be
> describing a concrete problem that it should be possible to figure out
> independently.
>
> In general in the new trait-based code, we should not be using the
> `get_inner_and_call` macro any more:
>
> https://github.com/servo/devices/blob/d6c62e0873b2ec9a68c97a3370667e8b9a879cf4/src/bluetooth.rs#L188-L256
>
> It was written to effectively perform the same work as the new traits,
> so I would expect the resulting trait implementations to directly call
> the appropriate platform-specific code, rather than using a macro.
>
> On 3/31/19 3:45 PM, Niveditha Shankar wrote:

[Josh Bowman-Matthews]

The devices crate is a crates.io dependency rather than a github one,
since we publish new changes to crates.io. That means adding an entry to
the existing [patch.crates-io] category instead of creating a new
category for the github repository.

[Niveditha Shankar]

Could you please brief us a little on "Bluez(Arc<BluetoothAdapterBluez>),
Android(Arc<BluetoothAdapterAndroid>)... " so that we can get an idea of
how they should be declared in trait.

Since the implementation has to be per platform, I understand that the
function calls for say, get_id() should be direct like - bluez.get_id(). It
would be helpful if I had a better understanding of
Bluez(Arc<BluetoothAdapterBluez>).

Thank you!

Niveditha Shankar


On Sun, Mar 31, 2019 at 8:32 PM Josh Bowman-Matthews <jo...@joshmatthews.net>
wrote:

> The devices crate is a crates.io dependency rather than a github one,
> since we publish new changes to crates.io. That means adding an entry to
> the existing [patch.crates-io] category instead of creating a new
> category for the github repository.
>
> On 3/31/19 6:56 PM, Niveditha Shankar wrote:

> > Thank you , I will look into it and see if I can fix them.
> >
> > We referred to the links you sent us on overriding dependancies and tried
> > them. We are unable to run it. We get the error
> >
> > *error:* failed to resolve patches for `

> https://github.com/servo/devices` <https://github.com/servo/devices>

[Josh Bowman-Matthews]

Previously we had an enum
(https://doc.rust-lang.org/book/ch06-01-defining-an-enum.html) like this:
enum BluetoothAdapter {
Bluez(Arc<BluetoothAdapterBluez>),
Android(Arc<BluetoothAdapterAndroid>),
}

The Bluez, Android, etc. names gave us variants to refer to, with the
real type stored inside. We should now be able to define something like
this instead:

trait BluetoothAdapter {
...
}

struct Bluez(Arc<BluetoothAdapterBluez>);
impl BluetoothAdapter for Bluez {
..
}

and in methods of this BluetoothAdapter implementation, refer to
`self.0.foo` to access property `foo` on the inner BluetoothAdapterBluez
(since Arc<T> is a smart pointer that automatically dereferences to the
inner type).

Cheers,
Josh

[Niveditha Shankar]

Thanks for clarifying the doubt!
We were doing exactly this but did not know how to access the respective
properties under BluetoothAdapter. Now we get it!

-Niveditha Shankar

On Mon, Apr 1, 2019 at 10:23 AM Josh Bowman-Matthews <jo...@joshmatthews.net>
wrote:

> Previously we had an enum
> (https://doc.rust-lang.org/book/ch06-01-defining-an-enum.html) like this:
> enum BluetoothAdapter {

> Bluez(Arc<BluetoothAdapterBluez>),

> Android(Arc<BluetoothAdapterAndroid>),
> }
>
> The Bluez, Android, etc. names gave us variants to refer to, with the
> real type stored inside. We should now be able to define something like
> this instead:
>
> trait BluetoothAdapter {
> ...
> }
>
> struct Bluez(Arc<BluetoothAdapterBluez>);
> impl BluetoothAdapter for Bluez {
> ..
> }
>
> and in methods of this BluetoothAdapter implementation, refer to
> `self.0.foo` to access property `foo` on the inner BluetoothAdapterBluez
> (since Arc<T> is a smart pointer that automatically dereferences to the
> inner type).
>
> Cheers,
> Josh
>
> On 4/1/19 12:55 AM, Niveditha Shankar wrote:

[Niveditha Shankar]

Hi Josh,

We tried to run servo with our modified devices crate.

Now we are getting the following errors,

error[E0277]: the size for values of type `(dyn

device::adapter::BluetoothAdapter + 'static)` cannot be known at
compilation time

--> components/bluetooth/lib.rs:193:5

|

193 | adapter: Option<BluetoothAdapter>,

| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at
compile-time

|

= help: the trait `std::marker::Sized` is not implemented for `(dyn
device::adapter::BluetoothAdapter + 'static)`



= note: required by `std::option::Option`


/////

error[E0277]: the size for values of type `(dyn

device::adapter::BluetoothAdapter + 'static)` cannot be known at
compilation time

--> components/bluetooth/lib.rs:338:5

|

338 | / fn get_adapter(&mut self) -> BluetoothResult<BluetoothAdapter> {

339 | | match self.get_or_create_adapter() {

340 | | Some(adapter) => {

341 | | if !adapter.is_powered().unwrap_or(false) {

... |

347 | | }

348 | | }

| |_____^ doesn't have a size known at compile-time

|

error: aborting due to 4 previous errors



I tried to replace Option<BluetoothAdapter> with
Option<Box<BluetoothAdapter> which only gave out more errors.



-Niveditha Shankar

On Thu, Apr 25, 2019 at 4:53 PM Niveditha Shankar <nsha...@ncsu.edu> wrote:

> Hi Josh,
>
> We tried to run servo with our modified devices crate.
>
> Now we are getting the following errors,
>
> *error[E0277]**: the size for values of type `(dyn
> device::adapter::BluetoothAdapter + 'static)` cannot be known at
> compilation time*
>
> *--> *components/bluetooth/lib.rs:193:5
>
> *|*
>
> *193* *| * adapter: Option<BluetoothAdapter>,
>
> *| * *^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^* *doesn't have a size
> known at compile-time*
>
> *|*
>
> *= **help*: the trait `std::marker::Sized` is not implemented for
> `(dyn device::adapter::BluetoothAdapter + 'static)`
>
> *= **note*: to learn more, visit <
> <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
> https://doc.rust-lang.org/
> book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>
> *= **note*: required by `std::option::Option`
>
>
> /////
>
>
> *error[E0277]**: the size for values of type `(dyn
> device::adapter::BluetoothAdapter + 'static)` cannot be known at
> compilation time*
>
> *--> *components/bluetooth/lib.rs:338:5
>
> *|*
>
> *338* *| **/* fn get_adapter(&mut self) ->
> BluetoothResult<BluetoothAdapter> {
>
> *339* *| **|* match self.get_or_create_adapter() {
>
> *340* *| **|* Some(adapter) => {
>
> *341* *| **|* if !adapter.is_powered().unwrap_or(false) {
>
> *...* *|*
>
> *347* *| **|* }
>
> *348* *| **|* }
>
> *| **|_____^* *doesn't have a size known at compile-time*
>
> *|*
>
>
>
> *error**: aborting due to 4 previous errors*
>
>
>
> I tried to replace Option<BluetoothAdapter> with
> Option<Box<BluetoothAdapter> which only gave out more errors.
>
>
>
> -Niveditha Shankar
>
>
> On Thu, Apr 25, 2019 at 4:49 PM Niveditha Shankar <nsha...@ncsu.edu>
> wrote:
>
>> Hi Josh,
>>
>> We tried to run servo with our modified devices crate. We then realized
>> we had to make one small change in fn new() for mac and android. So I will
>> go ahead and commit that change.
>>
>>
>> Now we are getting the following errors,
>>
>> *error[E0277]**: the size for values of type `(dyn
>> device::adapter::BluetoothAdapter + 'static)` cannot be known at
>> compilation time*
>>
>> *--> *components/bluetooth/lib.rs:193:5
>>
>> *|*
>>
>> *193* *| * adapter: Option<BluetoothAdapter>,
>>
>> *| * *^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^* *doesn't have a size
>> known at compile-time*
>>
>> *|*
>>
>> *= **help*: the trait `std::marker::Sized` is not implemented for
>> `(dyn device::adapter::BluetoothAdapter + 'static)`
>>
>> *= **note*: to learn more, visit <
>> <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>> https://doc.rust-lang.org/
>> book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>>
>> *= **note*: required by `std::option::Option`
>>
>>
>> *error[E0277]**: the size for values of type `(dyn
>> device::adapter::BluetoothAdapter + 'static)` cannot be known at
>> compilation time*
>>
>> *--> *components/bluetooth/lib.rs:207:5
>>
>> *|*
>>
>> *207* *| **/* pub fn new(
>>
>> *208* *| **|* receiver: IpcReceiver<BluetoothRequest>,
>>
>> *209* *| **|* adapter: Option<BluetoothAdapter>,
>>
>> *210* *| **|* embedder_proxy: EmbedderProxy,
>>
>> *...* *|*
>>
>> *225* *| **|* }
>>
>> *226* *| **|* }
>>
>> *| **|_____^* *doesn't have a size known at compile-time*
>>
>> *|*
>>
>> *= **help*: the trait `std::marker::Sized` is not implemented for
>> `(dyn device::adapter::BluetoothAdapter + 'static)`
>>
>> *= **note*: to learn more, visit <
>> <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>> https://doc.rust-lang.org/
>> book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>>
>> *= **note*: required by `std::option::Option`
>>
>>
>> *error[E0277]**: the size for values of type `(dyn
>> device::adapter::BluetoothAdapter + 'static)` cannot be known at
>> compilation time*
>>
>> *--> *components/bluetooth/lib.rs:320:5
>>
>> *|*
>>
>> *320* *| **/* pub fn get_or_create_adapter(&mut self) ->
>> Option<BluetoothAdapter> {
>>
>> *321* *| **|* let adapter_valid = self
>>
>> *322* *| **|* .adapter
>>
>> *323* *| **|* .as_ref()
>>
>> *...* *|*
>>
>> *335* *| **|* self.adapter.clone()
>>
>> *336* *| **|* }
>>
>> *| **|_____^* *doesn't have a size known at compile-time*
>>
>> *|*
>>
>> *= **help*: the trait `std::marker::Sized` is not implemented for
>> `(dyn device::adapter::BluetoothAdapter + 'static)`
>>
>> *= **note*: to learn more, visit <
>> <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>> https://doc.rust-lang.org/
>> book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>>
>> *= **note*: required by `std::option::Option`
>>
>>
>> *error[E0277]**: the size for values of type `(dyn
>> device::adapter::BluetoothAdapter + 'static)` cannot be known at
>> compilation time*
>>
>> *--> *components/bluetooth/lib.rs:338:5
>>
>> *|*
>>
>> *338* *| **/* fn get_adapter(&mut self) ->
>> BluetoothResult<BluetoothAdapter> {
>>
>> *339* *| **|* match self.get_or_create_adapter() {
>>
>> *340* *| **|* Some(adapter) => {
>>
>> *341* *| **|* if !adapter.is_powered().unwrap_or(false) {
>>
>> *...* *|*
>>
>> *347* *| **|* }
>>
>> *348* *| **|* }
>>
>> *| **|_____^* *doesn't have a size known at compile-time*
>>
>> *|*
>>
>> *= **help*: the trait `std::marker::Sized` is not implemented for
>> `(dyn device::adapter::BluetoothAdapter + 'static)`
>>
>> *= **note*: to learn more, visit <
>> <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>> https://doc.rust-lang.org/
>> book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
>>
>> *= **note*: required by `std::result::Result`
>>
>>
>> *error**: aborting due to 4 previous errors*
>>
>>
>>
>> I tried to replace Option<BluetoothAdapter> with
>> Option<Box<BluetoothAdapter> which only gave out more errors.
>>
>>
>>
>> -Niveditha Shankar
>>
>>
>>
>>>>
>>>>