Move to cargo workspaces and single toplevel Cargo.lock

[larsbergstrom]

This seems to work except for a couple of things I'd like feedback from @alexcrichton on:

1. ports/geckolib/Cargo.toml has a [replace] block, but that seems to be getting ignored when being built as part of a workspace. You can repro by doing ./mach build-geckolib from the root - you'll get:

[larsberg@larsberg servo]$ ./mach build-geckolib
   Compiling string_cache v0.2.20
/Users/larsberg/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/string_cache-0.2.20/src/lib.rs:15:35: 15:64 error: #[feature] may not be used on the stable release channel
/Users/larsberg/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/string_cache-0.2.20/src/lib.rs:15 #![cfg_attr(feature = "unstable", feature(unsafe_no_drop_flag))]
                                                                                                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error
error: Could not compile `string_cache`.

To learn more, run the command again with --verbose.
GeckoLib build completed in 0:00:01
[larsberg@larsberg servo]$ 

2. Is the right way to perform a cargo update still to go to each of the workspaces and perform the update there? That seems to be the only way that works - we get an error if we do it at toplevel.

3. Minor: the [root] name is webdriver_server, which seems... really weird.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @wafflespeanut: python/servo/command_base.py

[alexcrichton]

ports/geckolib/Cargo.toml has a [replace] block, but that seems to be getting ignored when being built as part of a workspace

Ah yeah [replace] only works at the workspace root, in this case the Cargo.toml at the top of the tree. That's similar to how [replace] was ignored in dependencies before.

Is the right way to perform a cargo update still to go to each of the workspaces and perform the update there?

It should be the case that cargo update anywhere in the tree should work, including the root of the workspace. I can imagine though that cargo update at the root is broken (something I forgot to take into account), but if it's buggy executing cargo update anywhere else in the tree that's very bad! Could you open an issue on this as well? (just to help me track it)

Minor: the [root] name is webdriver_server, which seems... really weird.

Hehe, indeed! That's due to this PR which guarantees that the root of a lock file is deterministically chosen. Looks like though I accidentally chose max_by_key instead of min_by_key to get the alphabetically first crate...

In any case it shouldn't matter in general hopefully!

[larsbergstrom]

Ah yeah [replace] only works at the workspace root, in this case the Cargo.toml at the top of the tree. That's similar to how [replace] was ignored in dependencies before.

Hrm, that's not going to work, as we only [replace] for the stable-targeting member (geckolib) where we specifically have an override for a package that uses nightly features. We wouldn't want to use that normally because it's slower, IIUC (cc @SimonSapin).

And that sounds fine on the other two points - thanks! :-)

[alexcrichton]

Could the [replace] be expressed with optional features in that case? Where geckolib doesn't depend on the unstable version or tells the crate to disable unstable features?

[bors-servo]

☔ The latest upstream changes (presumably #12366) made this pull request unmergeable. Please resolve the merge conflicts.

[larsbergstrom]

@SimonSapin @emilio What is the plan for string_cache? Do you think we can move the [replace]d bits of code into upstream string_cache? Or does it need to be moved in-tree for easier editing and feature compilation to switch between the geckolib behavior and servo behavior?

According to @alexcrichton, our current [replace] strategy will not work going forward with Cargo workspaces unless we do it at the toplevel.

I suppose that we could clone both versions in, do a top-level [replace], and switch between them based on feature flags, but it seems like we should just be landing the geckolib changes upstream at that point.

[emilio]

@larsbergstrom: TL;DR; I don't think it can be upstreamed without headache.

So the point of it as far as I know is to be a drop-in replacement for string_cache, just backed with Gecko types instead of Servo's.

While moving it to the main string_cache repo could be doable, no other feasible consumer would ever want to use this feature. Plus it depends on gecko_bindings, which is something we definitely need a lot more control over (and also doesn't make a lot of sense to upstream).

We could try to split this dependency, I guess, but this would be at the cost of even more unsafety and pointer casting in the geckolib side, and more maintenance burden for string_cache.

[replace] at the top level could work, though I don't know enough about how it can be conditionally replaced to know if it's going to be easy or not (I guess it's not too hard?). It might be worth to try it.

So yeah, IMO moving it into string_cache would only be a last-resort option, though I don't know what other people might think about that.

[SimonSapin]

In the current setup, this replacement is only essential for the selectors crate’s dependency on string-cache. I think a nicer solution is to remove that dependency entirely and make selectors generic over the atom type.

But with selectors as it is now, this is not possible because it has a trait like this, implemented by its users:

use string_cache::BorrowedAtom;
pub trait Element {
    // Other things…

    fn get_local_name<'a>(&'a self) -> BorrowedAtom<'a>;
}

Without the string-cache dependency, (borrowed) atoms would be associated types on some traits. But to express in a trait a generic return type BorrowedAtom<'a> with the lifetime of &'a self we’d need an associated type constructor that takes a lifetime parameter.

So we’d need either:

• Language support for associated type constructors or some other form of HKT. There’s an RFC for this that seems popular, but I don’t know how much time it will take to have all the design details figured out, be implemented, and then be stabilized.
• Or removing the BorrowedAtom<'a> optimization, and going back to &'a Atom (which involves one more pointer indirection than necessary). @bholley, @emilio, how do you feel about doing this?

[nox]

We could also just make local_name return an Atom, which wouldn't increase the reference counter of most local names since most local names in the wild correspond to static atoms.

[bholley]

BorrowedAtom isn't an optimization over &'a Atom, it's an optimization over Atom. Returning a &'a Atom isn't acceptable for the Gecko case because returning a pointer to a pointer in an unowned data structure is terrifying without the support of borrowck.

@nox makes a good point about the static atoms, but I'm assuming this was a measured optimization, so presumably it helped some how.

[bholley]

Also, we definitely want our string_cache override to live in geckolib (and depend on gecko_bindings), and I don't think we should block on new language features for this.

[nox]

I got another fancy idea.

In the trait:

type LocalName: Borrow<Atom>;

In Servo impl:

type LocalName = Atom;

In Gecko impl:

type LocalName = BorrowedAtom;

With:

pub struct BorrowedAtom(UnsafeCell<()>);

impl Borrow<Atom> for Name {
    fn borrow(&self) -> &Atom {
        /* SOME TRANSMUTE MAGIC I GOT WRONG AGAIN DISREGARD ME */
    }
}

Then in the local_name impl on the Gecko side, you just use return the underlying *mut nsIAtom transmuted as &BorrowedAtom.

[SimonSapin]

@bholley Is returning a clone()’d Atom acceptable?

I'm assuming this was a measured optimization

I’m assuming this was not measured. IMO it’s fairly idiomatic Rust to try to avoid clone() calls on principle alone, and to change a T to &T if this helps do that.

[bholley]

Given that these calls are hot (I've certainly seen them in the profiles), I'd still rather avoid cloning, because even if the reference counting is a no-op for static atoms, we still need to do FFI calls to do it in geckolib. So I guess it matters whether or not it was a measured optimization for servo. :-P

What about @nox's idea?

[nox]

@nox's idea is not even doable so forget about it.

[SimonSapin]

Another idea, inspired by rust-lang/rfcs @ #1598 (comment) and https://github.com/nikomatsakis/nll/blob/master/graph-algorithms/src/lib.rs, partially implemented at servo/rust-selectors@f283d1a:

pub trait SelectorImpl
where Self: for<'a> BorrowedNamespaceConstructor<'a> {
   // ...
}

pub trait BorrowedNamespaceConstructor<'a> {
   type BorrowedNamespace: PartialEq<string_cache::Namespace> + PartialEq;
}

pub trait Element {
    type Impl: SelectorImpl;
    // ...
    fn get_namespace<'a>(&'a self) -> <Self::Impl as BorrowedNamespaceConstructor<'a>>::BorrowedNamespace;
}

pub struct ExampleSelectorImpl;

impl<'a> BorrowedNamespaceConstructor<'a> for ExampleSelectorImpl {
    type BorrowedNamespace = string_cache::BorrowedNamespace<'a>;
}

impl SelectorImpl for ExampleSelectorImpl {
   // ...
}

[alexcrichton]

Note that if it's too onerous to push around the dependencies, it makes sense to me that projects in a workspace may have different [replace] sections, conceptually at least. In terms of how it's expressed in a lock file and actually encoded I'm not really sure how it'd work, but it's not out of the question that we could get it to work in Cargo, just very difficult most likely.

[bholley]

If we do @SimonSapin's idea, does that mean that we can move the static atom list into servo/servo? That would be super-duper awesome, because right now adding atoms to the list involves publishing an update on crates.io, which makes it very tempting to just use a dynamic atom where we should use a static one.

[SimonSapin]

@bholley There are ideas around to have sets of static atoms defined outside of the string-cache crate (servo/string-cache#22) and a PR that I am way overdue to review (servo/string-cache#136). I need to take some time to review/land/extend all this and use it across Servo dependencies.

But I think it’s all largely independent from making selectors generic over the atom types.

[larsbergstrom]

r? @nox (or anybody) for just the cargo version update. r=SimonSapin once that is done!

[Manishearth]

@bors-servo r=SimonSapin,Manishearth

[bors-servo]

📌 Commit 9230583 has been approved by SimonSapin,Manishearth

[aneeshusa]

GH won't let me leave a comment there, but does this need to be updated in ./mach fetch as well? Can CARGO_PATHS be deleted?

[larsbergstrom]

fetch is still project-specific, based on my local testing, which is why I didn't do it over there.

[bors-servo]

⌛ Testing commit 9230583 with merge 5da07a7...

[bors-servo]

💔 Test failed - linux-dev

[larsbergstrom]

Looks like two failures:

./Cargo.lock:1: duplicate versions for package "semver"
    found dependency on version 0.1.20
    but highest version is 0.2.3
    try upgrading with ./mach cargo-update -p semver:0.1.20
    The following packages depend on version 0.1.20:
        rustc_version

And not getting built to target/release/servo in some cases? Need to test that more.

[SimonSapin]

Unless the duplicate version is easy to fix let’s add an exception in tidy.

[bors-servo]

☔ The latest upstream changes (presumably #13062) made this pull request unmergeable. Please resolve the merge conflicts.

[larsbergstrom]

The main things to do here are:

1. Rebase & re-merge cargo.locks again (hoping for no version conflicts!)
2. Ensure that cargo vendor on ports/geckolib does NOT pull in the full dependency graph

[nox]

Superseded by #14381.