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

protobuf : add version 4.25.0 #23095

Closed
wants to merge 1 commit into from
Closed

protobuf : add version 4.25.0 #23095

wants to merge 1 commit into from

Conversation

sophieeihpos
Copy link
Contributor

@sophieeihpos sophieeihpos commented Mar 14, 2024 •
edited
Loading

protobuf/4.25.0

Part 1 fix for #23084

© 2024 Morgan Stanley.
THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE Conan-io project Contributor License Agreement.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS

@github-actions GitHub Actions
Copy link
Contributor

🤖 Beep Boop! This pull request is making changes to 'recipes/protobuf//'.

👋 @Hopobcn you might be interested. 😉

@sophieeihpos sophieeihpos mentioned this pull request Mar 14, 2024
Closed
3 tasks
@conan-center-bot

This comment has been minimized.

Sign in to view
@ghost
Copy link

ghost commented Mar 14, 2024 •
edited by ghost
Loading

I detected other pull requests that are modifying protobuf/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

This was referenced Mar 14, 2024
Closed
Closed
Closed
@sophieeihpos sophieeihpos force-pushed the protobuf_add_3.22.3 branch 2 times, most recently from 39036a8 to 00acc63 Compare March 14, 2024 11:23
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@Ahajha
Copy link
Contributor

Ahajha commented Mar 15, 2024

Note: There's another PR to add 4.25.3 here: #21622

Also, note that protobuf's versioning is a little strange, there is no 3.22, it goes from 3.21 to 4.22. See here: https://protobuf.dev/support/version-support/#cpp

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot conan-center-bot added the Missing dependencies Build failed due missing dependencies in Conan Center label Mar 19, 2024
@conan-center-bot

This comment has been minimized.

Sign in to view
@sophieeihpos sophieeihpos force-pushed the protobuf_add_3.22.3 branch 2 times, most recently from 05ece74 to a28355d Compare March 19, 2024 14:39
@conan-center-bot

This comment has been minimized.

Sign in to view
@sophieeihpos sophieeihpos force-pushed the protobuf_add_3.22.3 branch 2 times, most recently from 2d3647c to e5397a9 Compare March 20, 2024 10:09
@conan-center-bot

This comment has been minimized.

Sign in to view
@sophieeihpos sophieeihpos force-pushed the protobuf_add_3.22.3 branch 2 times, most recently from dc51fcc to 64904ab Compare April 16, 2024 17:05
@conan-center-bot

This comment has been minimized.

Sign in to view
@sophieeihpos sophieeihpos force-pushed the protobuf_add_3.22.3 branch 2 times, most recently from d886bab to a508cfe Compare April 17, 2024 09:00
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@HennerM
Copy link

HennerM commented Apr 20, 2024

@sophieeihpos thanks for working on this! Do you know when this can get merged?

@Ahajha
Copy link
Contributor

Ahajha commented Apr 21, 2024

Just as an FYI for reviewers, there is another PR open here: #21622.

@sophieeihpos
Copy link
Contributor Author

Any estimate when this PR or the other will get reviewed ? @Ahajha @uilianries

@Ahajha
Copy link
Contributor

Ahajha commented Apr 25, 2024

I'll try to re-review in the next day or two, sorry for the delay. (Will still need two other reviews). I did glance at it a few days ago and everything seemed okay, my only immediate concern is that a maintainer might request that the new option you've added go in a separate PR, but I'm not sure on the policy myself. Adding some more context to the changes in the PR description also tends to help things along I find.

Ahajha
Ahajha approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@Ahajha Ahajha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM - other reviewers keep in mind there is another PR so maybe there's some stuff in that one that might be worth looking at.

