Consider handing the crates to https://github.com/facebookexperimental/starlark-rust/

[damienmg]

cc @stepancheg @illicitonion

Facebook released their own fork on this repository at https://github.com/facebookexperimental/starlark-rust/. Seeing the low support we can offer, I am for handling them over the crates and making this repository a redirection to the facebook one.

I am opening this bug to hear a bit more your thoughts about it. I haven't look too deep in the code yet to assess exactly what / how this would look like but from preliminary request I did some time ago there is no legal blocker on the google side.

Let me know if you have big thoughts on this.

Also ccing @ndmitchell

[ndmitchell]

Thanks @damienmg - I'm very happy to answer any questions you have, and even more happy to have any of you contributing to starlark-rust at the new location :)

[illicitonion]

Having not read the changes on the fork in any detail, this sounds reasonable - it's exciting to see a lot of active development going on (and @ndmitchell's discussions on the main starlark repo have been great to follow).

The compatibility notes are interesting, and it's great to see the extensions are all en/dis-ableable :)

My handful of concerns are:

• Will it make it harder to accept contributions from outside Facebook, and consume as a library from outside Facebook? It looks like there's an "fbshipit" process that's gone through, but there are no examples of pull requests on GitHub - would contributions still be welcome, and as trivial as making a PR? Would CI run on those PRs, and be the source of truth for tests that need to pass for a PR? (One of the issues folks have had contributing to Bazel in general is being told "Your tests fail some internal CI, we can't give you more details"). Will turnaround times for changes realistically stay similar? Are there any aims at stability guarantees, semver compatibility, and releases, or is this more of a "develop at head, pick a snapshot" kind of model? I see some magic "@oss-enable" and "@oss-disable" comments that are slightly concerning in that regard, as I don't know what they do...
• It'd be nice if the git history included all of the commits from this repo (and then maybe an opaque "Here's a bunch of changes we didn't clean up for open source consumption" commit), rather than squashing all of this repo's changes into one...
• It's a little bit of a shame that it's nightly-only, but that's I'm sure fixable if it becomes a real problem for anyone :)

[ndmitchell]

Will it make it harder to accept contributions from outside Facebook, and consume as a library from outside Facebook?

Fractionally, yes, but I hope the fraction is small enough to be unmeasurable. To answer your concerns:

• You are welcome to submit a PR. You must sign a FB CLA (just like a Google one for this repo). From that it can be merged. Normal workflow, and there is a public GitHub CI.
• That PR gets mirrored internally, and runs through our CI infra in addition to the GitHub actions - so there's a chance there will be an internal failure that doesn't show up externally - and if there is I'll give you enough detail to know what is going on. However, I consider any failure in our internal code that doesn't also show up as a single specific test case in the Starlark repo to be a bug in Starlark - because it vastly hampers my ability to develop Starlark quickly.
• With that said, there is one specific issue I'm aware of - we use the unreleased Rust fmt 2.0 internally, and the released version is 1.0, and they sometimes disagree. That might be annoying, but once Rust fmt 2.0 comes out, will go away. And I'm happy to take a diff which hasn't had formatting run, then after add a diff on top that does the formatting.
• Turn around times are based on the humans hitting accept. There is an oncall rota, and I aim to comment on all diffs within 24 hours. Also see https://github.com/ndmitchell/neil#contributions for my personal contribution guidelines, which don't apply to this repo, but should give you a sense of what I personally aim for, and here, I'll also be able to use work time for it.
• Internally, we don't care about releases, they're a service for the users - and if you need them, we're not against making them on some schedule or when things change such that you do want them.
• I will do semver compat, but mostly by bumping the 0.1 each release. I don't want to break code more than necessary, but equally, given the stability and audience of this project, I'm happy to break things to make it 1% better. I imagine the threshold will keep growing over time. I did give the API a thorough audit before we made it open source.
• The magic oss-enable and disable are so that for internal development we use HEAD of Gazebo (a Rust library we also maintain) rather than the release. That's it. Everything else you are seeing exactly what we have.

It'd be nice if the git history included all of the commits from this repo

Agreed! Legal and policy restrictions made that impossible. Sorry :(

It's a little bit of a shame that it's nightly-only, but that's I'm sure fixable if it becomes a real problem for anyone :)

Agreed - I think the benefits of trait alias and box syntax are worth the cost of being nightly. Once those two stabilise, I hope to go back to stable.

[ndmitchell]

Everything else you are seeing exactly what we have

Actually, not quite true, there's an internal linter which I can't release yet (mostly because it uses a boat load of unsupported Rust features, which is just not supportable in the outside world). It provides about 5 lints, with suggestions like using features of Gazebo such as dupe() over clone(). Should be minor, should rarely hit your code, and I'll mirror all lints - but trying to be completely forthcoming so you can make an informed decision, and not get surprised after the fact.

[illicitonion]

Thanks @ndmitchell, that all sounds great: +1 from me - excited to see where things go!

I'm not aware of anyone currently relying on releases, but claiming each release is incompatible feels reasonable.

[Mythra]

FWIW Saw this pop up, but am depending on releases and nightly is unfortunately not an option for me 😞 So if we were to switch from google to the FB fork, I'd have that concern. If we do plan on switching would it be possible to get a heads up so I can fork, and prepare to maintain that myself?

[ndmitchell]

@Mythra it would be sad if you had to maintain a separate version just for that. Are you able to share what is behind your requirement to stick to stable Rust? (Either here, or feel free to ping me and we chat chat over video or private email.) Is using stable rust but setting RUSTC_BOOTSTRAP=1 an option?

To go into more detail, we currently use unstable for 7 features. But if we had to, we could easily remove 3, and put one behind a flag. That gives us:

