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

[OpenCV] Make the protobuf version a property #7437

Conversation

ericriff
Copy link
Contributor

I'm using a custom version of protobuf. I spent a bunch of time trying to debug a build error, turns out my protobuf versions were not in sync.
I can see this happening again when someone updates the version here. Current implementation is error prone.

By making the version a property we ensure that both the requirement and the build_requierement are in sync.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@ericriff ericriff changed the title [OpenCV]Make the protobuf version a property [OpenCV] Make the protobuf version a property Sep 27, 2021
@ghost
Copy link

ghost commented Sep 27, 2021

I detected other pull requests that are modifying opencv/4.x recipe:

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

@ghost ghost mentioned this pull request Sep 27, 2021
4 tasks
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Fair enough, as github diff hides untouched code, any dependencies bump could result in your case.

@ericriff
Copy link
Contributor Author

It is hard to tell if this is making progress or if it is stuck.
CCI has been giving me a hard time haha

@prince-chrismc
Copy link
Contributor

30hrs for OpenCV might be sensible... Maybe the team can look internal at Jenkins tomorrow? 🙏

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented Sep 29, 2021

Triggering that build again, sometimes it happens that the CI hangs 🤷


About the PR, protobuf usually raise an error on runtime about mismatching versions to generate and use the generated protos, doesn't it? Nevermind, this will make the recipe more maintainable.

I would also add one extra check. Conan can override the protobuf version required here from downstream consumers, but the build-requires will always be the same. We can add something like the following to ensure that, at least when we are generating the packages, everything matches:

def build(self):
    if self.deps_cpp_info["protobuf"].version != self. _protobuf_version:
        raise ConanInvalidConfiguration("You are linking with protobuf/{} while your build require is protobuf/{}. Both need to be the same")

Typically this should go in validate(), but I think we cannot get the version there yet. Maybe we can improve the check, maybe patch versions are compatible.

@conan-center-bot
Copy link
Collaborator

All green in build 2 (460ce1c4e9264c5dcad60de71783cfae67f04598):

  • opencv/4.1.2@:
    All packages built successfully! (All logs)

  • opencv/4.5.0@:
    All packages built successfully! (All logs)

  • opencv/4.5.1@:
    All packages built successfully! (All logs)

  • opencv/4.5.2@:
    All packages built successfully! (All logs)

  • opencv/4.5.3@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 6c65d4c into conan-io:master Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants