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

[Merged by Bors] - Fix dynamic linking (on linux) #7333

Closed

Conversation

tim-blackbird
Copy link
Contributor

Problemo

Some code in #5911 and #5454 does not compile with dynamic linking enabled.
The code is behind a feature gate to prevent dynamically linked builds from breaking, but it's not quite set up correctly.

Solution

Forward the dynamic feature flag to the bevy_diagnostic crate and gate the code behind it.

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Jan 22, 2023
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps labels Jan 22, 2023
@alice-i-cecile alice-i-cecile added the A-Diagnostics Logging, crash handling, error reporting and performance analysis label Jan 22, 2023
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

Could you give the feature a more explicit name like dynamic_linking?

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I can confirm that non-dynamic linking still works as expected and the changes make sense to me, but I can't test the dynamic feature on my machine.

@IceSentry
Copy link
Contributor

I like the idea of renaming the dynamic feature, but I think this should be done in a separate PR that isn't intended to fix a bug.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 22, 2023
@mockersf
Copy link
Member

I like the idea of renaming the dynamic feature, but I think this should be done in a separate PR that isn't intended to fix a bug.

You could argue that the bug was caused by the feature name not being explicit...

Copy link
Contributor

@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.

Tested. It works on my machine!

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Jan 22, 2023

You could argue that the bug was caused by the feature name not being explicit...

Agreed!
But it would be a breaking change so I think it merits it's own discussion :)

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 22, 2023
# Problemo

Some code in #5911 and #5454 does not compile with dynamic linking enabled.
The code is behind a feature gate to prevent dynamically linked builds from breaking, but it's not quite set up correctly.

## Solution

Forward the `dynamic` feature flag to the `bevy_diagnostic` crate and gate the code behind it.


Co-authored-by: devil-ira <[email protected]>
@bors bors bot changed the title Fix dynamic linking (on linux) [Merged by Bors] - Fix dynamic linking (on linux) Jan 22, 2023
@bors bors bot closed this Jan 22, 2023
@tim-blackbird tim-blackbird deleted the fix-dynamic-linking-linux branch January 24, 2023 15:18
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Problemo

Some code in bevyengine#5911 and bevyengine#5454 does not compile with dynamic linking enabled.
The code is behind a feature gate to prevent dynamically linked builds from breaking, but it's not quite set up correctly.

## Solution

Forward the `dynamic` feature flag to the `bevy_diagnostic` crate and gate the code behind it.


Co-authored-by: devil-ira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants