Skip to content
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

Make optional the usage of some unstable features #18854

Merged
merged 18 commits into from Oct 13, 2017
Merged

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Oct 12, 2017

With --no-default-features --features default-except-unstable, more crates can now be compiled on stable Rust. This will help integrate them in rustc’s regression testing and compiler performance benchmarking.


This change is Reviewable

@jdm jdm assigned nox Oct 12, 2017
@SimonSapin SimonSapin force-pushed the servo-unstable-feature branch from 8ca7c3e to 01e2f3d Oct 12, 2017
Copy link
Member

nox left a comment

I have some concerns about building individual crates with ./mach build, and at least one feature in this isn't additive.

@@ -20,7 +20,7 @@ doctest = false

[features]
gecko_like_types = []
unstable = []
bench = []

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

Breaking change, right?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 13, 2017

Author Member

Yeah, but not the first since we moved this crate in this repo and stopped caring about the version number.

@@ -9,6 +9,9 @@ publish = false
name = "layout_thread"
path = "lib.rs"

[features]
unstable = ["parking_lot/nightly"]

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

With this, what gets built if I run ./mach build -p layout_thread?

extern crate core;
use self::core::nonzero::NonZero;

pub type NonZeroU32 = NonZero<u32>;

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

Make a wrapper even in this branch.

@@ -9,6 +9,9 @@ publish = false
name = "compositing"
path = "lib.rs"

[features]
unstable = ["nonzero/unstable"]

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

What gets built if I run ./mach build -p compositing?

@@ -19,10 +19,14 @@ mod imp {
use self::core::nonzero::NonZero;

pub type NonZeroU32 = NonZero<u32>;
pub type NonZeroUsize = NonZero<usize>;

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

No, make a wrapper.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 13, 2017

Author Member

Yes, I did that in a later commit.

@@ -10,6 +10,9 @@ publish = false
name = "gfx"
path = "lib.rs"

[features]
unstable = ["simd"]

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

What gets built if I run ./mach build -p gfx?

@@ -9,6 +9,9 @@ publish = false
name = "profile"
path = "lib.rs"

[features]
unstable = []

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

What gets built if I run ./mach build -p profile?

#[cfg(not(feature = "unstable"))]
/// The joint session future is the merge of the session future of every
/// browsing_context, sorted chronologically.
fn joint_session_future<'a>(&'a self, top_level_browsing_context_id: TopLevelBrowsingContextId)

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

This is incorrect, features are supposed to be additive and this is not.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 13, 2017

Author Member

