Closed Bug 1429816 Opened 6 years ago Closed 6 years ago

Update xpcom-macros to syn 0.12

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(4 files)

syn 0.12 was released earlier this week. It updates the API which is used by proc macros. We should move our custom derives in tree over to using this new interface, including xpcom-macros (being added in bug 1293362).
MozReview-Commit-ID: 5LhWjnMstdE
Attachment #8942236 - Flags: review?(nfroyd)
MozReview-Commit-ID: LKr42ccjxBr
Attachment #8942237 - Flags: review?(nfroyd)
Comment on attachment 8942236 [details] [diff] [review]
Part 1: Revendor to bring in `syn` 0.12 and dependencies

Review of attachment 8942236 [details] [diff] [review]:
-----------------------------------------------------------------

Questions below before we do this.

::: third_party/rust/quote-0.3.15/Cargo.toml
@@ +1,3 @@
> +[package]
> +name = "quote"
> +version = "0.3.15" # don't forget to update version in readme for breaking changes

Is there any way we could not have two versions of quote in the tree?

::: third_party/rust/syn-0.11.11/Cargo.toml
@@ +1,3 @@
> +[package]
> +name = "syn"
> +version = "0.11.11" # don't forget to update version in readme for breaking changes

Uh.  Is there any way we could not have two versions of syn in the tree?
Attachment #8942236 - Flags: review?(nfroyd)
Comment on attachment 8942237 [details] [diff] [review]
Part 2: Update xpcom-macros to build with syn 0.12

Review of attachment 8942237 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, sure, just need to get the vendoring details worked out.
Attachment #8942237 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Questions below before we do this.
> 
> ::: third_party/rust/quote-0.3.15/Cargo.toml
> @@ +1,3 @@
> > +[package]
> > +name = "quote"
> > +version = "0.3.15" # don't forget to update version in readme for breaking changes
> 
> Is there any way we could not have two versions of quote in the tree?
> 
> ::: third_party/rust/syn-0.11.11/Cargo.toml
> @@ +1,3 @@
> > +[package]
> > +name = "syn"
> > +version = "0.11.11" # don't forget to update version in readme for breaking changes
> 
> Uh.  Is there any way we could not have two versions of syn in the tree?

Unfortunately we currently have some custom derives in tree which still build against the old syn API. It'd be a pretty big change to replace all of them at the same time. I'm guessing the answer to that question is no.

If we really don't want it right now, I can perform the upgrade later whenever syn 0.12 ends up in tree due to a different procedural macro or dependency updating.
Flags: needinfo?(nfroyd)
Ugh, crate ecosystem.  OK.

Let's do this: I think all the syn 0.11 dependecies are coming through servo/servo (and serde, which servo also depends on?).  Let's file a servo bug to get everything onto the new release.  If that happens by the end of January, great, let's land this.  If it doesn't happen, that's unfortunate, and we can land this anyway?  (Or we can just hold off until servo *does* update; your call.)

I'm assuming here that updating the xpcom-macros crate to accommodate different revisions is not super time-consuming (or you already have code somewhere that works with both)?  If that's a bad assumption on my part, please let me know, and we can reconsider the timeline, probably just by landing everything now.
Flags: needinfo?(nfroyd)
I filed a bug on servo: https://github.com/servo/servo/issues/19786.

I figure we can hold off on landing these patches until we end up with syn 0.12 in tree for some other reason.
For the record, syn 0.12 has been in the tree for a while now, and in fact we'll probably add syn 0.13 soon. It would be great if these patches could be rebased to make xpcom_macros just use syn 0.13. Since xpcom_macros is the only thing still using 0.11, it would drop 0.11 and add 0.13 which should be a net no-op :) And then other things can start using 0.13 as well.
Comment on attachment 8966433 [details]
Bug 1429816 - Part 2: Revendor dependencies.

https://reviewboard.mozilla.org/r/235136/#review240826

::: third_party/rust/clap/Cargo.toml:77
(Diff revision 1)
>  
>  [dependencies.bitflags]
>  version = "1.0"
>  
>  [dependencies.clippy]
> -version = "~0.0.166"
> +version = "0.0.181"

Why is there a change to clap showing up here? That's weird.
Comment on attachment 8966433 [details]
Bug 1429816 - Part 2: Revendor dependencies.

https://reviewboard.mozilla.org/r/235136/#review240826

> Why is there a change to clap showing up here? That's weird.

That's... Interesting... This commit was created via `./mach vendor rust` so I guess the file changed on the crates.io side ? Is that even possible ?
Comment on attachment 8966432 [details]
Bug 1429816 - Part 1: Bump syn and quote in xpcom.

https://reviewboard.mozilla.org/r/235134/#review240966

rs=me
Attachment #8966432 - Flags: review?(nfroyd) → review+
Comment on attachment 8966433 [details]
Bug 1429816 - Part 2: Revendor dependencies.

https://reviewboard.mozilla.org/r/235136/#review240972
Attachment #8966433 - Flags: review?(nfroyd) → review+
(In reply to eijebong from comment #12)
> That's... Interesting... This commit was created via `./mach vendor rust` so
> I guess the file changed on the crates.io side ? Is that even possible ?

If I run ./mach vendor rust on a clean tree I don't see any changes to clap. If I pull your changes and run ./mach vendor rust on top of those it undoes the changes to clap (and also to the time crate). I suspect somehow your crates.io cache might be polluted?
Comment on attachment 8966432 [details]
Bug 1429816 - Part 1: Bump syn and quote in xpcom.

https://reviewboard.mozilla.org/r/235134/#review241004

Thanks! :-)

::: xpcom/rust/xpcom/xpcom_macros/src/lib.rs:238
(Diff revision 1)
> +            let value = if let Lit::Str(ref s) = attr.lit {
> +                s.value()
> +            } else {
> +                Err("Unexpected non-string value in #[refcnt]")?
> +            };
> +

nit: unnecessary newline
Attachment #8966432 - Flags: review?(nika) → review+
Comment on attachment 8966433 [details]
Bug 1429816 - Part 2: Revendor dependencies.

https://reviewboard.mozilla.org/r/235136/#review241006
Attachment #8966433 - Flags: review?(nika) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fa8c1abc6b6
Part 1: Bump syn and quote in xpcom. r=froydnj,mystor
https://hg.mozilla.org/integration/autoland/rev/8ef553153fc0
Part 2: Revendor dependencies. r=froydnj,mystor
https://hg.mozilla.org/mozilla-central/rev/5fa8c1abc6b6
https://hg.mozilla.org/mozilla-central/rev/8ef553153fc0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.