Fixing import paths so generated headers work #161
Merged
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
stevenhartley
pushed a commit
that referenced
this pull request
May 23, 2024
The generated code needs to be in the `uprotocol` namespace folder structure for clarity however the protos root was the uprotocol folder so generated code was missing the uprotocol folder. This change fixes the protos and cleans up the build to still work and validate the build/changes. #161
gregmedd
pushed a commit
to gregmedd/up-spec
that referenced
this pull request
May 23, 2024
The generated code needs to be in the `uprotocol` namespace folder structure for clarity however the protos root was the uprotocol folder so generated code was missing the uprotocol folder. This change fixes the protos and cleans up the build to still work and validate the build/changes. eclipse-uprotocol#161
|
I've combined #162 into this PR. |
gregmedd
pushed a commit
to gregmedd/up-spec
that referenced
this pull request
May 23, 2024
The generated code needs to be in the `uprotocol` namespace folder structure for clarity however the protos root was the uprotocol folder so generated code was missing the uprotocol folder. This change fixes the protos and cleans up the build to still work and validate the build/changes. eclipse-uprotocol#161
gregmedd
force-pushed
the
bugfix/proto-import-paths
branch
from
78e563e
to
27d1d9d
Compare
The protobuf compiler uses the concept of a "canonical" path when
generating source files. This is the base point where it searches for
import files. It _expects_ that path will match the base path for
searching for headers, too. The end result is that
import "v1/ustatus.proto"
will generate a C++ header that contains
#include "v1/ustatus.pb.h"
while we have been explicitly asked to use includes starting with
`uprotocol` in up-cpp:
#include "uprotocol/v1/ustatus.pb.h"
We were able to work around this with some tricks when the import paths
were file names only (e.g. `import ustatus.proto`). However, the proto
files have moved under the `v1/` directory as of eclipse-uprotocol#141.
Based on my reading of the protoc documentation and several stack
overflow postings, it seems the intended way for .proto files to be
written in this scenario is to import the full canonical path as
intended for the output files:
import "uprotocol/v1/ustatus.proto"
so that protoc can generate the correct paths in output sources in the
first place.
The generated code needs to be in the `uprotocol` namespace folder structure for clarity however the protos root was the uprotocol folder so generated code was missing the uprotocol folder. This change fixes the protos and cleans up the build to still work and validate the build/changes.
gregmedd
force-pushed
the
bugfix/proto-import-paths
branch
from
27d1d9d
to
25731fa
Compare
sophokles73
reviewed
May 24, 2024
Applying suggestions from review. This parameter should not be needed when pom.xml is in the current directory. Co-authored-by: Steven Hartley <steven@orangepeel.ca>
The --file flag is not needed since pom.xml is in the repo root now. The proto source root was also pointing to the wrong place now that the xml file has been moved up a directory. Also fixing the URLs for the project/repository since they were pointing to the old core API archive.
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.
The protobuf compiler uses the concept of a "canonical" path when generating source files. This is the base point where it searches for import files. It expects that path will match the base path for searching for headers, too. The end result is that
will generate a C++ header that contains
while we have been explicitly asked to use includes starting with
uprotocolin up-cpp:We were able to work around this with some tricks when the import paths were file names only (e.g.
import ustatus.proto). However, the proto files have moved under thev1/directory as of #141.Based on my reading of the protoc documentation and several stack overflow postings, it seems the intended way for .proto files to be written in this scenario is to import the full canonical path as intended for the output files:
so that protoc can generate the correct paths in output sources in the first place.