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

Update for supporting OpenCV 4.1.1 #8

Merged
merged 8 commits into from
Sep 18, 2019
Merged

Update for supporting OpenCV 4.1.1 #8

merged 8 commits into from
Sep 18, 2019

Conversation

kumaakh
Copy link

@kumaakh kumaakh commented Sep 9, 2019 •
edited
Loading

bkotzz
bkotzz reviewed Sep 10, 2019
View reviewed changes
Copy link
Member

@bkotzz bkotzz left a comment

Choose a reason for hiding this comment

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

Nice work!

cmake/projects/OpenCV/hunter.cmake Outdated
VERSION
"4.1.1"
URL
"https://github.com/kumaakh/hunter-opencv/archive/hunter-4.1.1.zip"
Copy link
Member

Choose a reason for hiding this comment

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

Let’s get the PR against the cpp-pm fork merged first, and then we can release that and update this link to be the cpp-pm release

@kumaakh
Copy link
Author

kumaakh commented Sep 10, 2019

Nice work!
@headupinclouds mentored me on hunter and also helped me to resolve the last issues. Thanks David.

@bkotzz bkotzz mentioned this pull request Sep 10, 2019
Merged
@rbsheth
Copy link
Member

rbsheth commented Sep 11, 2019

Yup, like @bkotzz said, let's get cpp-pm/opencv#1 merged and released first and then we can update this PR before merge. Thanks for your contribution!

@xsacha
Copy link

xsacha commented Sep 11, 2019 •
edited
Loading

Just a note, I have already done OpenCV 4.1.1 in my own hunter fork:
https://github.com/xsacha/opencv/tree/hunter-4.1.1
based on 4.0.0 with minimal changes (it was just a rebase). This change looks like it does the same rebase.

I am already using this in production.
Will be happy to see this merged :)

@rbsheth
Copy link
Member

rbsheth commented Sep 16, 2019

@kumaakh I merged your PR in the OpenCV fork and published a release: https://github.com/cpp-pm/opencv/releases/tag/v4.1.1-p0

Please update this PR to use that release.

@kumaakh
Copy link
Author

kumaakh commented Sep 17, 2019

Thanks Rahul,
I have updated the PR with the correct archive and SHA etc.
Where is the cpp-pm equivalent of "https://github.com/ingenue/hunter" ? AKA where should I merge the CI patch ?

@bkotzz
Copy link
Member

bkotzz commented Sep 17, 2019

@kumaakh we haven’t moved that over yet. You should be able to use the ingenue one for now. I’m assuming you won’t need to make changes to it

@kumaakh
Copy link
Author

kumaakh commented Sep 17, 2019 •
edited
Loading

AFAIK We'll have to merge these changes to pkg.opencv branch of "https://github.com/ingenue/hunter" to get clean CI badge

@bkotzz
Copy link
Member

bkotzz commented Sep 17, 2019

The main idea is, you need a temporary branch that has the changes you made from this branch, plus the appveyor/Travis changes from the pkg.opencv branch, and push it remotely so that CI runs. You can merge this one into that one, that one into this one, or just checkout those two files into this one. Let me know if you are blocked for any reason

@kumaakh
Copy link
Author

kumaakh commented Sep 17, 2019

@bkotzz thanks for clarifying.

I submitted a PR to ingenue : ingenue#392
And it has kicked off the jobs at appveyor and travis

rbsheth
rbsheth reviewed Sep 17, 2019
View reviewed changes
Copy link
Member

@rbsheth rbsheth left a comment

Choose a reason for hiding this comment

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

Looks like the Travis jobs errored due to a timeout but the ones that didn't are working. Can you address the small nit? Then we should be good to go.

cmake/projects/OpenCV/hunter.cmake Outdated
VERSION
"4.1.1-p0"
URL
"https://github.com/cpp-pm/opencv/archive/v4.1.1-p0.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can you remove the extra space before the URL?

Copy link
Author

Choose a reason for hiding this comment

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

Done with NIT

@kumaakh
Copy link
Author

kumaakh commented Sep 18, 2019

Wondering how to avoid the timeouts issue on travis CI. Would a second run work faster due to cacheing ?

@rbsheth
Copy link
Member

rbsheth commented Sep 18, 2019

@kumaakh I don't think cache would help, since the OpenCV package didn't complete running but the dependencies should already have been cached. Taking it on faith right now. 🤞

@rbsheth rbsheth merged commit f56e321 into cpp-pm:master Sep 18, 2019
@rbsheth
Copy link
Member

rbsheth commented Sep 18, 2019

Released: https://github.com/cpp-pm/hunter/releases/tag/v0.23.216

@rbsheth rbsheth mentioned this pull request Sep 18, 2019
Closed
dan-42 pushed a commit to dan-42/hunter that referenced this pull request Oct 23, 2019
@kumaakh @dan-42
Update for supporting OpenCV 4.1.1 (cpp-pm#8)
932124b 
* Update for supporting OpenCV 4.1.1

* Update for supporting OpenCV 4.1.1

* sha update

* point to https://github.com/cpp-pm/opencv/archive/v4.1.1-p0.tar.gz

* OpenCV version 4.1.1-p0

* fix spaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbsheth rbsheth rbsheth left review comments

@bkotzz bkotzz bkotzz approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@kumaakh @rbsheth @xsacha @bkotzz