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

[msmpi] build from source #13218

Closed

Conversation

albertziegenhagel
Copy link
Contributor

@albertziegenhagel albertziegenhagel commented Aug 29, 2020

This MR changes the MSMPI port to build the library from source instead of unpacking the binary installers.

This has multiple benefits:

  • We do not need to rely on the MSMPI redistributable package being installed on the system.
  • The port now provides both, the MSMPI SDK (headers and import libraries) and the redistributable package (DLLs and the tools like mpiexec.exe). This allows msmpi.dll to be picked up by the app-local deployment mechanisms of vcpkg.
  • We can patch the MSMPI source code to make it usable by gfortran. See 10.0 not usable in MSYS/mingw64 with gfortran Microsoft-MPI#7.

Open questions for the port:

  • Building MSMPI requires the Windows SDK as well the WDK being installed. I could not figure out yet how to check whether they are installed in vcpkg. While there is vcpkg_get_windows_sdk as far as I understand it does not really check whether it is installed. I could not find anything for the WDK.

Maybe it would be possible to build MSMPI without the WDK being installed. I have opened an issue in the MSMPI repository about that here: microsoft/Microsoft-MPI#48.

Additionally the port will install the static libraries msmpifec.lib, msmpifmc.lib, msmpifes.lib and msmpifms.lib again, that have been removed from the port in #6945. Those libraries are actually required when consuming MSMPI from Fortran. A good explanation can be found in the CMake FindMPI.cmake module: https://gitlab.kitware.com/cmake/cmake/-/blob/670672f10e8b720d42b44fad80472805efa9ec00/Modules/FindMPI.cmake#L898-918

MRs to upstream the patches applied in the port can be found here:

@MVoz
Copy link
Contributor

MVoz commented Aug 29, 2020

ms-mpi deps: networkdirect

file(TO_NATIVE_PATH "${SDK_ARCHIVE}" SDK_ARCHIVE)
file(TO_NATIVE_PATH "${SOURCE_PATH}/sdk" SDK_SOURCE_DIR)
file(TO_NATIVE_PATH "${CURRENT_BUILDTREES_DIR}/msiexec-${TARGET_TRIPLET}.log" MSIEXEC_LOG_PATH)
vcpkg_acquire_msys(MSYS_ROOT PACKAGES "mingw-w64-${MSYS_TARGET}-gcc-fortran")
Copy link
Contributor

@MVoz MVoz Aug 29, 2020

Choose a reason for hiding this comment

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

