Do not symlink system protobuf headers but only the required .proto files #43153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Symlinking the system headers has proven to be problematic as newer versions of protobuf add or remove headers which makes having a static array of header files hard to impossible. Turns out the headers don't need to be symlinked at all but only the .proto files used as inputs need to be present.
| "google/protobuf/wrappers.proto", | ||
| ] | ||
|
|
||
| genrule( | ||
| name = "link_headers", | ||
| outs = HEADERS, | ||
| name = "link_proto_files", |
There was a problem hiding this comment.
This rule is no longer used now
There was a problem hiding this comment.
I think that is ok. I'm not fully sure but as it declares the outs the rules using the proto files will run this implicitly. Or do I misunderstand that? I'll rerun the build to verify just to make sure.
There was a problem hiding this comment.
My tests on 2 different systems using this succeeded, so my assessment seems to be correct.
mihaimaruseac
approved these changes
Sep 15, 2020
google-ml-butler
bot
added
kokoro:force-run
rthadur
added
kokoro:force-run
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Symlinking the system headers has proven to be problematic as newer versions of protobuf add or remove headers which makes having a static array of header files hard to impossible. Turns out the headers don't need to be symlinked at all but only the .proto files used as inputs need to be present.
Example: In #34792 @cbalint13 removed the
port_def.inc"header". But the workspace.bzl requests protobuf 3.8.0 (tensorflow/tensorflow/workspace.bzl
Line 423 in 610a78b
This leads to build failures due to this missing (although I think if all would be working well it would just take the header from the system if
$INCLUDEDIRis in the compilers header search path).This partially resolves/helps #37861