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

[openexr,openimageio,suitesparse,theia] updates for non-win32 #6371

Merged
merged 42 commits into from
May 31, 2019

Conversation

cenit
Copy link
Contributor

@cenit cenit commented May 9, 2019

may require #6364 for proper build results

patches ported from #5169

@vicroms vicroms self-assigned this May 9, 2019
@vicroms
Copy link
Member

vicroms commented May 10, 2019

Hi @cenit

#6364 has gone through, perhaps merging the latest master should make the CI tests pass.

I noticed that suitesparse didn't get rebuilt, instead is failing due to a cached failure tombstone. This is the second time I have noticed this issue with suitesparse 🤔

@NancyLi1013
Copy link
Contributor

Hi @cenit, could you also bump the CONTROL version in openimageio port? I noticed that this port didn't update the version in CONTROL file.

@vicroms
Copy link
Member

vicroms commented May 14, 2019

Failure logs

x64-windows master test notes
ceres Pass Fail Regression
sophus Pass Skip
openmvg Pass Skip
g2o Pass Skip
cartographer Pass Skip

x64-windows-static had no regressions

x64-osx master test notes
theia Fail Pass
sophus Pass Fail Regression
arm64-windows master test notes
ceres Pass Skip
sophus Pass Skip
openmvg Fail Skip

x86-windows had no regressions

x64-linux master test notes
openimageio Fail Pass

x64-uwp had no regressions

arm-uwp had no regressions

@cenit
Copy link
Contributor Author

cenit commented May 14, 2019

I found out that clapack was not really working before, at least on windows: FindLAPACK from cmake failed on it and so every port using it was not properly initialized. Also suitesparse didn't find it and so was built without, and ceres[suitesparse] was silently failing (no real suitesparse feature enabled, failing gracefully to a simple ceres).
I fear this "clapack" enabling will trigger many new failures... because it will be found for real only since now...

ports/suitesparse/portfile.cmake Outdated Show resolved Hide resolved
ports/clapack/portfile.cmake Show resolved Hide resolved
@@ -0,0 +1,46 @@
include(${CMAKE_ROOT}/Modules/SelectLibraryConfigurations.cmake)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should just be a -config.cmake file, not using our vcpkg-cmake-wrapper.cmake "hack".

Copy link
Contributor Author

@cenit cenit May 15, 2019

Choose a reason for hiding this comment

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

The problem is that a -config.cmake file has lower priority than an integrated module, while our hack has higher. I’d agree with you but if I do a config then cmake will happily forget it, except when dev is explicitly requesting find_package(LAPACK CONFIG) [or NO_MODULE], which no one does

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. In that case, it would be better to "split" this into two parts:

  1. This content should be in a FindLAPACK.cmake file installed into share/clapack
  2. We should use vcpkg-cmake-wrapper.cmake to temporarily append this directory to the module path, then run the user's original find_package() command. This retains the user's ability to deploy their own find modules or config files.
# vcpkg-cmake-wrapper.cmake
set(LAPACK_PREV_MODULE_PATH ${CMAKE_MODULE_PATH})
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR})
_find_package(${ARGS})
set(CMAKE_MODULE_PATH ${LAPACK_PREV_MODULE_PATH})

This helps to ensure we get the full, proper find_package() context so that the various functions work correctly.

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 tried to implement your suggestions. Please let me know what do you think

@cenit
Copy link
Contributor Author

cenit commented May 15, 2019

#6294 should fix openmvg. May I ask you @vicroms any update on regressions?

@cenit cenit mentioned this pull request May 15, 2019
@vicroms
Copy link
Member

vicroms commented May 15, 2019

New regressions 😞

Failure logs

x64-windows master test notes
mlpack Pass Fail Regression
gamma Pass Skip
libsndfile Pass Fail Regression
aubio Pass Skip
x64-windows-static master test notes
gamma Pass Skip
libsndfile Pass Fail Regression
aubio Pass Skip
x64-osx master test notes
sophus Pass Skip
clapack Pass Fail Regression
theia Fail Skip
g2o Pass Skip
mlpack Pass Skip
dlib Pass Skip
blaze Pass Skip
geogram Pass Skip
ceres Pass Skip
ensmallen Pass Skip
arm64-windows master test notes
ceres Pass Skip
sophus Pass Skip
libsndfile Pass Fail Regression
openmvg Fail Skip
x86-windows master test notes
gamma Pass Skip
libsndfile Pass Fail Regression
aubio Pass Skip
x64-linux master test notes
sophus Fail Skip
clapack Pass Fail Regression
openimageio Fail Pass
dlib Pass Skip
blaze Fail Skip
suitesparse Pass Skip
geogram Fail Skip
ceres Pass Skip
g2o Pass Skip
x64-uwp master test notes
libsndfile Pass Fail Regression
aubio Pass Skip
arm-uwp master test notes
libsndfile Pass Fail Regression
aubio Pass Skip