(x86 doesn't work))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSMPI seems to support x86 just fine (see the current CI results). The binary distribution always includes a 32bit and 64bit version as well. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

nothing is visible in the logs, well, if it works, let it work

Some projects might have *.lib/*.dll files in their directories that are not actually build artifacts of the projects themselves. This change allows to specify a sub-directory for vcpkg_install_msbuild, so that only binaries from that directory get installed. This behaves similar to the already present `INCLUDES_SUBPATH`.
@albertziegenhagel
Copy link
Contributor Author

As discussed above, I have added a patch to the MSMPI source code that replaces the usage of MessageCompile by a custom build step that invokes mc.exe manually. This allows to build MSMPI without the WDK. This patch makes the CI for x64-windows and x86-windows succeed.

x64-windows-static does not work, since MSMPI does not actually support being build as static library. This can not be resolved by just replacing the configuration type DynamicLibrary by StaticLibrary. Maybe one could patch some additional files to make it build as static library, but I am not sure what implications that would have, since I don't think MSMPI was designed to be build as static library.
As a side note: MSMPI did not really support the static triplet before these changes as well. Since only the SDK, but not the runtime has been extracted by the portfile, the static triplet did only install the the import library for the DLL, but not the DLL itself. That is why the post-build checks succeeded.

@MVoz
Copy link
Contributor

MVoz commented Aug 30, 2020

I think we need to add the assembly of another package, as a dependency of this -->> networkdirect

@albertziegenhagel
Copy link
Contributor Author

I think we need to add the assembly of another package, as a dependency of this -->> networkdirect

Good catch. I missed it since it is not listed as dependency and NuGet just picked it up, unnoticed by me. I modified the port to use the networkdirect-sdk version from vcpkg.

@MVoz
Copy link
Contributor

MVoz commented Aug 31, 2020

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please update the networkDirect-sdk version info. See documentation.

ports/networkdirect-sdk/portfile.cmake Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

-- Note: msmpi only supports dynamic library linkage. Building dynamic library.
CMake Error at scripts/cmake/vcpkg_check_linkage.cmake:42 (message):
  Refusing to build unexpected dynamic library against the static CRT.  If
  this is desired, please configure your triplet to directly request this
  configuration.
Call Stack (most recent call first):
  ports/msmpi/portfile.cmake:3 (vcpkg_check_linkage)
  scripts/ports.cmake:79 (include)

@JackBoosY JackBoosY added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:author-response labels Aug 31, 2020
@albertziegenhagel
Copy link
Contributor Author

Please update the networkDirect-sdk version info. See documentation.

Sorry, I have updated the port-version now.

-- Note: msmpi only supports dynamic library linkage. Building dynamic library.
CMake Error at scripts/cmake/vcpkg_check_linkage.cmake:42 (message):
Refusing to build unexpected dynamic library against the static CRT. If
this is desired, please configure your triplet to directly request this
configuration.
Call Stack (most recent call first):
ports/msmpi/portfile.cmake:3 (vcpkg_check_linkage)
scripts/ports.cmake:79 (include)

This is intended (see this comment above). I am not sure what to do about it. I think one would need to update the ci.baseline.txt, but I was not sure whether I should add msmpi as fail or skip in this case.

@JackBoosY
Copy link
Contributor

@albertziegenhagel Add msmpi:x64-windows-static=fail to ci.baeline.txt.

@albertziegenhagel
Copy link
Contributor Author

@albertziegenhagel Add msmpi:x64-windows-static=fail to ci.baeline.txt.

@JackBoosY Thanks for the info! I've updated the CI baseline.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines requires:discussion and removed requires:author-response labels Aug 31, 2020
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Why are you disabling SafeSEH and CFG?

@albertziegenhagel
Copy link
Contributor Author

Why are you disabling SafeSEH and CFG?

I had to disable CFG because due to it being enabled, one gets undefined symbols to __guard_check_icall_fptr when MSMPI is used by gfortran. See microsoft/Microsoft-MPI#7. Since gfortran is now the "official"/"default" Fortran compiler on Windows in vcpkg this was necessary to support other ports (i.e. ScaLAPACK, an MPI parallel version of LAPACK written in Fortran) that I want to submit a Merge Request for after this one gets merged.

And I had to disable SafeSEH (for x86 builds) because without it being disabled I got LNK1281: Unable to generate SAFESEH image. After some research this seems to happen because the one object that is build by gfortran is missing the necessary data. The only way I found to fix this was to disable SafeSEH. And since MSMPI is mostly written in C and very little Fortran, an option about exception handling shouldn't make that much of a difference, but I might be wrong about this one. But to be honest, I don't know how the x86 Version of the official MSMPI distribution is build without disabling SafeSEH, so I might be missing some other solution to this problem.

@BillyONeal BillyONeal added depends:upstream-changes Waiting on a change to the upstream project and removed requires:discussion info:reviewed Pull Request changes follow basic guidelines labels Aug 31, 2020
@BillyONeal
Copy link
Member

Unfortunately while I would like to see fewer binary dependencies I don't think we can in good faith merge something that explicitly breaks Security Development Lifecycle rules :/

@ras0219
Copy link
Contributor

ras0219 commented Sep 1, 2020

Note that this is likely impacted by #13019

@albertziegenhagel
Copy link
Contributor Author

@BillyONeal I can totally understand this. I have removed the patches to disable CFG and SAFESEH from the MR. The CFG patch is only required for downstream Fortran projects which do not yet exist in vcpkg. I will try to find a fix for missing SAFESEH data in the Fortran object file. Feel free to close this MR until a fix is found. I will reopen it as soon as I can present an acceptable solution.

@ras0219 Do you have any suggestion how I should incorporate the changes of #13019 into this MR? I noticed that the packages in the vcpkg_find_fortran.cmake are now all listed explicitly. I don't think duplicating all of this information into the msmpi portfile is a good idea, but I can not use vcpkg_find_fortran here, since MSMPI explicitly requires gfortran, even when a different Fortran compiler has been specified in the users toolchain file.

@cenit
Copy link
Contributor

cenit commented Sep 1, 2020

is it possible to add a flag to vcpkg_find_fortran to force GFORTRAN?

@ras0219
Copy link
Contributor

ras0219 commented Sep 3, 2020

A flag in vcpkg_find_fortran to force GFORTRAN would be reasonable to me, however depending on how this exactly interacts I think it may be better to not force gfortran.

If it is critical that this library uses the exact same fortran compiler as every other fortran port in your tree, then this port should NOT force gfortran. It should call vcpkg_find_fortran() and (possibly) error if it finds the results unacceptable. Users can then make the right decision for their more complex environment.

If the fortran compiler used by this port is completely decoupled from every other port in the tree, then it's reasonable for this port to hardcode a specific fortran compiler so that changes to other ports or the core infrastructure has no chance of breaking it.

@JackBoosY
Copy link
Contributor

Pinging @albertziegenhagel for response. Is work still being done for this PR?

@albertziegenhagel
Copy link
Contributor Author

Pinging @albertziegenhagel for response. Is work still being done for this PR?

@JackBoosY Sorry for the very late answer... I am not currently working on this PR, so I will close it for the time being and might reopen it as soon as I have time to continue working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly depends:upstream-changes Waiting on a change to the upstream project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants