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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Prepare crevice for vendored release #3394

Closed
wants to merge 10 commits into from
Closed

[Merged by Bors] - Prepare crevice for vendored release #3394

wants to merge 10 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Dec 19, 2021 •
edited
Loading

Objective

  • Our crevice is still called "crevice", which we can't use for a release
  • Users would need to use our "crevice" directly to be able to use the derive macro

Solution

  • Rename crevice to bevy_crevice, and crevice-derive to bevy-crevice-derive
  • Re-export it from bevy_render, and use it from bevy_render everywhere
  • Fix derive macro to work either from bevy_render, from bevy_crevice, or from bevy

Remaining

  • It is currently re-exported as bevy::render::bevy_crevice, is it the path we want?
  • After a brief suggestion to Cart, I changed the version to follow Bevy version instead of crevice, do we want that?
  • Crevice README.md need to be updated
  • in the Cargo.toml, there are a few things to change. How do we want to change them? How do we keep attributions to original Crevice?
authors = ["Lucien Greathouse <me@lpghatguy.com>"]
documentation = "https://docs.rs/crevice"
homepage = "https://github.com/LPGhatguy/crevice"
repository = "https://github.com/LPGhatguy/crevice"

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 19, 2021
@mockersf mockersf added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on and removed S-Needs-Triage This issue needs to be labelled labels Dec 19, 2021
@cart
Copy link
Member

cart commented Dec 20, 2021

Great work! In general this looks good to me. I think the readme should cover the following topics (in this order):

  • This is a Bevy fork of crevice
  • Non bevy users should probably consume upstream crevice (with a link to it)
  • Explain rationale for the fork / the changes we have made:
    • Easier derive macro usage for bevy users
    • We are currently using unmerged features (ex: the crevice arrays pr)
    • Might ultimately rename types to meet Bevy needs (ex: derive(Uniform) instead of derive(AsStd140))

I think we should leave attributions as they are for now, with the exception of the repo link, which should point to the bevy repo.

@mockersf
Copy link
Member Author

I updated the readme 👍

@mockersf
Copy link
Member Author

Ooh two compiler panics in CI, fancy!

@mockersf
Copy link
Member Author

rust nightly ICE probably related to one of rust-lang/rust#92187 rust-lang/rust#92163 and could be fixed by rust-lang/rust#92175

@cart cart added this to the Bevy 0.6 milestone Dec 23, 2021
MinerSebas
MinerSebas suggested changes Dec 23, 2021
View reviewed changes
Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

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

This crate does also need to be added to the /tools/publish.sh

@cart
Copy link
Member

cart commented Dec 23, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 23, 2021
@mockersf @cart
Prepare crevice for vendored release (#3394)
79d36e7 
# Objective

- Our crevice is still called "crevice", which we can't use for a release
- Users would need to use our "crevice" directly to be able to use the derive macro

## Solution

- Rename crevice to bevy_crevice, and crevice-derive to bevy-crevice-derive
- Re-export it from bevy_render, and use it from bevy_render everywhere
- Fix derive macro to work either from bevy_render, from bevy_crevice, or from bevy

## Remaining

- It is currently re-exported as `bevy::render::bevy_crevice`, is it the path we want?
- After a brief suggestion to Cart, I changed the version to follow Bevy version instead of crevice, do we want that?
- Crevice README.md need to be updated
- in the `Cargo.toml`, there are a few things to change. How do we want to change them? How do we keep attributions to original Crevice?
```
authors = ["Lucien Greathouse <me@lpghatguy.com>"]
documentation = "https://docs.rs/crevice"
homepage = "https://github.com/LPGhatguy/crevice"
repository = "https://github.com/LPGhatguy/crevice"
```


Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Dec 23, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Prepare crevice for vendored release [Merged by Bors] - Prepare crevice for vendored release Dec 23, 2021
@bors bors bot closed this Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@cart cart cart left review comments

@MinerSebas MinerSebas MinerSebas requested changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Milestone
Bevy 0.6
Development

Successfully merging this pull request may close these issues.
4 participants
@mockersf @cart @bjorn3 @MinerSebas