[openblas] make openblas build on MinGW #20986

ChristopherSchwartzXRITE · 2021-10-25T16:06:37Z

What does your PR fix?

Fixes building openblas on MinGW on Windows.
The MinGW build using GCC seems to require the same explicit build flags as the non-windows systems, but it the VCPKG_TARGET_IS_WINDOWS will be set to true. Adding the extra check for NOT VCPKG_TARGET_IS_MINGW will make the portfile use the working set of build flags.
Which triplets are supported/not supported? Have you updated the CI baseline?

all + mingw, no
Does your PR follow the maintainer guide?

yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

yes

Tested with x64-mingw-static and x64-mingw-dynamic on Windows.

ghost · 2021-10-25T16:06:57Z

All CLA requirements met.

dg0yt · 2021-10-25T16:16:49Z

Tested with x64-mingw-static on Windows.

Could you also test dynamic? Note that only windows dynamic triplets are tested in CI so it is really unclear if x64-mingw-dynamic should work.

ChristopherSchwartzXRITE · 2021-10-25T16:34:55Z

I just built openblas with x64-mingw-dynamic as well and can confirm that the build succeeds and that openblas_utest.exe runs and passes all tests.

NancyLi1013

vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
        threads         USE_THREAD
        simplethread    USE_SIMPLE_THREADED_LEVEL3
)
vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
    	"dynamic-arch"      DYNAMIC_ARCH
)

Could you please add the features in one vcpkg_check_features?

NancyLi1013 · 2021-10-26T01:44:08Z

Could you please also add double quotes for the paths in this PR?

Such as "${SOURCE_PATH}" and file(INSTALL "${SOURCE_PATH}/LICENSE" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}" RENAME copyright)

Please refer to https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/cmake-guidelines.md

…ble quotes according to maintainer guidelines.

ChristopherSchwartzXRITE · 2021-10-26T07:37:42Z

vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
        threads         USE_THREAD
        simplethread    USE_SIMPLE_THREADED_LEVEL3
)
vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
    	"dynamic-arch"      DYNAMIC_ARCH
)

Could you please add the features in one vcpkg_check_features?

I will make the change as requested.
However, won't this change the logic of the portfile?
Doesn't the seconds vcpk_check_features call overwrite the FEATURE_OPTIONS variable?

I'm not the original author of the portfile, so I'm not sure if that was intentional.

NancyLi1013 · 2021-10-27T06:45:38Z

I will make the change as requested.
However, won't this change the logic of the portfile?
Doesn't the seconds vcpk_check_features call overwrite the FEATURE_OPTIONS variable?
I'm not the original author of the portfile, so I'm not sure if that was intentional.

In my opinion, it will have no effect on the function. Let's see some points from the author.

@xandox Could you please help confirm if these two functions have any effect each other?

Thanks.

xandox · 2021-10-27T09:45:42Z

@NancyLi1013 @ChristopherSchwartzXRITE Hi, I am sorry, but I am not sure what your question about?
If it is about to join or not to joint calls vcpkg_check_features in one call - IMHO better to join (I do not see any reason to split it), if it question how exactly vcpkg_check_features works - I am not a author of this function and better to see it sources.

ChristopherSchwartzXRITE · 2021-10-27T09:47:59Z

Hi @xandox . The question was about whether its OK to join the functions (or rather, if you had deliberately split them).
It seems that joining them has your blessing.

xandox · 2021-10-27T09:50:03Z

@ChristopherSchwartzXRITE Oh, really? Not, it was not a conscious decision :) Probably I did not notice first call.

dg0yt · 2021-10-27T15:17:25Z

vcpkg_check_features doesn't care for the original value of the output variable, so the second call overwrites FEATURE_OPTIONS.

#18265, merged 11 Jun, added the first call, and it added the passing of FEATURE_OPTIONS to vcpkg_configure_cmake for VCPKG_TARGET_IS_UWP only.

#15238, merged 26 Jul, added the second call, and it added the passing of FEATURE_OPTIONS to vcpkg_configure_cmake for VCPKG_TARGET_IS_WINDOWS only.

With two calls, the threading options are lost, and the DYNAMIC_ARCH option is passed to UWP and Windows.
With the merged call, threading options and DYNAMIC_ARCH are passed to both UWP and Windows.

Now what was the intention? What about non-windows targets?

CC @JackBoosY for #18265.

github-actions

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

ports/openblas/portfile.cmake

… functions.

github-actions

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

ports/openblas/portfile.cmake

… functions (2) - missed a spot.

JackBoosY · 2021-10-29T10:06:44Z

I will take a look in the next week.

JackBoosY · 2021-11-04T08:55:08Z

vcpkg_check_features doesn't care for the original value of the output variable, so the second call overwrites FEATURE_OPTIONS.

#18265, merged 11 Jun, added the first call, and it added the passing of FEATURE_OPTIONS to vcpkg_configure_cmake for VCPKG_TARGET_IS_UWP only.

#15238, merged 26 Jul, added the second call, and it added the passing of FEATURE_OPTIONS to vcpkg_configure_cmake for VCPKG_TARGET_IS_WINDOWS only.

