-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unbreak publishing #303
Unbreak publishing #303
Conversation
Sorry for breaking things and thanks for the fix. Let me know when this is ready and I'll give it a review. |
c58ad0c
to
69fda84
Compare
Cargo.toml
Outdated
@@ -42,14 +42,14 @@ sparkle = { version = "0.1", optional = true } | |||
osmesa-sys = { version = "0.1", optional = true } | |||
rwh_05 = { package = "raw-window-handle", version = "0.5.2", features = ["std"], optional = true } | |||
rwh_06 = { package = "raw-window-handle", version = "0.6.2", features = ["std"], optional = true } | |||
serial_test = "3.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does serial_test need to be added to the main packages dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is to allow the compilation of android_example. It imports the test module from the main library in android-example/rust/src/lib.rs:17
for implementation of JNI methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If serial_test
is required by the example, then shouldn't it be added to the Cargo.toml of the example crate?
Adding a mod via path =
shouldn't be affected by the Cargo.toml of the foreign crate. Not sure what I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android-example doesn't use serial_test directly. It uses the test
module of the surfman crate. It is the test module in surfman that uses the serial_test, so I don't think adding it to android-example's manifest will work.
However, I think another approach to eliminate this dependency would be to conditionally import serial_test in surfman/test.rs using cfg(not(feature = "sm-test"))
and similarly use cfg_attr(not(feature="sm-test"), serial)
on all the test methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I believe we should at the very least make the serial_test
dependency optional and move it behind a feature flag, since there is no need to pull in serial_test
when compiling servo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 5ae1ac4, I've made the use of #[serial]
conditional on not(feature = "sm-test")
and I've move serial_test back to dev-dependencies.
Co-authored-by: Josh Matthews <josh@joshmatthews.net> Co-authored-by: Mukilan Thiyagarajan <mukilan@igalia.com> Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>
I've fixed some more errors in android-example (crate-type & edition in android-example/rust/Cargo.toml). The rust compilation fails at the final link step. That might need more investigation. @jdm was there anything else you wanted to address or is this in a state to be merged? |
The #[serial] is used to serialize unit tests, but it is not needed when building with sm-test feature enabled to expose the functions to android-example client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to add a working symlink for this library but couldn't complete a build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I can look into this separately. I'll merge this PR for now to allow publishing of the crate.
Fixes I made in order to publish 0.9.5.