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

This commit *was not* tested wit C++. Proceed with caution. #249

Closed
wants to merge 1 commit into from
Closed

This commit *was not* tested wit C++. Proceed with caution. #249

wants to merge 1 commit into from

Conversation

cszyszka
Copy link

This commit fixes compatibility with python3 for the code generated with protoc for python.
Proto definition files were moved to osi3 direcotry.
Proto files were modified in the way that the include stattment now specifis osi3 packge.
e.g.
[OLD] import "osi_version.proto";
changed to
[NEW] import "osi3/osi_version.proto";
This changes fix the issue for python generted code.

For full details see the discussion in the protobuf repository.
https://github.com/google/protobuf/issues/1491

More details on this work will be commented in
https://github.com/google/protobuf/issues/1491

    This commit fixes compatibility with python3 for the code generated with protoc for python.
    Proto definition files were moved to osi3 direcotry.
    Proto files were modified in the way that the include stattment now specifis osi3 packge.
    e.g.
    [OLD] import "osi_version.proto";
    changed to
    [NEW] import "osi3/osi_version.proto";
    This changes fix the issue for python generted code.

    For full details see the discussion in the protobuf repository.
    protocolbuffers/protobuf#1491

    More details on this work will be commented in
    protocolbuffers/protobuf#1491
@ghost ghost added the Bug Problems in the build system, build scripts, etc or faults in the interface. label Jun 11, 2018
@ghost ghost added this to the v3.0.1 milestone Jun 11, 2018
@ghost
Copy link

ghost commented Jun 11, 2018

@cszyszka please crosscheck with your team in order to get the CI running in order to merge this PR, thank you!

@pmai
Copy link
Contributor

pmai commented Jun 11, 2018

So may I summarize: Protobufs suck... who would've guessed.

We cannot move the protos to a subdirectory, because the CMake ProtoBuf support does not work correctly if the proto files are in a subdirectory (and other issues). And we cannot keep it in the same directory, because protobuf python3 build support does not work if its not in a subdirectory. Lovely. I should have stayed in the 80s.

Likely only solution for now: Copy *.proto to temp subdir prior to running the python side of the build.

@cszyszka
Copy link
Author

cszyszka commented Jun 11, 2018

Likely only solution for now: Copy *.proto to temp subdir prior to running the python side of the build.

This is not a problem what about changes to import statements, will that work with CMAKE/CPP ?

@pmai
Copy link
Contributor

pmai commented Jun 11, 2018

Probably need to change the imports on the fly too, i.e. similar to the proto3 converter script (that we still rely on sh and sed in this day and age makes me sad, too):

for f in osi_*.proto
do
    sed -e 's:import "osi_:import "osi3/osi_:' $f > osi3/$f
done

As an intermediate build step between the c++ build and the python build...

@jruebsam
Copy link
Contributor

Don't know if this helps but this is a similar issue: protocolbuffers/protobuf#762
Unfortunately im quiet busy at the moment, sorry about that. BR

@cszyszka
Copy link
Author

Don't know if this helps but this is a similar issue: protocolbuffers/protobuf#762

@jR7 This issue references another one which I used to solve the issue with import statements in generated code for python. protocolbuffers/protobuf#1491

@cszyszka cszyszka closed this Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Problems in the build system, build scripts, etc or faults in the interface.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants