Re-add --rust-target option to replace --unstable-rust #892
Conversation
|
Since the last pull request, I removed unnecessary lint suppressions and warn when using the |
|
The failure is related to rust-lang/rust#32836. |
|
It looks like this bug is not from my code, but exists in master. I modified When running |
|
I commented out parts of the headers that are causing the failures. These tests failed when using the Rust |
|
I think the problem is for the field cannot derive Copy the behavior is not standardized. You would need to add the feature gate in the header. @fitzgen maybe fall back to generate the BingenUnion when the field cannot be Copy? |
|
I filed issue #895 for the pre-existing error |
| @@ -27,7 +26,10 @@ struct rte_ipv6_tuple { | |||
| }; | |||
| }; | |||
|
|
|||
| // TODO(tmfink) uncomment once test passes | |||
| #if 0 | |||
There was a problem hiding this comment.
Thanks for leaving them uncommented in the non-union cases so we don't lose test coverage. That is essential to landing this.
There was a problem hiding this comment.
I left these lines disabled because of issue #908, which exist in master.
|
I'm hesitant to land this without fixing #895 at the same time (or at least before the next release), as it will expose that bug to many more people than were exposed to it before. |
|
@tmfink do you think you could take a crack at #895? I expect it shouldn't be too difficult. Essentially, we need a new method on Something like this: fn can_be_rust_union(&self, ctx: &BindgenContext) {
self.fields().all(|f| f.ty().can_derive_copy(ctx))
} |
|
@fitzgen What about adding a checking of the result of item.can_derive_copy(ctx) at https://github.com/rust-lang-nursery/rust-bindgen/blob/master/src/codegen/mod.rs#L1448 It should already do this check in the analysis. |
|
The check I mean checking all the fields can derive copy |
I was actually thinking of |
Yes, this is where I was imagining the |
|
@tmfink great, thanks! Let me know when you have something for me to look at :) |
|
☔ The latest upstream changes (presumably #887) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can you squash the commits, please? This looks fine to me. Thanks! |
src/ir/comp.rs
Outdated
| pub fn can_be_rust_union(&self, ctx: &BindgenContext) -> bool { | ||
| ctx.options().rust_features().untagged_union() && | ||
| self.fields().iter().all(|f| | ||
| match f { |
There was a problem hiding this comment.
nit: Maybe match *f for consistency with the rest of the codebase.
Instead of specifying whether or not to use stable, specify the Rust release to support (one of several stable/beta releases or nightly). The `--unstable-rust` option is still accepted and implies nightly. The definitions of `RustTarget` and `RustFeatures` are created with macros. For each test that uses unions, there is a version that uses the latest stable and 1.0. This also fixes the bug where unions were generated with non-Copy fields.
tmfink
force-pushed
the
feature-832-custom-rust-target
branch
from
ab3ae47
to
e7fa289
Compare
|
@emilio I squashed the commits, but it looks like the CI job failed because of an error with the setup or possibly network: We may want to consider not spawning so many "sub-jobs". If any sub-job fails, then the main job fails. |
|
@bors-servo r+ Let's see, if we see many jobs intermittently failing, we'd need to reduce the number of them I agree (they were just bumped in #901). |
|
📌 Commit e7fa289 has been approved by |
Re-add --rust-target option to replace --unstable-rust Re-apply commit. Addresses #832 Instead of specifying whether or not to use stable, specify the Rust release to support (one of several stable/beta releases or nightly). The `--unstable-rust` option is still accepted and implies nightly. The definitions of `RustTarget` and `RustFeatures` are created with macros. For each test that uses unions, there is a version that uses the latest stable and 1.0.
|
☀️ Test successful - status-travis |
|
Note that we can restart individual jobs that fail, but I agree that isn't ideal. Still, breaking up our CI into multiple jobs shaved off more than half of our wall clock time, so I'd like to do our best to preserve it. The intermittent there didn't look like something fundamental to having multiple jobs, just that when we have multiple jobs, we have more chances to hit an extant intermittent. If my intuition is incorrect, and this failure is caused by having multiple jobs, then we should fix that (of course) and investigate if regrouping jobs helps any. "Just" a matter of time, effort, and priorities :) |
Re-apply commit. Addresses #832
Instead of specifying whether or not to use stable, specify the Rust
release to support (one of several stable/beta releases or nightly).
The
--unstable-rustoption is still accepted and implies nightly.The definitions of
RustTargetandRustFeaturesare created withmacros.
For each test that uses unions, there is a version that uses the latest stable
and 1.0.