With two calls, the threading options are lost, and the DYNAMIC_ARCH option is passed to UWP and Windows. With the merged call, threading options and DYNAMIC_ARCH are passed to both UWP and Windows.

Now what was the intention? What about non-windows targets?

vcpkg_check_features should only be called once, #18265 is my mistake.

ChristopherSchwartzXRITE · 2021-11-04T10:01:29Z

Thanks for sorting this out and fixing the broken uwp build @JackBoosY!

JackBoosY · 2021-11-05T02:47:30Z

[10/660] /usr/bin/c++ -DH5_BUILT_AS_STATIC_LIB -I/mnt/vcpkg-ci/buildtrees/shogun/src/b686d6e1cc-534b82672e.clean/src -I/mnt/vcpkg-ci/buildtrees/shogun/x64-linux-dbg/src -I/mnt/vcpkg-ci/installed/x64-linux/include/libxml2 -isystem /mnt/vcpkg-ci/installed/x64-linux/include/eigen3 -isystem /mnt/vcpkg-ci/installed/x64-linux/include -Wall -Wno-unused-parameter -Wformat -Wformat-security -Wparentheses -Wshadow -Wno-unknown-pragmas -Wno-deprecated -fPIC -g -fPIC -gsplit-dwarf -std=c++11 -MD -MT src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o -MF src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o.d -o src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o -c /mnt/vcpkg-ci/buildtrees/shogun/src/b686d6e1cc-534b82672e.clean/src/shogun/classifier/svm/NewtonSVM.cpp
FAILED: src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o 
/usr/bin/c++ -DH5_BUILT_AS_STATIC_LIB -I/mnt/vcpkg-ci/buildtrees/shogun/src/b686d6e1cc-534b82672e.clean/src -I/mnt/vcpkg-ci/buildtrees/shogun/x64-linux-dbg/src -I/mnt/vcpkg-ci/installed/x64-linux/include/libxml2 -isystem /mnt/vcpkg-ci/installed/x64-linux/include/eigen3 -isystem /mnt/vcpkg-ci/installed/x64-linux/include -Wall -Wno-unused-parameter -Wformat -Wformat-security -Wparentheses -Wshadow -Wno-unknown-pragmas -Wno-deprecated -fPIC -g -fPIC -gsplit-dwarf -std=c++11 -MD -MT src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o -MF src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o.d -o src/shogun/CMakeFiles/libshogun.dir/classifier/svm/NewtonSVM.cpp.o -c /mnt/vcpkg-ci/buildtrees/shogun/src/b686d6e1cc-534b82672e.clean/src/shogun/classifier/svm/NewtonSVM.cpp
In file included from /mnt/vcpkg-ci/buildtrees/shogun/src/b686d6e1cc-534b82672e.clean/src/shogun/classifier/svm/NewtonSVM.cpp:20:
/mnt/vcpkg-ci/buildtrees/shogun/src/b686d6e1cc-534b82672e.clean/src/shogun/mathematics/lapack.h:41:10: fatal error: cblas.h: No such file or directory
   41 | #include <cblas.h>
      |          ^~~~~~~~~
compilation terminated.

Seems shogun select cblas instead of openblas.

ChristopherSchwartzXRITE · 2021-11-12T09:23:10Z

Seems shogun select cblas instead of openblas.

This issue seems to persist.

I noticed that shogun is skipped for ci builds for all other plaforms except for linux in the ci.baseline.txt.
@NancyLi1013 Should I just add another line shogun:x64-linux = skip to the file within the scope of this PR so that we can move on?

JackBoosY · 2021-11-12T09:50:08Z

Seems shogun select cblas instead of openblas.

This issue seems to persist.

I noticed that shogun is skipped for ci builds for all other plaforms except for linux in the ci.baseline.txt. @NancyLi1013 Should I just add another line shogun:x64-linux = skip to the file within the scope of this PR so that we can move on?

Nope, we should completly solve it in this PR.
I will take a look in next week.

github-actions

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

ports/shogun/portfile.cmake

JackBoosY · 2021-11-17T07:03:36Z

I can't repro this issue because shogun found cblas.h in /usr/include/x86_64-linux-gnu/.
The reason why it happend is the header cblas.h is removed into include/openblas.
@ChristopherSchwartzXRITE Can you confirm whether this change was made by upstream?
Now it should be okay.

ChristopherSchwartzXRITE · 2021-11-17T07:54:28Z

I can't repro this issue because shogun found cblas.h in /usr/include/x86_64-linux-gnu/. The reason why it happend is the header cblas.h is removed into include/openblas. @ChristopherSchwartzXRITE Can you confirm whether this change was made by upstream? Now it should be okay.

To me it looks as if the behavior to install cblas.h into include/openblas/ instead of include was a change in the OpenBLAS repo in October 2018.
See lines 295 and 296 in the diff OpenMathLib/OpenBLAS@474f7e9#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR295.

I'm wondering why it wasn't noticed sooner 🤔 .

[openblas] make openblas build on MinGW

b6719b7

NancyLi1013 self-assigned this Oct 26, 2021

