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

hwloc update to 2.6 (it has native CMake support) #20899

Closed
bgoglin opened this issue Oct 21, 2021 · 26 comments · Fixed by #22060
Closed

hwloc update to 2.6 (it has native CMake support) #20899

bgoglin opened this issue Oct 21, 2021 · 26 comments · Fixed by #22060
Assignees
Labels
category:port-update The issue is with a library, which is requesting update new revision

Comments

@bgoglin
Copy link

bgoglin commented Oct 21, 2021

Library name: hwloc
New version number: 2.6

Hello
I am the upstream maintainer of hwloc. I was told about this project and we had some users requesting native CMake support for building hwloc on Windows. So I took your CMakeLists.txt as a base, updated it to hwloc 2.6 and added some additional features. See open-mpi/hwloc#484

Differences between your CMakeLists.txt and mine:

  • I commented-out your snprintf/strtoull reenabling because we had some issues with them in the past, and I don't know how to make sure we're not running on such old/broken build environment. It shoudn't hurt much anyway.
  • I didn't apply any change that is currently in portfile.cmake because I am not why they are needed and why they are not in CMakeLists.txt
  • all tools and lstopo are now built but may be disabled with HWLOC_SKIP_LSTOPO and HWLOC_SKIP_TOOLS
  • I install these tools in bin/ instead tools/hwloc/ (if you need a way to customize that, let me know).
  • dolib.c isn't needed inside the library, it's an external tool that helps linking the library on MinGW

If you have some feedback before I release hwloc 2.6 officially next week, I'll be happy to update the CMakeLists. Otherwise we'll update it in future releases.

In the future, I'd like to be able to rename libhwloc.dll into libhwloc-15.dll to match other builds. If there's a need to disable that, let me know.

@Neumann-A
Copy link
Contributor

Is the CMakeLists.txt limited to windows or can it be used on all platforms? If it is limited to windows: Why is it limited?

@bgoglin
Copy link
Author

bgoglin commented Oct 21, 2021

It's only for Windows. For other platforms, autoconf works fine, duplicating everything in cmake would require a lot of work, and nobody requested it.

@Neumann-A
Copy link
Contributor

hmm vcpkg can also deal with autoconf on windows.
Furthermore: set(CMAKE_C_FLAGS_DEBUG "-DHWLOC_DEBUG=1") is wrong.
Seems like the CMakeLists.txt in vcpkg could use a serious overhaul (e.g. cmake_minimum_required(VERSION 3.0) should be something newer). Maybe it is better to just switch the vcpkg port to use autotools if hwloc can correctly work with that on windows. It seems like the port was added way before that was possible.

@bgoglin
Copy link
Author

bgoglin commented Oct 21, 2021

autoconf builds work under MinGW/MSYS and Cygwin. We've been using the former for 10 years for publishing binary releases. Cygwin is more recent and much easier to deploy. Both support pretty much everything we have on Unix, including make check, etc. And both are very slow but we are still able to test build nightly.

We distribute a basic MSVC solution but it's mostly outdated now. That's why @emmenlau asked us to ship a CMakeList based on your work. We'll keep it even if you switch to autoconf.

What's wrong with CMAKE_C_FLAGS_DEBUG? I am a beginner at CMake, and I was actually trying to understand why debug flags are always enabled in one CI slave :)

@Neumann-A
Copy link
Contributor

Neumann-A commented Oct 21, 2021

autoconf builds work under MinGW/MSYS and Cygwin

Then it should also work native on Windows.

Already got it through configure. Just hitting:
E:\vcpkg_folders\master\buildtrees\hwloc\src\febfb88078-d5554303bb\include\private/misc.h(32): fatal error C1189: #error: "unknown size for unsigned long."
So the configure check for that is probably wrong.
scrap that I still have not correctly forced the check for the C99 compiler. ac_cv_prog_cc_c99='' didn't work correctly

@Neumann-A
Copy link
Contributor

hmm funny error:

.././../src/febfb88078-d5554303bb.clean/hwloc/topology.c(73): error C2061: syntax error: identifier '__attribute__'
.././../src/febfb88078-d5554303bb.clean/hwloc/topology.c(73): error C2059: syntax error: ';'

HWLOC_HAVE_ATTRIBUTE_CONSTRUCTOR is set to 0 so this shouldn't happen. Probably a preprocessor bug?

@Neumann-A
Copy link
Contributor

@bgoglin I think your #ifdef's are all wrong. most of them should be #if. You want #if HWLOC_HAVE_ATTRIBUTE_CONSTRUCTOR and not #ifdef HWLOC_HAVE_ATTRIBUTE_CONSTRUCTOR because the latter will be true if #define HWLOC_HAVE_ATTRIBUTE_CONSTRUCTOR 0. Unfortunately, it is consistently done wrong in the sources of hwloc.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 21, 2021

Sidenotes:

  • When maintaining a port, it is nice to have to deal with only one build system on all platforms. Both cmake and autotools can do that.
  • On Windows, autotools is slow. It is also limited to generating Makefiles, while CMake allows to leverage Ninja for faster builds.
  • When using documented CMake variables, think twice if they are meant to come from the toolchain, the user, or from the project. Don't override incoming configuration.

@emmenlau
Copy link

I just wanted to chime in here. First, thanks a lot for the nice work! The CMakeLists.txt from vcpkg was a very good start for me to get hwloc building on Windows with Ninja, and @bgoglin could adopt it quite quickly to the latest version.

Its also awesome that vcpkg can use autotools nowadays. But just to ask, if hwloc would have native cmake support for Windows is it still reasonable to adopt the autotools build? I can see that having a single build system provides some benefits, but then again autotools has its downsides on Windows, so I hope its fair to at least ask this question...

@Neumann-A
Copy link
Contributor

So i get hwloc 2.5 to build nativ on windows for x64-windows-static. The x64-windows build fails with: LINK : fatal error LNK1104: cannot open file '.libs/libhwloc.def'. The reason for that is that somewhere hwloc is passing --output-def for that. Have to check how fontconfig solved shared libraries in the past.

@Neumann-A
Copy link
Contributor

So #20905 updates hwloc to 2.5 an switches it to autotools. The advantage here clearly is that all platforms are the same and pkgconfig files get generated as well as all executables. Furthermore arm builds work now.

@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Oct 22, 2021
@JackBoosY
Copy link
Contributor

Dear maintainer,
We want to use the cmake build system as much as possible, however, it is not perfect (not support all platforms), so we decided to use the makefile first, and adopt it after perfecting it.
Thanks for your attention and support to vcpkg, can I add you to the maintainer list?

@bgoglin
Copy link
Author

bgoglin commented Oct 23, 2021

@JackBoosY Sure, use whichever build system you want. There's no plan to switch away from autotools on Unix in upstream hwloc, and we'll look at adding autotools on Windows in our CI in the future.
I guess I am fine being added to the maintainer list as long as I don't get too many messages/notifications :)

@JackBoosY
Copy link
Contributor

@bgoglin According to our doc, can you please leave your email here?

@bgoglin
Copy link
Author

bgoglin commented Oct 25, 2021

firstname.lastname at inria.fr

@FunMiles
Copy link

FunMiles commented Jan 30, 2023

@bgoglin I would like to know if a more complete CMake system could be made? I use CMake exclusively for my code development and previously integrated hwloc via Conan. Unfortunately, conan is in transition from v1 to v2 and they temporarily stopped maintaining the plugin from my IDE (CLion). To avoid some pain, I decided to use the FetchContent feature of CMake to bring hwloc in my latest project. I failed at doing it with the autotools system and had to create a fork with a CMakeLists.txt for Mac OS. My fork is at https://github.com/FunMiles/hwloc
Compared to the version you made for Windows, created a MacOS CMakeLists.txt with a few things updated to more modern/flexible CMake. It would be great if that same file could handle at least Windows and Linux as well. I can easily test/tune for Mac and Linux but am not well versed with Windows to do much good.

Having a correct CMakeLists.txt allows to consume hwloc in CMake projects (with CMake >= 3.24) very easily:

    include(FetchContent)

    FetchContent_Declare(
            hwloc
            GIT_REPOSITORY https://github.com/FunMiles/hwloc.git
            GIT_TAG 6e47ca0d1f86fc14fbc5fef982618d5b5f48054c # based on release-2.8.0
            OVERRIDE_FIND_PACKAGE
    )
    find_package(hwloc)
    target_link_library(my_executable PRIVATE hwloc)

@bgoglin
Copy link
Author

bgoglin commented Jan 30, 2023

@FunMiles yes that's possible, but I am not good at CMake, hence it'll be good to keep it simple (so that I can maintain it without too much trouble)

@FunMiles
Copy link

FunMiles commented Jan 30, 2023

@bgoglin I can help a lot with CMake. I have not dug into much of how hwloc works however, so when things break I will probably need help. My new code using the approach outlined above compiles and link but crashes on an assert, which is of course frustrating...

assert(hwloc_filter_check_keep_object(topology, obj));

To be honest, I think the most difficult part is the configuration files. If I understood how all the information needed to create those files is done with autotools, I could probably do it to cover almost all cases with CMake.

@FunMiles
Copy link

FunMiles commented Jan 31, 2023

@bgoglin I am making a new post as I know you won't get notified if I edit the above.
It seems that the MacOS version of hwloc will crash in the above assert if compiled with HWLOC_DEBUG set to 1.
It happens with the autotools compiled version as well. Should I make a new issue about it? I am turning that flag off for my fork.

@JackBoosY
Copy link
Contributor

I can provide some help on cmake side if you guys need.

@bgoglin
Copy link
Author

bgoglin commented Jan 31, 2023

@FunMiles I cannot reproduce the failed assert on our old mac that runs the CI, neither on a M1. Can you open a proper issue at https://github.com/open-mpi/hwloc/issues ? One thing to try first: rebuild without debug, check that lstopo works, then export HWLOC_DEBUG_CHECK=1 in the environment and check that lstopo fails the assert. If so, do "lstopo foo.xml" without the export and attach foo.xml to the issue.

We may also want to move the reminder of the issue to another issue at https://github.com/open-mpi/hwloc/issues to avoid discussion no-vcpkg-related things here.

Regarding the configuration file, you may want to ignore a lot of autotools tests because those are related to operating systems that are old or likely won't care about CMake anytime soon. I can try to build a list of things you really want to detect for MacOS.

@Neumann-A
Copy link
Contributor

Please move the discussion. This is really not a vcpkg topic ;)

@FunMiles
Copy link

@bgoglin I opened an issue on hwloc repository demonstrating how to trigger the issue. What is the proper way to start a discussion on the CMakeLists.txt over there? @JackBoosY 's help would be welcome.
@Neumann-A Agreed 😄 . We'll work on this on the hwloc side.

@bgoglin
Copy link
Author

bgoglin commented Jan 31, 2023

@FunMiles the cmake discussion should just be another issue on hwloc's github, thanks.

@JackBoosY
Copy link
Contributor

@FunMiles Can you please cc me on that issue or provide the issue link here?

@FrankXie05
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants