1403213 - Move the rust nsstring bindings into servo/servo

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently nsstring exists in tree in the xpcom/rust/nsstring directory. It is also used by stylo which exists in the servo/servo github repository.

Stylo is required for various reasons to build without a mozilla-central repository being present (only servo/servo), so for reasons servo/servo has a second copy of nsstring with manual modifications made called nsstring_vendor which is used by stylo.

This has caused some changes to the bindings to not be re-vendored into stylo, which has historically not caused problems, but is a really bad idea. For example when bug 1343715 was landed, I forgot to mention that the nsstring changes needed to be synchronized into nsstring_vendor, meaning that nsstring_vendor didn't get the flags changes. Luckily this didn't cause problems, but it easily could've.

A solution to this problem would be to only have a single copy of nsstring (perhaps called gecko-nsstring on crates.io to avoid confusion with apple's NSString) which is used by both mozilla-central and stylo. This crate would be published to crates.io in the most recent version currently present in mozilla-central. In m-c, we'd use a [patch] entry to redirect requests for that crate locally to our in-tree version. 

The biggest problem with this would be ensuring that a new version of nsstring is published to crates.io when necessary, but I think servo CI would start failing if we did the update, correctly bumped the version number and didn't publish the new version. We'd probably add a comment to nsstring/src/lib.rs mentioning that you must bump the version number any time the file is modified.

I can't think of any other good approaches to fix this fragmentation without either moving the canonical version of nsstring out of tree (which doesn't seem like a good solution, as the canonical version should stay with the nsstring implementation IMO), or continuing to maintain this separate vendored version.

I'd love input into this idea, and suggestions about other potential solutions to this problem.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

ni? from manish, nathan and alex to see if they have any better ideas here.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Michael Layzell [:mystor] from comment #0)
> A solution to this problem would be to only have a single copy of nsstring
> (perhaps called gecko-nsstring on crates.io to avoid confusion with apple's
> NSString) which is used by both mozilla-central and stylo. This crate would
> be published to crates.io in the most recent version currently present in
> mozilla-central. In m-c, we'd use a [patch] entry to redirect requests for
> that crate locally to our in-tree version.

Would that work correctly so as to not vendor the gecko-nsstring crate into third_party/rust/ when we do vendor Rust packages?

Nit: I have a minor preference for calling the package gecko-string or gecko-strings; we are slowly moving away from the ns-prefixing.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Would that work correctly so as to not vendor the gecko-nsstring crate into
> third_party/rust/ when we do vendor Rust packages?

That's something I don't know the answer to. Alex, do we vendor a crate which is depended on from crates.io when running cargo vendor if the crate is [patch]-ed to a local directory?

Comment 4

[Alex Crichton [:acrichto]]

Sorry I'm not familiar enough with what's ging on here to give much of an opinion, but I can answer questions at least!

No, with [patch] you shouldn't need to vendor the crate from crates.io

Comment 5

[Manish Goregaokar [:manishearth]]

your plan seems good

Comment 6

[Nika Layzell [:nika] (ni? for response)]

I've been trying to figure out what the order of operations here will be for doing changes to nsstring under this system. This is what I have so far:

1. Make the changes to xpcom/rust/nsstring
2. Build with new changes & make sure everything works locally, incl. changes to servo.
3. Push patch to bugzilla & get an r+
4. Bump version of the crate in xpcom/rust/nsstring and nsgkrust/nsgkrust-gtest's Cargo.lock (dependents will use = "*" in tree?)
5. Publish the new version of nsstring to Crates.io
6. Copy servo changes into servo/servo, and update Servo's Cargo.lock to match new nsstring version 
NOTE: Cargo.tomls in Servo will have to use "*", otherwise the autolander will vendor a copy of the new nsstring into mozilla-central when merging into autoland, which we don't want.
7. Make a PR to Servo with these changes, and wait for it to be merged. This will likely break autoland, unless servo needed no changes & still compiles with the old version.
8. Land the gecko side of the patch directly onto autoland, which will unbreak autoland. The update is now complete.

