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

[lapack-reference|fortran] Add new port #12805

Merged
merged 30 commits into from
Aug 12, 2020

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Aug 7, 2020

Again. New approach without touching any main files. Other PR seemed broken not running in CI.

How it works:
If Fortran compiler is on path: Use that.
If VCPKG_CHAINLOAD_TOOLCHAIN is set: Assume Fortran compiler is setup by the toolchain.

If no Fortran compiler:
Windows: Use gfortran from MinGW and create a freestanding dll. using cmakes GNUtoMS
Others: Error with a message to install a Fortran compiler.

@Neumann-A Neumann-A marked this pull request as ready for review August 7, 2020 14:34
@Neumann-A
Copy link
Contributor Author

@cenit: Feel free to review and test this one.

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 7, 2020

@cenit any opinion on using openblas on OSX? The current osx failure is due to blas.pc missing and i would need to create one for the accelerate framework.

@centi
Copy link

centi commented Aug 7, 2020

@Neumann-A To be honest, I have no idea what this is about. Could you please clarify how am I connected to this? Did I report some issue (I've searched the issues but did not find any reported by me)?

I'm not sure if I ever used this tool, but maybe I did (or some other tool which uses this as a dependency) a long time ago and forgotten about it since. Anyway, I no longer use OSX, so I cannot help with the last question either.

@Neumann-A
Copy link
Contributor Author

@centi typo i meant cenit ;)

@cenit
Copy link
Contributor

cenit commented Aug 7, 2020

@centi typo i meant cenit ;)

Go for OpenBLAS also on osx. The current situation is very comparable with the accelerate framework, or even better. This is not true for clapack, but that's the whole scope of this PR

@Neumann-A
Copy link
Contributor Author

@cbezault @vicroms: opinions?

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Aug 10, 2020
ports/clapack/portfile.cmake Show resolved Hide resolved
ports/lapack/portfile.cmake Outdated Show resolved Hide resolved
ports/openblas/portfile.cmake Show resolved Hide resolved
ports/openblas/portfile.cmake Show resolved Hide resolved
ports/vcpkg-gfortran/portfile.cmake Show resolved Hide resolved
ports/mlpack/cmakelists.patch Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Show resolved Hide resolved
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 11, 2020
@strega-nil
Copy link
Contributor

This is amazing! Thanks @Neumann-A :)

@strega-nil strega-nil merged commit f3b96f3 into microsoft:master Aug 12, 2020
@FishHe FishHe mentioned this pull request Aug 13, 2020
@Neumann-A Neumann-A deleted the new_lapack_pr branch August 13, 2020 21:02
@traversaro traversaro mentioned this pull request Aug 21, 2020
@janholt
Copy link
Contributor

janholt commented Aug 21, 2020

ports/lapack/portfile.cmake has check for find_package(LAPACK):

