Skip to content

Use one BevyManifest instance in proc macros #16766

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
merged 2 commits into from
Dec 15, 2024
Merged

Use one BevyManifest instance in proc macros #16766

merged 2 commits into from
Dec 15, 2024

Conversation

@raldone01
Copy link
Contributor

@raldone01 raldone01 commented Dec 11, 2024 •
edited
Loading

Objective

  • Minor consistency improvement in proc macro code.
  • Remove get_path_direct since it was only used once anyways and doesn't add much.

Solution

  • Possibly a minor performance improvement since the Cargo.toml wont be parsed as often.

Testing

  • I don't think it breaks anything.
  • This is my first time working on bevy itself. Is there a script to do a quick verify of my pr?

Other PR

Similar to #7536 but has no extra dependencies.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@raldone01 raldone01 force-pushed the perf/shared_bevy_manifest branch 3 times, most recently from b215d32 to 5831a57 Compare December 11, 2024 14:39
@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2024

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@raldone01 raldone01 force-pushed the perf/shared_bevy_manifest branch from 5831a57 to 2edcc52 Compare December 11, 2024 15:02
@raldone01 raldone01 marked this pull request as ready for review December 11, 2024 15:12
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change D-Macros Code that generates Rust code S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 11, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Dec 11, 2024

Is there a script to do a quick verify of my pr?
Use cargo run -p ci, which runs the ci project in the tools folder :)

alice-i-cecile
alice-i-cecile requested changes Dec 11, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looks nice, but as the bot says, you'll need to bump the MSRV. I'm pretty sure that's due to the use of LazyLock here?

@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 11, 2024
@raldone01
Copy link
Contributor Author

raldone01 commented Dec 11, 2024 •
edited
Loading

No, the MSRV issue was because I also tried to use CARGO_MANIFEST_PATH instead of CARGO_MANIFEST_DIR which apparently needs a higher MSRV.

I did a quick search but did not find any annotations when it was added exactly, so I just reverted back to using CARGO_MANIFEST_DIR.

As you can see the all the ci runs passed.

@raldone01
Copy link
Contributor Author

raldone01 commented Dec 11, 2024

Use cargo run -p ci, which runs the ci project in the tools folder :)

I tried to run the ci scripts by cding into the folder and running them with cargo run.

Consider adding an advisory for cargo run -p ci to the PR-template.

@raldone01
Copy link
Contributor Author

raldone01 commented Dec 12, 2024

The pr says waiting on author. Do I have to do anything?

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 12, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Dec 12, 2024

Nope, I just forgot to update the label after your response :) This needs a second review then it's good to merge.

mockersf
mockersf reviewed Dec 13, 2024
View reviewed changes
@raldone01
Use one BevyManifest instance in the proc macros since `CARGO_MANIF…
b6e22ea 
…EST_DIR` won't change during the execution.
@raldone01 raldone01 force-pushed the perf/shared_bevy_manifest branch from 16e5686 to b6e22ea Compare December 14, 2024 20:55
@mockersf mockersf enabled auto-merge December 15, 2024 14:49
@mockersf mockersf added this pull request to the merge queue Dec 15, 2024
Merged via the queue into bevyengine:main with commit 760d0a3 Dec 15, 2024
28 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Dec 30, 2024
Closed
@BenjaminBrienen BenjaminBrienen mentioned this pull request Jan 5, 2025
Closed
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@raldone01 @mockersf @ecoskey
Use one BevyManifest instance in proc macros (bevyengine#16766)
79663b3 
# Objective

- Minor consistency improvement in proc macro code.
- Remove `get_path_direct` since it was only used once anyways and
doesn't add much.

## Solution
- Possibly a minor performance improvement since the `Cargo.toml` wont
be parsed as often.

## Testing

- I don't think it breaks anything.
- This is my first time working on bevy itself. Is there a script to do
a quick verify of my pr?

## Other PR

Similar to bevyengine#7536 but has no extra dependencies.

Co-authored-by: François Mockers <mockersf@gmail.com>
@jiangheng90 jiangheng90 mentioned this pull request Jan 21, 2025
Closed
@Jondolf Jondolf mentioned this pull request Jan 30, 2025
Closed
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
@raldone01 @mockersf @mrchantey
Use one BevyManifest instance in proc macros (bevyengine#16766)
a6a14b8 
# Objective

- Minor consistency improvement in proc macro code.
- Remove `get_path_direct` since it was only used once anyways and
doesn't add much.

## Solution
- Possibly a minor performance improvement since the `Cargo.toml` wont
be parsed as often.

## Testing

- I don't think it breaks anything.
- This is my first time working on bevy itself. Is there a script to do
a quick verify of my pr?

## Other PR

Similar to bevyengine#7536 but has no extra dependencies.

Co-authored-by: François Mockers <mockersf@gmail.com>
@raldone01 raldone01 mentioned this pull request Mar 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

Assignees

No one assigned

Labels

C-Code-Quality A section of code that is hard to understand or change D-Macros Code that generates Rust code D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@raldone01 @alice-i-cecile @mockersf