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

opencv/4.1.2 #4270

Merged
merged 8 commits into from
Feb 10, 2021
Merged

opencv/4.1.2 #4270

merged 8 commits into from
Feb 10, 2021

Conversation

blackliner
Copy link
Contributor

Specify library name and version: opencv/4.1.2

needed for CUDA 10.0 nvcc compatibility.

Due to this bug I had to add a version check for the jasper requires.

  • 👍 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.

@conan-center-bot

This comment has been minimized.

Sign in to view
@ghost ghost mentioned this pull request Jan 15, 2021
Closed
@ghost
Copy link

ghost commented Jan 15, 2021 •
edited by ghost
Loading

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 Jan 15, 2021
Merged
4 tasks
@conan-center-bot
Copy link
Collaborator

Some configurations of 'opencv/4.1.2' failed in build 2 (3cc3251485c97ad21c47532e42f819ce717a8663):

@blackliner
Copy link
Contributor Author

Cannot reproduce, everything builds fine locally 🤔

@uilianries
Copy link
Member

It can not find OpenEXR because the thirdparty folder is removed. Take a look on OpenCVFindLibsGrfmt.cmake present in OpenCV 4.1.2

@ghost ghost mentioned this pull request Feb 5, 2021
Merged
4 tasks
SpaceIm
SpaceIm reviewed Feb 8, 2021
View reviewed changes
recipes/opencv/4.x/conanfile.py Outdated
Comment on lines 88 to 89
#https://github.com/opencv/opencv/issues/17984
self.requires("jasper/2.0.16")
Copy link
Contributor

@SpaceIm SpaceIm Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It should raise in build() if this condition is not honored (overridden).

@uilianries @SSE4 @danimtb @jgsogo because version range is not allowed in CCI, maybe we could allow version range of each dependency in conandata.yml, and loop over them in build() (or validate() when implemented for deps versions) with an helper function, it would be easier and we could document compatible versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this recipe needs to raise in the validate method after the graph has been resolved. The requirement to jasper can be overridden somewhere else (consumer).

I don't fully get the suggestion (that loop...). We should be able to open automatically PRs trying to upgrade dependencies (the objective would be to have a consistent graph, all packages use the same versions of the dependencies) and, of course, if this is done we need a place to write down versions that are incompatible so the bot doesn't open them over and over again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

@blackliner blackliner Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

build or validate? Its in build now, shall I move it?
Btw, when is validate executed? It is missing in the docs of conan create
https://docs.conan.io/en/latest/reference/commands/creator/create.html

Copy link
Contributor

Choose a reason for hiding this comment

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

validate is a better place for it. Without building the package Conan knows that the binary with the resulting configuration cannot be built. Of course, Conan will raise if you try to build the package, but conan info will show valuable information too:

conan info ...
...
  ID: INVALID

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that deps versions were not yet available at validate() time (only final deps options values for the moment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to validate 🤷

Copy link
Contributor Author

@blackliner blackliner Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

bad idea, that does not even work, seems that deps_cpp_info is not available/filled in validate:

ERROR: opencv/4.1.2: Error in validate() method, line 275
        if tools.Version(self.version) < "4.5.0" and self.deps_cpp_info["jasper"].version > "2.0.16":
        KeyError: 'jasper'

--> reverted

Copy link
Contributor

Choose a reason for hiding this comment

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

It should go to build() for the moment.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'opencv/4.1.2' failed in build 3 (0c558f1031022ffa986c41a1c742aebd843d38a4):

Florian Berchtold added 2 commits February 8, 2021 16:30
@conan-center-bot
Copy link
Collaborator

Failure in build 4 (60b8a73538e023d816747af1756793fc626cc7b9):
Error running command 'conan info opencv/4.1.2@ --json {jsonName} -pr {profileName}'

[settings]
arch=x86_64
arch_build=x86_64
build_type=Release
compiler=gcc
compiler.libcxx=libstdc++
compiler.version=4.9
os=Linux
os_build=Linux
[options]
shared=False

...
ERROR: opencv/4.1.2: Error in validate() method, line 275
	if tools.Version(self.version) < "4.5.0" and self.deps_cpp_info["jasper"].version > "2.0.16":
	KeyError: 'jasper'

@conan-center-bot
Copy link
Collaborator

Some configurations of 'opencv/4.1.2' failed in build 5 (f4aabe6d3d0474cf9e7199d968c62df7124764d5):

Florian Berchtold added 2 commits February 8, 2021 17:22
@conan-center-bot
Copy link
Collaborator

Some configurations of 'opencv/4.1.2' failed in build 6 (2d9db83581da0862900c08d0e8dce36a06bcb3e3):

@blackliner
Copy link
Contributor Author

This whole openexr thing wont work :-/

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 8, 2021 •
edited
Loading

Just remove all the logic in this module file and inject conan variables related to OpenEXR to variables defined in this module file and used later in OpenCV build files.
By the way, why does it work for latest opencv versions in CCI and not this one?

@conan-center-bot
Copy link
Collaborator

All green in build 7 (1cb29a8d292a77f197b7a3b0238524588bf6d216)! 😊

@blackliner
Copy link
Contributor Author

wtf happened?

@blackliner blackliner requested a review from jgsogo February 8, 2021 21:02
@prince-chrismc
Copy link
Contributor

image

@jgsogo
Copy link
Contributor

jgsogo commented Feb 9, 2021 •
edited
Loading

(Maybe this was not the question, but...)

You patched openexr (1cb29a8) and it worked... messages are associated to a commit, the success belongs to the commit about "patch openexr" while the failure belongs to the commit "patch it" (2d9db83).

When a new commit arrives we try to stop running builds, but sometimes the message from those previous jobs is posted to GitHub too. @SSE4 is having a look to GitHub API to hide old messages, it can be a bit tricky for these concurrent jobs.

@blackliner
Copy link
Contributor Author

yea that makes sense @jgsogo , building 366 packages of OpenCV might take a day or two on 4 cores 😅

@conan-center-bot
Copy link
Collaborator

All green in build 8 (1cb29a8d292a77f197b7a3b0238524588bf6d216)! 😊

@github-actions github-actions bot mentioned this pull request Feb 9, 2021
Open
@conan-center-bot conan-center-bot merged commit 4cf5149 into conan-io:master Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SpaceIm SpaceIm SpaceIm left review comments

@danimtb danimtb danimtb approved these changes

@intelligide intelligide intelligide approved these changes

@prince-chrismc prince-chrismc prince-chrismc approved these changes

@jgsogo jgsogo Awaiting requested review from jgsogo

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

Successfully merging this pull request may close these issues.
8 participants
@blackliner @conan-center-bot @uilianries @SpaceIm @prince-chrismc @jgsogo @danimtb @intelligide