if(VCPKG_TARGET_IS_UWP OR VCPKG_TARGET_IS_WINDOWS AND VCPKG_TARGET_ARCHITECTURE MATCHES "arm")
    # Install clapack wrappers. 
    file(INSTALL ${CMAKE_CURRENT_LIST_DIR}/clapack/vcpkg-cmake-wrapper.cmake DESTINATION ${CURRENT_PACKAGES_DIR}/share//${PORT})
    file(INSTALL ${CMAKE_CURRENT_LIST_DIR}/clapack/FindLAPACK.cmake DESTINATION ${CURRENT_PACKAGES_DIR}/share//${PORT})
endif()
vcpkg_configure_cmake(SOURCE_PATH ${CURRENT_PORT_DIR}
                      OPTIONS -DCMAKE_PREFIX_PATH="${CURRENT_PACKAGES_DIR}")

seems it does not work in case of custom clapack/FindLAPACK.cmake installed (under the if clause): -DCMAKE_PREFIX_PATH="${CURRENT_PACKAGES_DIR}" does not participate in finding vcpkg-cmake-wrapper.cmake or FindLAPACK.cmake.

Possible solution is to use -DCMAKE_MODULE_PATH="${CURRENT_PACKAGES_DIR}/share/${PORT}"

remz1337 pushed a commit to remz1337/vcpkg that referenced this pull request Aug 23, 2020
* [vcpkg/script] add vcpkg_find_fortran

* [openblas] add pkg-config fixes

* [lapack] add lapack-reference and reduce dependency on clapack

* fix build issues

* dont touch any main files

* move toolchain var into parent scope

* fix a few more issues

* create link in the noblas case

* removed unnecessary check handled by vcpkg_find_fortran.

* move dumpbin check

* fix last issue

* depend on openblas

* set cmake_binary_dir so that compiler id run gets put into buildtree.

* more paths

* add missing PARENT_SCOPE

* ws change

* [mlpack] remove dep on clapack

* comment out patches

* remove openblas again

* Install lapack wrapper since it is missing linkage against -lm and -lgfortran

* PREPEND mingw path to make sure cmake picks it up correctly

* depend on openblas also on osx

* add clapack on windows to skip due to conflicting library installs

* add clapack to skip on linux

* add -fPIC as a fortran compiler flag

* do not add the flag on windows

* add gcc explicitly to the cmake args.

* ws change

* applyrequested changes from CR

* fix the failing patch
Comment on lines +48 to +55
if(VCPKG_CRT_LINKAGE STREQUAL "static")
set(VCPKG_CRT_LINKAGE dynamic PARENT_SCOPE)
message(STATUS "VCPKG_CRT_LINKAGE linkage for ${PORT} using vcpkg's internal gfortran cannot be static due to linking against MinGW libraries. Forcing dynamic CRT linkage")
endif()
if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
set(VCPKG_LIBRARY_LINKAGE dynamic PARENT_SCOPE)
message(STATUS "VCPKG_LIBRARY_LINKAGE linkage for ${PORT} using vcpkg's internal gfortran cannot be static due to linking against MinGW libraries. Forcing dynamic library linkage")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

This may breaks the unity of CRT linkage. Should we use vcpkg_check_linkage instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

dynamic only linkage in a static triplet cause an error. This would trigger a lot of cascade ports to fail without any notice. it’s very different from a static linkage in a dynamic triplet

while the original line is very problematic, as you say, also the fix would trigger many problems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#19413 solves all the problems with gfortran by switching to the ifort instead. CRT linkage is also not that important if memory is not passed between the different CRTs.

Comment on lines +24 to +55
set(CBLAS OFF)
if("cblas" IN_LIST FEATURES)
set(CBLAS ON)
endif()

set(USE_OPTIMIZED_BLAS OFF)
if("noblas" IN_LIST FEATURES)
set(USE_OPTIMIZED_BLAS ON)
set(pcfile "${CURRENT_INSTALLED_DIR}/lib/pkgconfig/openblas.pc")
if(EXISTS "${pcfile}")
file(CREATE_LINK "${pcfile}" "${CURRENT_PACKAGES_DIR}/lib/pkgconfig/blas.pc" COPY_ON_ERROR)
endif()
set(pcfile "${CURRENT_INSTALLED_DIR}/debug/lib/pkgconfig/openblas.pc")
if(EXISTS "${pcfile}")
file(CREATE_LINK "${pcfile}" "${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig/blas.pc" COPY_ON_ERROR)
endif()
endif()

set(VCPKG_CRT_LINKAGE_BACKUP ${VCPKG_CRT_LINKAGE})
vcpkg_find_fortran(FORTRAN_CMAKE)
if(VCPKG_USE_INTERNAL_Fortran)
if(VCPKG_CRT_LINKAGE_BACKUP STREQUAL static)
# If openblas has been built with static crt linkage we cannot use it with gfortran!
set(USE_OPTIMIZED_BLAS OFF)
#Cannot use openblas from vcpkg if we are building with gfortran here.
if("noblas" IN_LIST FEATURES)
message(FATAL_ERROR "Feature 'noblas' cannot be used without supplying an external fortran compiler")
endif()
endif()
else()
set(USE_OPTIMIZED_BLAS ON)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

@Neumann-A Is this the desired outcome of feature configuration, in particular with regard to enabling OPTIMZED_BLAS?

Requested features Static CRT & internal Fortran Otherwise Dependencies
core - OPTIMIZED_BLAS none
cblas CBLAS CBLAS, OPTIMIZED_BLAS none
noblas - OPTIMIZED_BLAS blas (->openblas)
cblas,noblas error error blas (->openblas)

I'm concerned about OPTIMIZED_BLAS being enabled for the "Otherwise" "core"/"cblas" cases. The build result will depend on the result of find_package(BLAS). I would assume OPTIMIZED_BLAS should be restricted to the noblas case so that the actual dependency can be switched in a controlled way by overriding port blas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #24327. But what vcpkg really needs is a) options RFC and b) a proper Windows Fortran compiler. (#24622)

Copy link
Contributor

Choose a reason for hiding this comment

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

That PR doesn't clearly adress the particular aspect I'm looking at: set(USE_OPTIMIZED_BLAS ON) at Line 54 regardless of features. In turn this will enable find_package(BLAS) in the project regardless of controlled dependencies. I'm proposing to turn this off. The effective result would be:

Requested features Static CRT & internal Fortran Otherwise Dependencies
core - - none
cblas CBLAS CBLAS none
noblas - OPTIMIZED_BLAS blas (->openblas)
cblas,noblas error error blas (->openblas)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah i see simply removing line 53-54 should be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants