$ ./mach build [...] /usr/local/bin/rustc xpcom/rust/nsstring/src/lib.rs --color always --crate-name nsstring --crate-type lib -C opt-level=2 -C panic=abort -C codegen-units=1 -g -C metadata=c5e833dfcf49fca4 --out-dir objdir/toolkit/library/gtest/rust/./x86_64-unknown-freebsd/release/deps --emit=dep-info,link --target x86_64-unknown-freebsd -L dependency=objdir/toolkit/library/gtest/rust/./x86_64-unknown-freebsd/release/deps error: implementing Drop adds hidden state to types, possibly conflicting with `#[repr(C)]` --> xpcom/rust/nsstring/src/lib.rs:604:1 | 604 | impl<'a> Drop for nsString<'a> { | ^ | note: lint level defined here --> xpcom/rust/nsstring/src/lib.rs:106:9 | 106 | #![deny(warnings)] | ^^^^^^^^ note: the `#[repr(C)]` attribute is attached here --> xpcom/rust/nsstring/src/lib.rs:214:9 | 214 | pub struct $String<'a> { | ^ xpcom/rust/nsstring/src/lib.rs:568:1: 574:2 note: in this expansion of define_string_types! (defined in xpcom/rust/nsstring/src/lib.rs) error: implementing Drop adds hidden state to types, possibly conflicting with `#[repr(C)]` --> xpcom/rust/nsstring/src/lib.rs:504:1 | 504 | impl<'a> Drop for nsCString<'a> { | ^ | note: lint level defined here --> xpcom/rust/nsstring/src/lib.rs:106:9 | 106 | #![deny(warnings)] | ^^^^^^^^ note: the `#[repr(C)]` attribute is attached here --> xpcom/rust/nsstring/src/lib.rs:214:9 | 214 | pub struct $String<'a> { | ^ xpcom/rust/nsstring/src/lib.rs:464:1: 470:2 note: in this expansion of define_string_types! (defined in xpcom/rust/nsstring/src/lib.rs) error: aborting due to 2 previous errors

Michael, can you bump minimum Rust version in build/moz.configure/rust.configure ?

I thought that we weren't going to land that Drop bits until we had made the required changes...

This is my bad, I incorrectly assumed that bug 1318531 included bumping the minimum allowed rustc version (/me should make sure to read the patches :P). Would it be better to fix this by bumping the minimum allowed version, or backing out bug 1316698? This patch bumps the minimum rustc version, but I'm also OK with backing out the changes until 1.14 or so.

Comment on attachment 8814772 [details] [diff] [review] Update rust requirement to 1.13 Review of attachment 8814772 [details] [diff] [review]: ----------------------------------------------------------------- I would rather back out the nsstring changes.

I backed out Bug 1316696 which I think should fix this bug.

Great! Please go ahead and land this though. We need 1.13 for the next update of the mp4 parser as well.

Comment on attachment 8814772 [details] [diff] [review] Update rust requirement to 1.13 From rillian's comment it seems like we're going to want this soon anyways for the mp4 stuff, so I figure I'll mark this patch for review and once it lands I can reland the nsstring changes.

(In reply to Ralph Giles (:rillian) needinfo me from comment #6) > Great! Please go ahead and land this though. We need 1.13 for the next > update of the mp4 parser as well. Is that "need" a "we did things that we can't possibly do in 1.12" or a "we used the shiniest features we could"? When we you aiming to have the mp4 parser update landed? I was really hoping we could hold off a version bump until 1.14 came out, rather than requiring 1.13 and then 1.14 three weeks later.

(In reply to Nathan Froyd [:froydnj] from comment #8) > Is that "need" a "we did things that we can't possibly do in 1.12" or a "we > used the shiniest features we could"? When we you aiming to have the mp4 > parser update landed? I was really hoping we could hold off a version bump > until 1.14 came out, rather than requiring 1.13 and then 1.14 three weeks > later. We replaced all the `try!()` macros with the `?` operator. Undoing that would take time, and waiting until 1.14 is stable gives us less time to test the mp4 parser update on Nightly. I guess that's "shiniest features" but I think both that and the no-inline-drop improvements in nsstring are worth upgrading for. Why is delaying the bump better?

(In reply to Ralph Giles (:rillian) needinfo me from comment #9) > (In reply to Nathan Froyd [:froydnj] from comment #8) > > > Is that "need" a "we did things that we can't possibly do in 1.12" or a "we > > used the shiniest features we could"? When we you aiming to have the mp4 > > parser update landed? I was really hoping we could hold off a version bump > > until 1.14 came out, rather than requiring 1.13 and then 1.14 three weeks > > later. > > We replaced all the `try!()` macros with the `?` operator. Undoing that > would take time, and waiting until 1.14 is stable gives us less time to test > the mp4 parser update on Nightly. It seems like it would have been better to update the parser in-tree and then do the breaking changes, but meh. > I guess that's "shiniest features" but I think both that and the > no-inline-drop improvements in nsstring are worth upgrading for. Why is > delaying the bump better? We don't have anything using no-inline-drop (or `?`) in-tree right now, and continually upgrading Rust will just frustrate people who are trying to get their work done. It's also good practice for when we really require Rust, because we definitely won't be upgrading every six weeks then. ;)

Comment on attachment 8814772 [details] [diff] [review] Update rust requirement to 1.13 Review of attachment 8814772 [details] [diff] [review]: ----------------------------------------------------------------- Is there a pressing reason to get the nsstring Drop bits in? Otherwise, it seems better to wait for 1.14, which is coming out in a few short weeks.

Comment on attachment 8814772 [details] [diff] [review] Update rust requirement to 1.13 Review of attachment 8814772 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review to encourage Michael to respond to previous review. ;)

(In reply to Nathan Froyd [:froydnj] from comment #11) > Comment on attachment 8814772 [details] [diff] [review] > Update rust requirement to 1.13 > > Review of attachment 8814772 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is there a pressing reason to get the nsstring Drop bits in? Otherwise, it > seems better to wait for 1.14, which is coming out in a few short weeks. There is no pressing reason other than the new code is nicer than the old code. Nobody in-tree uses nsstring fields yet, the only consumers AFAIK are in stylo, and potentially some of the stuff which qDot is working on. I think the timelines for both of those use cases are long enough that we _could_ wait for 1.14.

We seem to already require newer Rust to build, so this can be closed, right?

Yup closing. In addition the changes which I was making which made it unable to build have already landed in tree as well :).