recipes/protobuf/all/conanfile.py Outdated
Comment on lines 77 to 78
if self.options.shared and is_msvc_static_runtime(self):
raise ConanInvalidConfiguration("Protobuf can't be built with shared + MT(d) runtimes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Minor): Is this still the case in 4.25.0+? If so, maybe this should be moved outside the condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 4.25.0+, shared windows build is not supported. Shared windows abseil is not available in its recipe, and static windows abseil does not build with shared protobuf.

recipes/protobuf/all/conanfile.py Outdated
Comment on lines 82 to 89
if self.settings.compiler == "clang":
if Version(self.settings.compiler.version) < "4":
raise ConanInvalidConfiguration(f"{self.ref} doesn't support clang < 4")
else:
if self.options.shared and self.settings.os == "Windows":
# The executable protoc is not supported for shared type on Windows.
# For v4.25.x , the dependency, shared abseil is not supported on Windows; shared protobuf cannot build with static asbeil on Windows.
raise ConanInvalidConfiguration(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Minor): The validation of the two versions is very different, maybe it would be cleaner to use the same method of checking the minimum compiler version for all versions? It's like this in the other PR (see here: https://github.com/conan-io/conan-center-index/pull/21622/files#diff-5dfb647bf8fb82769ddb94143d6b47475c5d4d2db2346115ee6b1991a6db6445R57)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I can update

recipes/protobuf/all/test_package/conanfile.py Outdated
Comment on lines 23 to 24
if Version(self.dependencies["protobuf"].ref.version) >= "4.25.0" and not self.settings.compiler.get_safe("cppstd", False):
tc.variables["CMAKE_CXX_STANDARD"] = "14"
Copy link
Contributor

@Ahajha Ahajha Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized there's a better way of doing this, since in the test package we're in control of the CMake file, in the CMake we can add the following:

if (protobuf_VERSION VERSION_LESS "4.0.0")
    target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_11)
else()
    target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14)
endif()

In place of the existing target_compile_features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try and see.

@sophieeihpos
Copy link
Contributor Author

I'll try to re-review in the next day or two, sorry for the delay. (Will still need two other reviews). I did glance at it a few days ago and everything seemed okay, my only immediate concern is that a maintainer might request that the new option you've added go in a separate PR, but I'm not sure on the policy myself. Adding some more context to the changes in the PR description also tends to help things along I find.

Thank you for the review. Any ideas where I can get more reviewers ?

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 54 (0cb412c23f0c7ac1143ded54e3f238bbfffbeb81):

  • protobuf/4.25.0:
    All packages built successfully! (All logs)

  • protobuf/3.20.0:
    All packages built successfully! (All logs)

  • protobuf/3.21.12:
    All packages built successfully! (All logs)

  • protobuf/3.21.9:
    All packages built successfully! (All logs)

  • protobuf/3.17.1:
    All packages built successfully! (All logs)

  • protobuf/3.20.3:
    All packages built successfully! (All logs)

  • protobuf/3.18.3:
    All packages built successfully! (All logs)

  • protobuf/3.19.6:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 56 (0cb412c23f0c7ac1143ded54e3f238bbfffbeb81):

  • protobuf/4.25.0:
    All packages built successfully! (All logs)

  • protobuf/3.21.9:
    All packages built successfully! (All logs)

  • protobuf/3.21.12:
    All packages built successfully! (All logs)

  • protobuf/3.20.0:
    All packages built successfully! (All logs)

  • protobuf/3.17.1:
    All packages built successfully! (All logs)

  • protobuf/3.20.3:
    All packages built successfully! (All logs)

  • protobuf/3.18.3:
    All packages built successfully! (All logs)

  • protobuf/3.19.6:
    All packages built successfully! (All logs)

@sophieeihpos
Copy link
Contributor Author

@RubenRBS @uilianries Could you please help review ?

@saschasc
Copy link

Is there any advantage adding 4.25.0 if there exists a PR for 4.25.3?

#21622

Anyway would be great if some of the reviewers could push this. Thank you.

@sophieeihpos
Copy link
Contributor Author

Is there any advantage adding 4.25.0 if there exists a PR for 4.25.3?

#21622

Anyway would be great if some of the reviewers could push this. Thank you.

The other PR was created on 5th Dec 2023. I was hoping to break it down into smaller pieces, have more frequent merges, and have a minimal viable product at least. I am happy to get either of these two merged.

@ghost ghost mentioned this pull request May 29, 2024
Merged
@jcar87
Copy link
Contributor

jcar87 commented May 30, 2024 •
edited
Loading

Thanks @sophieeihpos - closing as superseded by #24154

It's unclear from this PR what problems the proposed rpath logic are trying to fix - if this is still a concern, could you please open a new issue providing more details? We will be happy to assist, thank you so much!

@jcar87 jcar87 closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ahajha Ahajha Ahajha approved these changes

@uilianries uilianries Awaiting requested review from uilianries

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sophieeihpos @conan-center-bot @Ahajha @HennerM @saschasc @jcar87