Move the rust nsstring bindings into servo/servo

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
ni? from manish, nathan and alex to see if they have any better ideas here.
Flags: needinfo?(nfroyd)
Flags: needinfo?(manishearth)
Flags: needinfo?(acrichton)
(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.
Flags: needinfo?(nfroyd)
(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?
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
Flags: needinfo?(acrichton)
your plan seems good
Flags: needinfo?(manishearth)
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.?
Flags: needinfo?(manishearth)
Yeah, this sounds ... about right :|
Flags: needinfo?(manishearth)
(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.
Flags: needinfo?(michael)
> 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.
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.
Flags: needinfo?(michael) → needinfo?(nfroyd)
(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! :)
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.
Assignee: nobody → nika
Summary: Consider putting nsstring on crates.io → Move the rust nsstring bindings into servo/servo
> 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!
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.
Flags: needinfo?(nfroyd)
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
Attachment #8918026 - Flags: review?(nfroyd)
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?
Attachment #8918026 - Flags: review?(nfroyd) → feedback+
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)
Attachment #8918026 - Flags: review+
(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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
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.
OK.  Let's move the gtest stuff outside of servo/servo, then, and call this good.
This was delated because I ran into some bindgen issues, but I think they're all fixed now.

MozReview-Commit-ID: HFdQiuMnGhJ
Attachment #8919392 - Flags: review?(nfroyd)
Attachment #8918026 - Attachment is obsolete: true
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!
Attachment #8919392 - Flags: review?(nfroyd) → review+
MozReview-Commit-ID: HFdQiuMnGhJ
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/74c58b336d30
Move nsstring into servo/support/gecko/nsstring, r=froydnj
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
Flags: needinfo?(nika)
Attachment #8921199 - Flags: review?(nfroyd) → review+
Comment on attachment 8921199 [details]
Bug 1403213 - Move nsstring into servo/support/gecko/nsstring,

https://reviewboard.mozilla.org/r/192190/#review197372
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/autoland/rev/e07ca12a3b94
Move nsstring into servo/support/gecko/nsstring, r=froydnj
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a172387463ec
Update another reference to nsstring; r=mystor
https://hg.mozilla.org/mozilla-central/rev/e07ca12a3b94
https://hg.mozilla.org/mozilla-central/rev/a172387463ec
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Flags: needinfo?(nika)
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.
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
You need to log in before you can comment on or make changes to this bug.