Is there any way to handle this which is less complex / doesn't require so many * dependencies etc.?

Comment 7

[Manish Goregaokar [:manishearth]]

Yeah, this sounds ... about right :|

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Michael Layzell [:mystor] from comment #0)
> Currently nsstring exists in tree in the xpcom/rust/nsstring directory. It
> is also used by stylo which exists in the servo/servo github repository.
> 
> Stylo is required for various reasons to build without a mozilla-central
> repository being present (only servo/servo), so for reasons servo/servo has
> a second copy of nsstring with manual modifications made called
> nsstring_vendor which is used by stylo.

Servo needs to be buildable without Gecko. But AFAIK the reverse is either (a) no longer true, or (b) soon will be no longer true. In which case can we just have a single copy somewhere under servo/ ?  I'd *really* like to avoid the process mentioned in comment 6.

Comment 9

[Nicholas Nethercote [inactive]]

> In which case can we just have a single copy somewhere under servo/ ?

Failing that, can we have two copies and then add a check somewhere in the build system to ensure they always have the same contents? We've used that hack in the past, it's clumsy but trivial and effective.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

This is true, but it also seems pretty gross to me that we need to have the canonical location for our rust bindings be inside of the servo/ subdirectory in order to ensure that we are able to use them within servo code. 

Not sure that I can think of a much better solution than that for handling this problem without adding a ton of overhead to every push (now the overhead will just be that if you want to make changes to our string representation, you need to land those changes across both servo and mozilla-central, which was already the case).

Nathan, how do you feel about this as an option? It'd probably take the form of a directory like servo/components/xpcom which would have the nsstring crate inside of it, as well as other crates which need to be usable across both servo and gecko code. I'd probably also add a file into xpcom/rust/nsstring.txt which explains where to find the binding code.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Michael Layzell [:mystor] from comment #10)
> This is true, but it also seems pretty gross to me that we need to have the
> canonical location for our rust bindings be inside of the servo/
> subdirectory in order to ensure that we are able to use them within servo
> code. 

It doesn't seem that gross to me. If Servo didn't exist these bindings also wouldn't exist, so storing them on the servo/ side makes as much sense to me as storing them on the Gecko side.

But even if it is gross, I strongly prefer "it's in a gross place but updating is trivial" vs. "it's in a sensible place but there's another copy on crates.io and updating is a nightmare". Our future selves will thank us! :)

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Alright, I think the active plan for now will be to move the nsstring bindings into servo/servo. With this approach the updating process will look like:

1. Write a patch to the nsstring bindings against mozilla-central, build and confirm that everything works.
2. Get the patch r+-ed against m-c
3. Extract the binding part of the patch into a patch against servo/servo, and make a PR against servo.
4. When that patch is merged into servo proper, it will merge into autoland, which may break. Immediately land m-c changes onto autoland, unbreaking it.

Once bug 1288282 is fixed, we will be able to atomically commit across mozilla-central and servo/servo, so this process will become much simpler.

Comment 13

[Nicholas Nethercote [inactive]]

> 1. Write a patch to the nsstring bindings against mozilla-central, build and
> confirm that everything works.
> 2. Get the patch r+-ed against m-c
> 3. Extract the binding part of the patch into a patch against servo/servo,
> and make a PR against servo.
> 4. When that patch is merged into servo proper, it will merge into autoland,
> which may break. Immediately land m-c changes onto autoland, unbreaking it.

This is exactly the Stylo process (https://public.etherpad-mozilla.org/p/stylo) and makes sense to me. Thanks!

Comment 14

[Nathan Froyd [:froydnj]]

Apologies for the latency in this response!

(In reply to Nika Layzell [:mystor] (busy til. Oct 17) from comment #10)
> Nathan, how do you feel about this as an option? It'd probably take the form
> of a directory like servo/components/xpcom which would have the nsstring
> crate inside of it, as well as other crates which need to be usable across
> both servo and gecko code. I'd probably also add a file into
> xpcom/rust/nsstring.txt which explains where to find the binding code.

Putting things in servo/ is probably the least gross option we have, given our setup.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Move nsstring into servo/support/gecko/nsstring

This patch moves all of nsstring into the servo subdirectory.

This is just a combined patch for review, and I'll file the servo PR once this is r+-ed.

MozReview-Commit-ID: 9pPUDb4esCT

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8918026 [details] [diff] [review]
Move nsstring into servo/support/gecko/nsstring

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

No objection in principle to this patch.

I don't think the servo people are going to be super-excited about having moz.build files in their project, so we probably want to find some way to make this work that doesn't involve adding gecko-specific files to servo/.

::: servo/support/gecko/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +TEST_DIRS += [
> +    'nsstring/gtest',

This patch doesn't have the gtest directory?

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8918026 [details] [diff] [review]
Move nsstring into servo/support/gecko/nsstring

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

r+ on plopping it inside servo/support/gecko

(as discussed previously)

Comment 18

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> I don't think the servo people are going to be super-excited about having
> moz.build files in their project, so we probably want to find some way to
> make this work that doesn't involve adding gecko-specific files to servo/.

There's no convenient directory which isn't the root in which to add the moz.build files for the gtests in servo/.

Theoretically, I could move the gtest directory into xpcom/rust/nsstring-gtest or something like that and avoid including tests in the servo tree at all? Would that be preferable to the current setup?

> This patch doesn't have the gtest directory?

If you look in the raw diff, these files were just moved and git's diff algorithm just wrote things like:

diff --git a/xpcom/rust/nsstring/gtest/Cargo.toml b/servo/support/gecko/nsstring/gtest/Cargo.toml
similarity index 100%
rename from xpcom/rust/nsstring/gtest/Cargo.toml
rename to servo/support/gecko/nsstring/gtest/Cargo.toml

So they didn't show up in splinter.

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Nika Layzell [:mystor] (busy til. Oct 17) from comment #18)
> (In reply to Nathan Froyd [:froydnj] from comment #16)
> > I don't think the servo people are going to be super-excited about having
> > moz.build files in their project, so we probably want to find some way to
> > make this work that doesn't involve adding gecko-specific files to servo/.
> 
> There's no convenient directory which isn't the root in which to add the
> moz.build files for the gtests in servo/.
> 
> Theoretically, I could move the gtest directory into
> xpcom/rust/nsstring-gtest or something like that and avoid including tests
> in the servo tree at all? Would that be preferable to the current setup?

I think so?  We can't really test nsstring-gtest anyway inside servo/servo on its own, right?  Do we have tests for nsstring that servo/servo infra can run, so we don't have the potential for a servo/servo green push on an nsstring change, only to wind up busting the gecko side of testing?

> > This patch doesn't have the gtest directory?
> 
> If you look in the raw diff, these files were just moved and git's diff
> algorithm just wrote things like:

Doh, OK.

Comment 20

[Nika Layzell [:nika] (ni? for response)]

No, unfortunately we can't run any nsstring tests on servo directly as we don't have the C++ implementation which it had to link to in tree. It's like all other gecko bindings in servo right now in that it is only tested after the servo change is landed onto auto land.

Comment 21

[Nathan Froyd [:froydnj]]

OK.  Let's move the gtest stuff outside of servo/servo, then, and call this good.

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Attachment: Move nsstring into servo/support/gecko/nsstring

This was delated because I ran into some bindgen issues, but I think they're all fixed now.

MozReview-Commit-ID: HFdQiuMnGhJ

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8919392 [details] [diff] [review]
Move nsstring into servo/support/gecko/nsstring

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

For avoidance of doubt, I'm not a reviewer in servo/servo, so you'll want to tag Manish for review on the servo/servo pull request.  Thanks for doing this!

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Attachment: Final Patch (incl servo changes)

MozReview-Commit-ID: HFdQiuMnGhJ

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74c58b336d30
Move nsstring into servo/support/gecko/nsstring, r=froydnj

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for referencing non-existing xpcom/rust/gtest/moz.build in xpcom/moz.build:

https://hg.mozilla.org/integration/autoland/rev/69998f1363be6b359dc68339892973687b4e834a
https://hg.mozilla.org/integration/autoland/rev/becff95c9e4cc38aec6cd731396d946e3c79a806

Second push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74c58b336d30628e14b8ed735ae2d6dbe6cf6bbd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=138548302&repo=autoland

2017-10-20T19:12:45.560Z] 19:12:45     INFO -  FATAL ERROR PROCESSING MOZBUILD FILE
[task 2017-10-20T19:12:45.560Z] 19:12:45     INFO -  ==============================
[task 2017-10-20T19:12:45.560Z] 19:12:45     INFO -  The error occurred while processing the following file:
[task 2017-10-20T19:12:45.560Z] 19:12:45     INFO -      /builds/worker/workspace/build/src/xpcom/moz.build
[task 2017-10-20T19:12:45.560Z] 19:12:45     INFO -  The underlying problem is we referenced a path that does not exist. That path is:
[task 2017-10-20T19:12:45.561Z] 19:12:45     INFO -      /builds/worker/workspace/build/src/xpcom/rust/gtest/moz.build
[task 2017-10-20T19:12:45.561Z] 19:12:45     INFO -  Either create the file if it needs to exist or do not reference it.
[task 2017-10-20T19:12:48.722Z] 19:12:48     INFO -  *** Fix above errors and then restart with\
[task 2017-10-20T19:12:48.722Z] 19:12:48     INFO -                 "/usr/local/bin/gmake -f client.mk build"
[task 2017-10-20T19:12:48.722Z] 19:12:48     INFO -  client.mk:384: recipe for target 'configure' failed
[task 2017-10-20T19:12:48.722Z] 19:12:48     INFO -  gmake: *** [configure] Error 1

Comment 27

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1403213 - Move nsstring into servo/support/gecko/nsstring,

Review commit: https://reviewboard.mozilla.org/r/192190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192190/

Comment 28

[Nathan Froyd [:froydnj]]

Comment on attachment 8921199 [details]
Bug 1403213 - Move nsstring into servo/support/gecko/nsstring,

https://reviewboard.mozilla.org/r/192190/#review197372

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e07ca12a3b94
Move nsstring into servo/support/gecko/nsstring, r=froydnj

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a172387463ec
Update another reference to nsstring; r=mystor

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e07ca12a3b94
https://hg.mozilla.org/mozilla-central/rev/a172387463ec

Comment 32

[Ionuț Goldan [:igoldan]]

Some slight perf improvements showed up after the builds got fixed:

== Change summary for alert #10149 (as of October 24 2017 17:54 UTC) ==

Improvements:

  3%  tp6_facebook_heavy summary linux64 opt e10s     330.94 -> 320.79
  2%  tp6_google summary linux64 pgo e10s             491.98 -> 481.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10149

Comment 33

[Nika Layzell [:nika] (ni? for response)]

I have no idea why this patch in particular would cause perf improvements - I imagine it was because of some other servo patch which was merged in at the same time.

Comment 34

[Ionuț Goldan [:igoldan]]

Also noticed these AWSY perf improvements:

== Change summary for alert #10364 (as of Tue, 24 Oct 2017 17:16:31 GMT) ==

Improvements:

  2%  Heap Unclassified summary windows10-64 pgo      46,480,797.96 -> 45,535,984.53
  2%  Heap Unclassified summary windows10-64 opt      46,479,760.05 -> 45,751,161.63

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10364