@cenit
Copy link
Contributor Author

cenit commented May 15, 2019

Exactly what I feared. No one was really using clapack... but I have to open logs later (on the phone now) to understand failures of clapack itself. I tried it on few architectures and it was working 🤔

@cenit
Copy link
Contributor Author

cenit commented May 15, 2019

ok I found some errors which should fix many problems. I hope for the best!

@cenit
Copy link
Contributor Author

cenit commented May 16, 2019

@vicroms please give me some nice news...

@cenit
Copy link
Contributor Author

cenit commented May 30, 2019

@vicroms I can see from public CI that x64-linux has failures for openmvg and sophus. I can't reproduce them locally, but I can't see logs from public CI, so I am lost.
Please let me know if the same result can be reproduced from internal CI, with logs in that case.
Thanks!
(Otherwise, apart from that, for me this PR should be finished)

@vicroms
Copy link
Member

vicroms commented May 30, 2019

Waiting for CI results.

@cenit
Copy link
Contributor Author

cenit commented May 30, 2019

I just saw that public CI was lamenting a failed thombstone even if I bumped the control just to trigger a rebuild... now I modified port file a little bit to see if it kicks it harder...

@cenit
Copy link
Contributor Author

cenit commented May 30, 2019

@vicroms thanks, modifying port file now it tried a rebuild of sophus but failed. I am unable to see how to download logs and also can't reproduce it locally, so I need your help

@vicroms
Copy link
Member

vicroms commented May 30, 2019

Hi @cenit

Glad to help!

I see a green check for this PR on Linux, which would explain why no logs were published. Looks like a failed sophus build is not considered a regression, and when there are no regressions no failure logs are published.

I also noticed that the public Windows CI is missing the Analyze Results job in the latest build. So it is also missing logs.

For now, we'll have to wait for build 2721208 in the private CI to finish and get the logs.

You may already know them, but the important things to review in CI builds are:

  • In the Logs tab:
    • The output from the vcpkg ci command for each triplet:
      • ** Test modified ports ** in vcpkg-Linux-PR
      • ** Build vcpkg and test ports ** in vcpkg-Windows-PR
  • In the Summary tab
    • The <triplet>Differences from Baseline sections at the end of the summary page that contain a regression/results table.
    • The Build artifacts published section that will contain ZIP files with the log files if a regression occurred.

NOTE: These may change as @Rastaban keeps working on the public CI.

@cenit
Copy link
Contributor Author

cenit commented May 30, 2019

ok thanks.
Anyway, it might not be considered a regression but sophus has to work after this pr. Also, it works on an ubuntu 16.04 machine here (with gcc-7) without any problem, so maybe it's just something stupid easily fixable...

@cenit
Copy link
Contributor Author

cenit commented May 31, 2019

Green mark? 😃

@vicroms
Copy link
Member

vicroms commented May 31, 2019

Green mark indeed, but the logs in CI look suspicious.
I'll run a new CI build just to make sure...

Results are legit, here are the affected packages:

x64-osx test notes
sophus Pass
clapack Pass
theia Pass
blaze Pass
geogram Pass
ceres Pass
g2o Pass
arm64-windows test notes
armadillo Skip Missing port dependency
x86-windows test notes
armadillo Skip Missing port dependency
x64-linux test notes
shogun Pass
blaze Pass
clp Pass
osi Pass
openimageio Pass
coinutils Pass
theia Pass
cartographer Pass
openmvg Pass

@vicroms vicroms merged commit 5898891 into microsoft:master May 31, 2019
@cenit
Copy link
Contributor Author

cenit commented Jun 1, 2019

Too many fixed ports? 🤣

@cenit cenit deleted the dev/cenit/theia branch June 1, 2019 06:23
@JackBoosY
Copy link
Contributor

Thank you for your contribution. : )

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.

6 participants