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

Change bevy_internal to be a 'thinner facade' #2851

Closed
wants to merge 4 commits into from
Closed

Change bevy_internal to be a 'thinner facade' #2851

wants to merge 4 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Sep 20, 2021

This change should (hopefully) fix the [src] links for the bevy crates on docs.rs

Objective

Solution

  • Move the modules with working [src] links into the bevy crate, as well as the prelude.
  • Note that the prelude move is only for conceptual reasons - bevy is the user facing crate.
  • I think bevy_internal still needs to exist for dynamic linking reasons - in particular so that Default/MinimalPlugins still get dynamically linked.
  • Although I think it would also be possible for us to remove bevy_internal entirely, and depend on the crates directly in bevy_dylib

@DJMcNab
Change bevy_internal to be a thinner facade
f946e84 
This change should (hopefully) fix the [src] links for the bevy crates
on docs.rs
@DJMcNab DJMcNab requested a review from bjorn3 September 20, 2021 21:15
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 20, 2021
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Sep 20, 2021
@bjorn3
Copy link
Contributor

bjorn3 commented Sep 20, 2021

Although I think it would also be possible for us to remove bevy_internal entirely, and depend on the crates directly in bevy_dylib

That makes it easy to forget to add a new bevy crate as bevy_dylib dependency. This will likely go unnoticed in most cases, and only causing slower link times and when using dylibs yourself possibly an error about dependencies being included in more than one dylib at the same time.

bjorn3
bjorn3 reviewed Sep 20, 2021
View reviewed changes
crates/bevy_log/Cargo.toml Outdated
[dev-dependencies]
bevy_internal = { path = "../bevy_internal", version = "0.5.0" }
# [dev-dependencies]
# bevy_internal = { path = "../bevy_internal", version = "0.5.0" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - I was checking out whether bevy_internal was used, and it's used for a doctest in here.

Not sure why I even commented it out to be honest...

bjorn3
bjorn3 reviewed Sep 20, 2021
View reviewed changes
crates/bevy_internal/src/lib.rs
mod default_plugins;
#[doc(hidden)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does #![doc(hidden)] at the top of the file work? And should it even be hidden at all if most users likely won't look at the docs of this crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, maybe it not being hidden would be fine? The #![doc(hidden)] at the top of the file would also hide MinimalPlugins and DefaultPlugins

@Nilirad
Copy link
Contributor

Nilirad commented Sep 21, 2021

I did some tests on this PR (commit: f946e84)

Documenting bevy crate only

I did

cargo clean --doc
cargo doc -p bevy --no-dependencies --open

Then going to bevy::ecs::system::Query, there is no source link.

Documenting the entire workspace

I did

cargo clean --doc
cargo doc --workspace --no-dependencies --open

And I found source links, like in bevy::ecs::system::Query.
But there are also duplication problems. For the aforementioned struct we also get:

  • bevy::ecs::prelude::Query
  • bevy_ecs::system::Query

there is no bevy_ecs::prelude::Query since bevy_ecs::prelude's children are #[doc(hidden)].
These duplicated items create confusion and clutter search results (and probably increase documenting times).

In a docs.rs context, the bevy_ecs results should disappear if we open the bevy docs, but the prelude items will still appear. Also, we need to be able to pass the --workspace flag to get the source links.

About preludes, I personally would prefer that the contents of the bevy prelude remain visible but are ignored by search results. Also, they should be linked to the non-prelude, bevy based location (e.g. if you are in bevy::ecs::prelude and click on Query, you go on bevy::ecs::system::Query instead of bevy::ecs::prelude::Query.

wooster0
wooster0 reviewed Oct 8, 2021
View reviewed changes
crates/bevy_log/src/lib.rs Outdated Show resolved Hide resolved
@DJMcNab @wooster0
Update crates/bevy_log/src/lib.rs
9bedd2a 
Co-authored-by: r00ster <r00ster91@protonmail.com>
@DJMcNab
Copy link
Member Author

DJMcNab commented Dec 12, 2021

I'm going to close this for now; I think we can do better, but just diving in without creating a model isn't feasible.

@DJMcNab DJMcNab closed this Dec 12, 2021
@DJMcNab DJMcNab deleted the docs_rs_fix branch December 12, 2021 11:55
DJMcNab added a commit to DJMcNab/bevy that referenced this pull request Jul 1, 2022
@DJMcNab
Remove our 'cyclic' dependencies
f7ecc6e 
None of them are remotely necessary, if we import
the trick from bevyengine#2851

This works around rust-lang/rust-analyzer#11410
@DJMcNab DJMcNab mentioned this pull request Jul 1, 2022
Closed
bors bot pushed a commit that referenced this pull request Jul 4, 2022
@DJMcNab
Remove the dependency cycles (#5171)
e64efd3 
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from #2851 for no-op plugin groups.
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@DJMcNab @inodentry
Remove the dependency cycles (bevyengine#5171)
e692be9 
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from bevyengine#2851 for no-op plugin groups.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@DJMcNab @james7132
Remove the dependency cycles (bevyengine#5171)
e10e84d 
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from bevyengine#2851 for no-op plugin groups.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@DJMcNab @ItsDoot
Remove the dependency cycles (bevyengine#5171)
f7052ac 
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from bevyengine#2851 for no-op plugin groups.
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

@wooster0 wooster0 wooster0 left review comments

Assignees
No one assigned
Labels
C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DJMcNab @bjorn3 @Nilirad @wooster0 @alice-i-cecile