Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bevy nursery: a path to upstreaming ecosystem crates #63

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Aug 8, 2022

RENDERED

Work done within the broader Bevy ecosystem is sometimes a good fit as part of the core engine.
These crates should live in a nursery owned by the Bevy org for a period of time,
where they can be polished and refined by the broader community,
before eventually being merged in completely.

@alice-i-cecile
Copy link
Member Author

This RFC came about due to a discussion with @cart, @aevyrie and @alercah at RustConf :)

It presents an alternate approach to some of the goals in #52, to enable faster and more experimental iteration on large well-contained projects.

@sixfold-origami sixfold-origami self-requested a review August 8, 2022 18:12
Copy link
Member

@BoxyUwU BoxyUwU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if "approving" RFCs is a thing but I like this 😆

Copy link

@djeedai djeedai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll note that Druid has a nursery repo exactly for that purpose, with pretty much the same rules (except the upstreaming ones which are not written).

This looks like a well thought RFC with direct benefit to all parties involved. All agreed.

@djeedai
Copy link

djeedai commented Aug 8, 2022

Ah yes 1 comment: I don't think "pick the winner" is necessarily a drawback so long as those crates are integrated under a feature flag. If there's a "second winner" or even 2 strong crates to begin with, they likely each have strengths and weaknesses and are more suited for some use cases, so I don't think it's bad to have say 2 crates for audio that the user can choose from.

Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this proposal because it provides a very clear path for upstreaming a crate, as well as explicit guidelines for those interested in doing so.

bevy_mod_picking was created based on @cart's prescient request to allow the picking API to be tested and mature in a parallel crate before integrating into the engine. With the nursery in place, it gives me a very clear path to contributing my work back to the engine, which is not currently the case. Instead of putting the work behind a flag, or in a PR that constantly needs to be rebased, this would allow for actual user testing like any other third party crate.

rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Show resolved Hide resolved
rfcs/63-nursery.md Show resolved Hide resolved
Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea. I have a couple comments.

4. Will have a branch prepared for the newest release of Bevy before we release a new major version.

You can find these crates by looking for the special `Nursery` tag on [Bevy Assets](https://bevyengine.org/assets/),
or by browsing the [`bevyengine` organization](https://github.com/bevyengine)'s list of repositories.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you like, you could also assign a nursery topic to the repositories themselves. A GitHub topic is like a label on a repository, and can be specified as a criteria in a search (if you wanted to, for example, link to a search displaying all the nursery repositories in the bevyengine org). For example, rust, game-engine, and 2d are topics on one of my own repositories in the screenshot below. You can add/edit topics by editing the About section of a repository.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, but I think this is probably redundant.

rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Outdated Show resolved Hide resolved
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly in favor of this effort, but I do have comments (left above instead of below 😄)

rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Outdated Show resolved Hide resolved
rfcs/63-nursery.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member Author

Feedback addressed :) The main point worth addressing explicitly at this point is "nursery crate owners get scoped merge rights". No override for controversial PRs any more; if that process needs accelerating we should address that separately.

We could grant:

  1. No merge rights.
  2. Merge rights to directly relevant code.
  3. Merge rights to adjacent areas of the engine (e.g. all of audio).
  4. Full merge rights for non-controversial PRs.

My personal feeling is that 2/3, possibly graduating to 4 is the right way to go :) In my experience, experimenting with authority over small areas has been a great model for both sides of the equation.

@aevyrie
Copy link
Member

aevyrie commented Aug 8, 2022

The main point worth addressing explicitly at this point is "nursery crate owners get scoped merge rights".

Why should contributors going through the nursery process get merge rights, when PR contributors do not? I think we need a good answer to that question before deciding on what, if any, merge rights to grant.

Does this imply that large enough PRs should be rejected, and told to go through the nursery process? What are the criteria for this threshold?

@cart
Copy link
Member

cart commented Aug 8, 2022

Why should contributors going through the nursery process get merge rights, when PR contributors do not? I think we need a good answer to that question before deciding on what, if any, merge rights to grant.

I agree with this. The fact that someone wrote code does not qualify them to maintain it within the context of Bevy. That is a separate concern and should have its own process / criteria.

Large PR vs Nursery are two paths that accomplish the same goal. They should be treated the same in this regard.

I'm open to considering opening the doors to scoped merge rights (maybe via codeowners). But my vote is to remove things of this nature from this RFC and then have that conversation on its own time.

@alice-i-cecile
Copy link
Member Author

After consideration, I agree with you @cart and @aevyrie. I've updated the RFC to match that position now :)

Copy link

@afonsolage afonsolage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a great idea, I love it, but I have some questions, 3 related to nursery crates versioning, and another 4º:

4 - When a nursery crate will be removed/discontinued from bevy-nursery? In case of removing a nursery crate, it will be archived or "returned" to original author?

1. Live in the [`bevyengine` organization](https://github.com/bevyengine) on GitHub.
2. Follow Bevy's standards for code quality and project management.
3. Explicitly welcome community contributions to help get them ready for release.
4. Will have a branch prepared for the newest release of Bevy before we release a new major version.
Copy link

@afonsolage afonsolage Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few questions about this:

1- If some bevy repo PR introduce a breaking change into some nursery crate, there will be some defined process to detect and warn nursery crate maintainers? Like a custom CI on every nursery crate which will detect it?

2- What happens if at the end of the 3-month cycle, bevy is ready for a new release, but some nursery crates doesn't, either because of some complex breaking change or because maintainers didn't have time to update?

3- When releasing patch notes/migration guides/news, the changes on nursery crates will be included?

Copy link

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any corrections to suggest, however I have some questions that I think are not answered in the RFC.

  • What about more defined rules on branch structure? For example:
    • Must the default branch be named main?
    • Should one branch constantly track Bevy's main branch?
    • If yes, what it should be? The default branch or a dedicated one?
  • What happens to crates that are initially adopted by the nursery but at one point they become no longer the best solution for upstreaming (e.g. new better competitor, neglect, changes in Bevy vision)?

Copy link

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very much in favour of this. Thanks a lot for moving this forward.

I am often very wary of adding new external plugins because of the mentioned drawbacks.

Having Bevy's official stamp of approval and guarantee that they are going to be maintained empowers users to try more functionality out.

rfcs/63-nursery.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants