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

Link compiled protobuf files to protobuf::libprotobuf #106370

Closed
wants to merge 1 commit into from
Closed

Link compiled protobuf files to protobuf::libprotobuf #106370

wants to merge 1 commit into from

Conversation

Flamefire
Copy link
Collaborator

@Flamefire Flamefire commented Aug 1, 2023 •
edited

This correctly propagates the required compile flags (such as -DPROTOBUF_USE_DLLS for shared builds of protobuf).

The manual extraction of the include path and adding that globally is also no longer required as that is part of that targets interface.

Fixes #106297
Fixes #106206

@Flamefire
Link compiled protobuf files to protobuf::libprotobuf
dc0cc29 
This correctly propagates the required compile flags (such as
`-DPROTOBUF_USE_DLLS` for shared builds of protobuf).

Fixes pytorch#106297
@pytorch-bot
Copy link

pytorch-bot bot commented Aug 1, 2023 •
edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/106370

Note: Links to docs will display an error until the docs builds have been completed.

❌ 16 New Failures

As of commit dc0cc29:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

This PR needs a release notes: label

If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@soulitzer soulitzer requested a review from malfet August 1, 2023 16:28
@soulitzer soulitzer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 1, 2023
@github-actions
Copy link

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Sep 30, 2023
@Flamefire
Copy link
Collaborator Author

This is still required. Any comments/todos here?

@github-actions github-actions bot closed this Nov 6, 2023
@Flamefire
Copy link
Collaborator Author

@soulitzer @malfet this was closed without being addressed but it fixes issues we do have. Can this be reopened and reviewed please?

@anthonyalayo
Copy link
Contributor

@Flamefire without this patch, what are the exact reprocussions? Is it that one cannot use a system protobuf at all? Is there any existing workarounds? (I am trying to understand if I am encountering the same issue. I can help push it forward.)

@Flamefire
Copy link
Collaborator Author

Without this patch there could be a system protobuf compiled as a shared library which requires some defines when linking against it such as -DPROTOBUF_USE_DLLS. Without those the linking or even compiling fails depending on the defines missing.

A workaround would be to exactly know which protobuf will be linked and (finding and) passing all required defines and potentially other flags manually. Not really reliable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malfet malfet Awaiting requested review from malfet

Assignees
No one assigned
Labels
open source Stale triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't build with non-static protobuf Build failure due to C++ version mismatch
4 participants
@Flamefire @anthonyalayo @soulitzer @pytorchbot