NancyLi1013 added the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label Oct 26, 2021

NancyLi1013 reviewed Oct 26, 2021

View reviewed changes

NancyLi1013 added the requires:author-response label Oct 26, 2021

Portfile cleanup: Join separate vcpkg_check_feature calls and add dou…

6dc178f

…ble quotes according to maintainer guidelines.

ChristopherSchwartzXRITE requested a review from NancyLi1013 October 26, 2021 07:38

github-actions bot approved these changes Oct 29, 2021

View reviewed changes

Portfile cleanup: Migrate vcpkg cmake commands from deprecated to new…

98e36e0

… functions.

github-actions bot approved these changes Oct 29, 2021

View reviewed changes

Portfile cleanup: Migrate vcpkg cmake commands from deprecated to new…

333df7d

… functions (2) - missed a spot.

github-actions bot approved these changes Oct 29, 2021

View reviewed changes

JackBoosY removed the requires:author-response label Oct 29, 2021

JackBoosY added 2 commits November 4, 2021 01:53

Re-factory code

c7c1fb0

version

e1c9b3d

github-actions bot approved these changes Nov 4, 2021

View reviewed changes

Merge branch 'master' into bugfix/openblas-for-mingw

bee51da

github-actions bot approved these changes Nov 12, 2021

View reviewed changes

JackBoosY added 2 commits November 16, 2021 22:19

[shogun] Fix include cblas path

54ce2f2

version

836e232

github-actions bot approved these changes Nov 17, 2021

View reviewed changes

JackBoosY mentioned this pull request Nov 17, 2021

[openblas:arm64-uwp\openblas:arm-uwp] build failure #21146

Closed

JackBoosY linked an issue Nov 17, 2021 that may be closed by this pull request

[openblas:arm64-uwp\openblas:arm-uwp] build failure #21146

Closed

JackBoosY self-assigned this Nov 17, 2021

JackBoosY approved these changes Nov 18, 2021

View reviewed changes

JackBoosY removed the request for review from NancyLi1013 November 18, 2021 09:35

JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 18, 2021

vicroms merged commit 4389f62 into microsoft:master Nov 19, 2021

ChristopherSchwartzXRITE deleted the bugfix/openblas-for-mingw branch November 22, 2021 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[openblas] make openblas build on MinGW #20986

[openblas] make openblas build on MinGW #20986

ChristopherSchwartzXRITE commented Oct 25, 2021 •

edited

Loading

ghost commented Oct 25, 2021 •

edited by ghost

Loading

dg0yt commented Oct 25, 2021

ChristopherSchwartzXRITE commented Oct 25, 2021

NancyLi1013 left a comment

NancyLi1013 commented Oct 26, 2021

ChristopherSchwartzXRITE commented Oct 26, 2021

NancyLi1013 commented Oct 27, 2021

xandox commented Oct 27, 2021

ChristopherSchwartzXRITE commented Oct 27, 2021

xandox commented Oct 27, 2021

dg0yt commented Oct 27, 2021

github-actions bot left a comment

github-actions bot left a comment

JackBoosY commented Oct 29, 2021

JackBoosY commented Nov 4, 2021

ChristopherSchwartzXRITE commented Nov 4, 2021

JackBoosY commented Nov 5, 2021

ChristopherSchwartzXRITE commented Nov 12, 2021 •

edited

Loading

JackBoosY commented Nov 12, 2021

github-actions bot left a comment

JackBoosY commented Nov 17, 2021 •

edited

Loading

ChristopherSchwartzXRITE commented Nov 17, 2021

[openblas] make openblas build on MinGW #20986

[openblas] make openblas build on MinGW #20986

Conversation

ChristopherSchwartzXRITE commented Oct 25, 2021 • edited Loading

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

ghost commented Oct 25, 2021 • edited by ghost Loading

dg0yt commented Oct 25, 2021

ChristopherSchwartzXRITE commented Oct 25, 2021

NancyLi1013 left a comment

Choose a reason for hiding this comment

NancyLi1013 commented Oct 26, 2021

ChristopherSchwartzXRITE commented Oct 26, 2021

NancyLi1013 commented Oct 27, 2021

xandox commented Oct 27, 2021

ChristopherSchwartzXRITE commented Oct 27, 2021

xandox commented Oct 27, 2021

dg0yt commented Oct 27, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

JackBoosY commented Oct 29, 2021

JackBoosY commented Nov 4, 2021

ChristopherSchwartzXRITE commented Nov 4, 2021

JackBoosY commented Nov 5, 2021

ChristopherSchwartzXRITE commented Nov 12, 2021 • edited Loading

JackBoosY commented Nov 12, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

JackBoosY commented Nov 17, 2021 • edited Loading

ChristopherSchwartzXRITE commented Nov 17, 2021

ChristopherSchwartzXRITE commented Oct 25, 2021 •

edited

Loading

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

ghost commented Oct 25, 2021 •

edited by ghost

Loading

ChristopherSchwartzXRITE commented Nov 12, 2021 •

edited

Loading

JackBoosY commented Nov 17, 2021 •

edited

Loading