• trait_alias - these are very helpful to keep complexity down. Removing them means we have to expand a pretty complex trait patterns at a fair number of call sites. Yuk. But, not impossible.
• box_pattern and box_syntax - there are 187 instances of these. For construction, it's more complex, especially in the way we create closures. For deconstruction, it probably means additional match clauses.

So moving back to stable definitely has a cost, both in terms of initial work and then in ongoing maintenance. But, it's a cost we would pay for someone who had an actual and pressing need.

[ndmitchell]

I've just removed trait_alias and iter_order_by (the patches will show up in the repo probably tomorrow). That means that 187 instances of Box are the remaining real issue to nightly (everything else is used about once, and easy to remove, at some very small cost of complexity).

[Mythra]

Hey,

Sorry about not commenting yesterday had a bunch going on.

To help show our current situation the main reason for us using nightly is we're an incredibly small team, and we've been bitten by nightly in the past. Although rust nightly is generally very reliable, there's time where it hasn't been, and we haven't had the ability to change what we've needed to change without blocking something important. That's our main reason for wanting to stay on stable.

• I've never looked into RUSTC_BOOTSTRAP I can take a look, but building our own compiler (which it seems it what bootstrapping is?) seems much more tedious than we can support.

I definitely don't want to hold up your development though either just for our small team. Seems unfair to y'all. Especially since we've done some things that we were planning to contribute up, but are relatively tied to how this crate works. Almost all related to diagnostics:

As the case above shows, being able to zero into a specific argument causing the problem. and highlight just that. Even for our custom type. It's definitely an awkward situation either way I feel, I just wanna make sure we don't hold y'all back either. Happy to work with what we can do though to make it the best situation for everyone.

[ndmitchell]

No worries about timing - there's no rush on this stuff.

To go into nightly vs stable, there are two things you could do:

• Pick the nightly that corresponds to the date of the last stable. We actually do that, and it's worked out well for us.
• For RUSTC_BOOTSTRAP the stated purpose is to allow bootstrapping. But, if you just set that variable, then a normal binary compiler you download and do not bootstrap works just fine. It's really a magic "allow features" flag, but people are encouraged not to use it unless they are bootstrapping. But I find it very handy.

As for improvements around parameter validation, that sounds fantastic! I have some idea for how to do value "parsing", that I think might make such things easier, and the better error messages we can do the more helpful it is. It sounds like you are going down the same kind of approach.

[froydnj]

* For `RUSTC_BOOTSTRAP` the stated purpose is to allow bootstrapping. But, if you just set that variable, then a normal binary compiler you download and do not bootstrap works just fine. It's really a magic "allow features" flag, but people are encouraged not to use it unless they are bootstrapping. But I find it very handy.

Could RUSTC_BOOTSTRAP just be set in starlark-rust's build.rs? That's hacky and gross, but there is some precedent for doing this (e.g. crates that want real SIMD support). I guess this works only so long as nightly-only things aren't exposed in the crate interface, but from the discussion above, it sounds like that would be the case?

[ndmitchell]

I would have hoped the Rust community came down hard on people setting RUSTC_BOOTSTRAP in the build.rs. It feels pretty grim, and something that users opting into is OK, but packages opting their users into for that package is cheating. But if that's the consensus, and the Rust community doesn't flame me, I'm game. I think we'd be fine with a stable-only interface (perhaps the exception backtrace would need to go, but the only reason I keep it is that I don't understand what it does!)

[jsgf]

I would have hoped the Rust community came down hard on people setting RUSTC_BOOTSTRAP

rust-lang/cargo#7088 for some discussion on this.

[ndmitchell]

Thanks @jsgf - so it seems it is discouraged, but that it will probably still be possible for users to specify it on a crate-by-crate basis.

But I've thought of another alternative. Perhaps we could eliminate the box keyword on a branch just for release. It would mean that releases were a bit of a pain, and probably force us to branch for each release, switch back to stable compatible, then maintain a long running 0.4.x branch with only minimal backporting. But it gives everyone what they want, and keeps the code on master nice and clean. When box does get stabilised, we can then converge once more.

[jsgf]

My impression from talking to @dtolnay is that box in its current form is unlikely to ever be stabilized.

[ndmitchell]

That's a shame. I also find it remarkably hard to get those kind of viewpoints and the reasoning behind them from the GitHub tickets etc associated with proposals...

For box, we really use it for two things:

1. To create closures for interpretation, with box move |ctx| .... Doing them with Box::new() is a more verbose in places the verbosity is most unwelcome. But I plan to move to a different machine architecture, which won't use closure interpretation, and thus I expect the number of such instances will decrease dramatically.
2. Using box in patterns, to unpack and match. Removing these is ugly, and requires nested matching. But if it's unlikely to ever become stable, and stable is a useful goal, we can suck it up.

[jsgf]

To clarify, I think the hope is that new syntax won't be needed, or at least not something so Box-specific. Allocation needs some answer for "placement new", and I think there's general agreement that there's hole in pattern matching. But I'm not very current on the details.

[ndmitchell]

Any other questions people have that would help decide on the crates ownership? A few people have reached out to me about wanting to depend on the revised starlark-rust, so it would be good to upload it Crates in the near future (whatever name it uses). For the moment they're using it as a git repo, but having it available on Crates makes it easier for them.

[damienmg]

Since there was no more comment for more than a week and seeing that the development on this repo is basically stalled. I would recommend to move the ownership of the crate to @ndmitchell. I will need another week or so to get that rubber stamped on my side so feel free to express blocking.

From my view there is enough will from Neil to move toward a place nice for everybody that this move make sense.

[damienmg]

Hello all,

I have gotten the necessary approval to move forward.

This repository will be replaced with a single README that explains the new states and then archived. The crates ownership will be handed over to the new repository. It is going to happen in the following weeks. Let me know if there are still concerns around this.