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

Support more protobuf versions in GitHub workflow #766

Open
wants to merge 83 commits into
base: master
Choose a base branch
from

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented Jan 22, 2024

Reference to a related issue in the repository

#765

Add a description

Fix pipeline behavior with different protobuf versions

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

If you can’t check all of them, please explain why.
If all boxes are checked or commented and you have achieved at least one positive review, you can assign the label ReadyForCCBReview!

Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff ClemensLinnhoff self-assigned this Jan 22, 2024
@ClemensLinnhoff ClemensLinnhoff linked an issue Jan 22, 2024 that may be closed by this pull request
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff
Copy link
Contributor Author

ClemensLinnhoff commented Jan 22, 2024

Protobuf 2.6.1 does not work out of the box, since it does not have '-all' in the link/package:
https://github.com/OpenSimulationInterface/open-simulation-interface/actions/runs/7609941012

If '-all' is changed to '', it will download abseil, but cannot install it, since protobuf 2.6.1 has autotools (configure). So the if condition distinguishing between autotools and cmake is not correct:
https://github.com/OpenSimulationInterface/open-simulation-interface/actions/runs/7609969004

Also, between 3.0.0 and 3.5.0 there is no package for all environments.

@ClemensLinnhoff
Copy link
Contributor Author

With my changes, protobuf 2.6.1 can be installed. But now installing python fails:
https://github.com/OpenSimulationInterface/open-simulation-interface/actions/runs/7610124644

Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff
Copy link
Contributor Author

I now managed to get the setup.py to work with protobuf 2.6.1. However, now a unit test fails. Apparently Protobuf 2.6.1 does not support Python 3...

My recommendation: Let's set the minimum proto version to 3.0.0, as it already says in the documentation anyways.

@pmai
Copy link
Contributor

pmai commented Feb 12, 2024

I now managed to get the setup.py to work with protobuf 2.6.1. However, now a unit test fails. Apparently Protobuf 2.6.1 does not support Python 3...

My recommendation: Let's set the minimum proto version to 3.0.0, as it already says in the documentation anyways.

At least officially protobuf states Python 3 support since 2.6.0... The error logs look more like there are changes in the location of the bundled proto include?

@jdsika
Copy link
Contributor

jdsika commented Feb 12, 2024

I now managed to get the setup.py to work with protobuf 2.6.1. However, now a unit test fails. Apparently Protobuf 2.6.1 does not support Python 3...
My recommendation: Let's set the minimum proto version to 3.0.0, as it already says in the documentation anyways.

At least officially protobuf states Python 3 support since 2.6.0... The error logs look more like there are changes in the location of the bundled proto include?

What about this here? protocolbuffers/protobuf#882

@jdsika
Copy link
Contributor

jdsika commented Feb 12, 2024

Considerations:

  • IF protobuffer min. version is set to >3.0.0 THEN we should switch to proto3 syntax
  • Remove proto 2to3 converter
  • Remove second build step
  • check flatc compatibility with proto3 syntax

@jdsika
Copy link
Contributor

jdsika commented Apr 25, 2024

@ClemensLinnhoff and @pmai this PR must be closed and we have to check if there is anything missing. See also my questions above. I think we are still inconsistent as the docs still mention proto version >3.0.0 and then my questions with the proto syntax arise!

@jdsika jdsika added this to the V3.7.0 milestone Apr 25, 2024
@jdsika jdsika added HelpWanted I have tried my best - please help me! Documentation Everything which impacts the quality of the documentation and guidelines. labels Apr 25, 2024
@ClemensLinnhoff
Copy link
Contributor Author

Python is now completely decoupled from the protobuf version. So the changes to setup.py are superseded.
What is the status on proto 2 support for C++? Then we might need the changes. However, only the CI pipeline has minor changes.

@ClemensLinnhoff ClemensLinnhoff changed the title Check different protobuf versions Support more protobuf versions in GitHub workflow Apr 26, 2024
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff ClemensLinnhoff marked this pull request as ready for review April 26, 2024 15:05
@jdsika jdsika added ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. and removed HelpWanted I have tried my best - please help me! labels May 2, 2024
@pmai
Copy link
Contributor

pmai commented May 6, 2024

Since this only affects our internal build pipeline, and we have no direct necessity to always build against multiple protobuf versions (we have not had any regressions due to PB changes, only our build pipeline is ever affected), I think this change is not release relevant, and we should look at this more carefully after the release (i.e. what really are our needs here, and how do we want to structure this; the past CI improvements were more for moving forwards). The danger that we inadvertantly blow up some part of the build or release process seems paramount.

@pmai pmai removed this from the V3.7.0 milestone May 6, 2024
@PhRosenberger PhRosenberger added this to the V3.8.0 milestone May 6, 2024
@jdsika jdsika modified the milestones: V3.8.0, V3.7.1 Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Everything which impacts the quality of the documentation and guidelines. ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check different Protobuf versions
4 participants