Features being additive is important when one library might be used in the same projects by multiple crate maintained by different people. But the constellation crate is not intended to be used outside of this repository. We’ve never worried about backward compat within components/*.

use euclid::Size2D;
use nonzero::NonZeroU32;

This comment has been minimized.

Copy link
@nox

nox Oct 13, 2017

Member

Please squash this commit ("Make tidy happy") with the rest of the changes.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 13, 2017

Author Member

Done.

@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

Regarding ./mach build -p gfx etc, there’s a bug in Cargo: rust-lang/cargo#4463

@nox
Copy link
Member

nox commented Oct 13, 2017

I'm not sure how that Cargo bug relates, given its spurious rebuilds occur even with the same features selected. My issue is that ./mach build will build Servo with unstable features because the top-level crate uses them, whereas ./mach build -p gfx without explicitly passing a --features argument will select a different set of features.

@SimonSapin SimonSapin force-pushed the servo-unstable-feature branch from 01e2f3d to 11bd81e Oct 13, 2017
@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

My issue is that ./mach build will build Servo with unstable features because the top-level crate uses them, whereas ./mach build -p gfx without explicitly passing a --features argument will select a different set of features.

Yes, because of that bug. And fixing that bug in Cargo will fix that problem. In the meantime I don’t know what to do about it. Adding default = […] and default-features = false everywhere doesn’t seem great.

@nox
Copy link
Member

nox commented Oct 13, 2017

Yes, because of that bug. And fixing that bug in Cargo will fix that problem.

I don't understand how this is considered a bug. Why should the workspace itself decide which features get used? That wouldn't even work for Servo, where Servo and geckolib don't use the same variants for all the crates.

@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

If I understand correctly, feature selection would be based on which top-level crate is being compiled, which itself is based on --manifest-path or the current directory. ./mach build changes into ports/servo before running Cargo.

@nox
Copy link
Member

nox commented Oct 13, 2017

So there would still be an issue, where building gfx directly wouldn't build the same gfx as the one that is built when running ./mach build. The Cargo issue seems to be about entirely wrong compiles, my concern is about rebuilding gfx for no reason.

@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

With the bug fixed, cargo build --manifest-path ports/servo/Cargo.toml -p style would enable the style/servo feature and cargo build --manifest-path ports/geckolib/Cargo.toml -p style would enable the style/gecko feature.

@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

@bors-servo r=nox (per IRC)

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

📌 Commit 11bd81e has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

Testing commit 11bd81e with merge ab761a3...

bors-servo added a commit that referenced this pull request Oct 13, 2017
Make optional the usage of some unstable features

With `--no-default-features --features default-except-unstable`, more crates can now be compiled on stable Rust. This will help integrate them in rustc’s regression testing and compiler performance benchmarking.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18854)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

💔 Test failed - mac-rel-wpt4

@jdm
Copy link
Member

jdm commented Oct 13, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

Testing commit 11bd81e with merge 8b3d553...

bors-servo added a commit that referenced this pull request Oct 13, 2017
Make optional the usage of some unstable features

With `--no-default-features --features default-except-unstable`, more crates can now be compiled on stable Rust. This will help integrate them in rustc’s regression testing and compiler performance benchmarking.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18854)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

💔 Test failed - linux-rel-css

@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

TIMEOUT [expected FAIL] /selectors-3_dev/xhtml1print/grid-first-letter-003.xht

  ▶ TIMEOUT [expected FAIL] /selectors-3_dev/xhtml1print/grid-first-letter-003.xht
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 17.2.0-devel
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 17.2.0-devel
  │ called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 32, message: "Broken pipe" } }) (thread FontCacheThread, at /checkout/src/libcore/result.rs:906)
  │ stack backtrace:
  │    0:     0x7fb5c97b83dc - backtrace::backtrace::trace::hec7ae591db5c7608
  │    1:     0x7fb5c97b8412 - backtrace::capture::Backtrace::new::hb67fc1d62ccb892b
  │    2:     0x7fb5c74f5505 - servo::main::{{closure}}::had3fb0051711e302
  │    3:     0x7fb5c98163b6 - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:578
  │    4:     0x7fb5c98161d4 - std::panicking::begin_panic<alloc::string::String>
  │                         at /checkout/src/libstd/panicking.rs:538
  │    5:     0x7fb5c9816149 - std::panicking::begin_panic_fmt
  │                         at /checkout/src/libstd/panicking.rs:522
  │    6:     0x7fb5c98160da - std::panicking::rust_begin_panic
  │                         at /checkout/src/libstd/panicking.rs:498
  │    7:     0x7fb5c98502d0 - core::panicking::panic_fmt
  │                         at /checkout/src/libcore/panicking.rs:71
  │    8:     0x7fb5c94751d1 - core::result::unwrap_failed::hf4602dd5e22e9990
  │    9:     0x7fb5c9477d68 - webrender_api::api::RenderApi::update_resources::h189284108e52c764
  │   10:     0x7fb5c8ee861a - gfx::font_cache_thread::FontCache::run::h17098598ad9cc0da
  │   11:     0x7fb5c8ecb378 - std::sys_common::backtrace::__rust_begin_short_backtrace::h754c10d8ab0fc1f3
  │   12:     0x7fb5c8ecbba4 - std::panicking::try::do_call::hf2c4cb937ecc7ffc
  │   13:     0x7fb5c981d4fc - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:99
  │   14:     0x7fb5c8ed5f2d - <F as alloc::boxed::FnBox<A>>::call_box::hacfdd34c8d32b1ed
  │   15:     0x7fb5c98150cb - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:772
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:24
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:90
  │   16:     0x7fb5c51ab183 - start_thread
  │   17:     0x7fb5c4cc1ffc - clone
  │   18:                0x0 - <unknown>
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: Io(Error { repr: Os { code: 32, message: "Broken pipe" } })
  │ failed to receive response to font request: Io(Error { repr: Os { code: 104, message: "Connection reset by peer" } }) (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZeroU32(NonZero(1))) }, at /checkout/src/libcore/result.rs:906)
  │ stack backtrace:
  │    0:     0x7fb5c97b83dc - backtrace::backtrace::trace::hec7ae591db5c7608
  │    1:     0x7fb5c97b8412 - backtrace::capture::Backtrace::new::hb67fc1d62ccb892b
  │    2:     0x7fb5c74f5505 - servo::main::{{closure}}::had3fb0051711e302
  │    3:     0x7fb5c98163b6 - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:578
  │    4:     0x7fb5c98161d4 - std::panicking::begin_panic<alloc::string::String>
  │                         at /checkout/src/libstd/panicking.rs:538
  │    5:     0x7fb5c9816149 - std::panicking::begin_panic_fmt
  │                         at /checkout/src/libstd/panicking.rs:522
  │    6:     0x7fb5c98160da - std::panicking::rust_begin_panic
  │                         at /checkout/src/libstd/panicking.rs:498
  │    7:     0x7fb5c98502d0 - core::panicking::panic_fmt
  │                         at /checkout/src/libcore/panicking.rs:71
  │    8:     0x7fb5c8ed5ba1 - core::result::unwrap_failed::he7b7c5c12b9b8432
  │    9:     0x7fb5c8ef27c4 - gfx::font_cache_thread::FontCacheThread::get_font_instance::h81efb3a1f9a2d72a
  │   10:     0x7fb5c8ef3127 - gfx::font_context::FontContext::create_layout_font::h7cafe11e319cc768
  │   11:     0x7fb5c8ef3dfc - gfx::font_context::FontContext::layout_font_group_for_style::hcfb66eb349aedc42
  │   12:     0x7fb5c780f741 - layout::text::TextRunScanner::scan_for_runs::h534e004494cb0df8
  │   13:     0x7fb5c7723612 - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_flow_for_block_starting_with_fragments::ha215179782ccf9e6
  │   14:     0x7fb5c77220f4 - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_flow_for_block_like::h1dc7baa0fa39d43a
  │   15:     0x7fb5c77216ed - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode>>::build_flow_for_block::hcb366551687a0fd0
  │   16:     0x7fb5c7698af2 - <layout::construct::FlowConstructor<'a, ConcreteThreadSafeLayoutNode> as layout::traversal::PostorderNodeMutTraversal<ConcreteThreadSafeLayoutNode>>::process::he1ba8292623b5232
  │   17:     0x7fb5c76f5321 - style::traversal::DomTraversal::handle_postorder_traversal::h9a9f086d232794d8
  │   18:     0x7fb5c775e087 - layout_thread::LayoutThread::handle_reflow::h08e10bdb7bc88478
  │   19:     0x7fb5c774fa94 - layout_thread::LayoutThread::handle_request_helper::ha966420573ae5b4f
  │   20:     0x7fb5c774b390 - <layout_thread::LayoutThread as layout_traits::LayoutThreadFactory>::create::{{closure}}::h259474fca527c891
  │   21:     0x7fb5c76ae3b5 - std::sys_common::backtrace::__rust_begin_short_backtrace::h91d5b316355deb44
  │   22:     0x7fb5c76b1f35 - std::panicking::try::do_call::h7b171f4e959dd588
  │   23:     0x7fb5c981d4fc - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:99
  │   24:     0x7fb5c76e19d2 - <F as alloc::boxed::FnBox<A>>::call_box::hae49c8c84b363d3b
  │   25:     0x7fb5c98150cb - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:772
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:24
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:90
  │   26:     0x7fb5c51ab183 - start_thread
  │   27:     0x7fb5c4cc1ffc - clone
  │   28:                0x0 - <unknown>
  │ ERROR:servo: failed to receive response to font request: Io(Error { repr: Os { code: 104, message: "Connection reset by peer" } })
  │ assertion failed: !self.Document().needs_reflow() ||
  │     (!for_display && self.Document().needs_paint()) ||
  │     self.window_size.get().is_none() || self.suppress_reflow.get() (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZeroU32(NonZero(1))) }, at /home/servo/buildbot/slave/linux-rel-css/build/components/script/dom/window.rs:1338)
  │ stack backtrace:
  │    0:     0x7fb5c97b83dc - backtrace::backtrace::trace::hec7ae591db5c7608
  │    1:     0x7fb5c97b8412 - backtrace::capture::Backtrace::new::hb67fc1d62ccb892b
  │    2:     0x7fb5c74f5505 - servo::main::{{closure}}::had3fb0051711e302
  │    3:     0x7fb5c98163b6 - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:578
  │    4:     0x7fb5c78a35ba - std::panicking::begin_panic::h39d93994ecc53727
  │    5:     0x7fb5c80294a7 - script::dom::window::Window::reflow::h7f6f2ec853c835b7
  │    6:     0x7fb5c7e45db7 - script::dom::document::Document::finish_load::h1be96fb44e771c54
  │    7:     0x7fb5c7f9a4a1 - script::dom::servoparser::ServoParser::do_parse_sync::h3e4c46deccfaa80c
  │    8:     0x7fb5c7f99c36 - script::dom::servoparser::ServoParser::parse_sync::hc140f2be00400af4
  │    9:     0x7fb5c7f9eb0b - <script::dom::servoparser::ParserContext as net_traits::FetchResponseListener>::process_response_eof::h72a45879ad646db9
  │   10:     0x7fb5c807ae58 - script::script_thread::ScriptThread::handle_msg_from_constellation::hbd0c6bb93f5c9ecc
  │   11:     0x7fb5c8074eaf - script::script_thread::ScriptThread::handle_msgs::{{closure}}::hf1ef8951038742c5
  │   12:     0x7fb5c8070c64 - script::script_thread::ScriptThread::handle_msgs::hc7dc8d41f6ae010d
  │   13:     0x7fb5c806c26b - script::script_thread::ScriptThread::start::h4462f3086dc3d480
  │   14:     0x7fb5c789ad22 - std::sys_common::backtrace::__rust_begin_short_backtrace::h827bbdc342243b9b
  │   15:     0x7fb5c7974495 - std::panicking::try::do_call::h614c0e1beac71ffd
  │   16:     0x7fb5c981d4fc - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:99
  │   17:     0x7fb5c7bf5742 - <F as alloc::boxed::FnBox<A>>::call_box::h61a9f1b3aae17011
  │   18:     0x7fb5c98150cb - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:772
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:24
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:90
  │   19:     0x7fb5c51ab183 - start_thread
  │   20:     0x7fb5c4cc1ffc - clone
  │   21:                0x0 - <unknown>
  │ ERROR:servo: assertion failed: !self.Document().needs_reflow() ||
  │     (!for_display && self.Document().needs_paint()) ||
  └     self.window_size.get().is_none() || self.suppress_reflow.get()
@SimonSapin
Copy link
Member Author

SimonSapin commented Oct 13, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

Testing commit 11bd81e with merge 78aaa85...

bors-servo added a commit that referenced this pull request Oct 13, 2017
Make optional the usage of some unstable features

With `--no-default-features --features default-except-unstable`, more crates can now be compiled on stable Rust. This will help integrate them in rustc’s regression testing and compiler performance benchmarking.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18854)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

@bors-servo bors-servo mentioned this pull request Oct 13, 2017
3 of 5 tasks complete
@bors-servo bors-servo merged commit 11bd81e into master Oct 13, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the servo-unstable-feature branch Oct 13, 2017
@SimonSapin SimonSapin mentioned this pull request Oct 13, 2017
90 of 99 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.