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

[BUG] Build broken on Catalina and below: linking errors, unconditional usage of non-existing headers #682

Open
barracuda156 opened this issue Dec 10, 2023 · 19 comments
Assignees
Labels
bug Something isn't working

Comments

@barracuda156
Copy link

Fails to build on Catalina and below: https://trac.macports.org/ticket/68871

On El Capitan down this fails (at least):

src/osx/sensors.cpp:22:10: fatal error: 'IOKit/hidsystem/IOHIDEventSystemClient.h' file not found
#include <IOKit/hidsystem/IOHIDEventSystemClient.h>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/osx/sensors.cpp:22:10: note: did not find header 'hidsystem/IOHIDEventSystemClient.h' in framework 'IOKit' (loaded from '/System/Library/Frameworks')
1 warning and 1 error generated.
make: *** [obj/osx/sensors.o] Error 1

On Catalina through Sierra linking fails:

Linking and optimizing binary...
undef: __ZNSt3__120__libcpp_atomic_waitEPVKvx
undef: __ZNSt3__123__libcpp_atomic_monitorEPVKv
undef: __ZNSt3__123__cxx_atomic_notify_oneEPVKv
Undefined symbols for architecture x86_64:
  "std::__1::__libcpp_atomic_wait(void const volatile*, long long)", referenced from:
      Runner::_runner(void*) in lto.o
  "std::__1::__libcpp_atomic_monitor(void const volatile*)", referenced from:
      Runner::_runner(void*) in lto.o
  "std::__1::__cxx_atomic_notify_one(void const volatile*)", referenced from:
      Runner::run(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, bool, bool) in lto.o
      Runner::stop() in lto.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [btop] Error 1

I will test on PowerPC tomorrow, but expect the same headers errors at the very least.

@barracuda156 barracuda156 added the bug Something isn't working label Dec 10, 2023
@imwints
Copy link
Contributor

imwints commented Dec 11, 2023

I had the same error when I tested the CI file for the cmake build. I don't remember exactly what thw issue was, but from the top of my head I would say your linker doesn't know where your standard C++ library is. When you install llvm through homebrew for example the installation prints out addition LDFLAGS to append to the linker, like -L/usr/local/lib/...

@barracuda156
Copy link
Author

@imwints If that was the case, the build would fail on all macOS versions, but it does not: https://ports.macports.org/port/btop/details (see port health)

@imwints
Copy link
Contributor

imwints commented Dec 11, 2023

@barracuda156

Mmh yeah,I didn't look close enough. I reproduces a similar report with another std::__1::__libcpp... symbol, it happens when omitting the -stdlib=libc++ flag. It is likely that the ld command doesn't know about the standard library you compiler is using by default. And then don't forget to tell it where the standard library is, when it's in a non-default directory

@imwints
Copy link
Contributor

imwints commented Dec 11, 2023

And for the first issue, I'd be interested if the error is reproducible with cmake aswell, because in comparison to the make build cmake adds some macOS sdk paths by default

Edit:
Ok the macports build also passes the sdk flags so this shouldn't be an issue

@barracuda156
Copy link
Author

-stdlib=libc++

@imwints MacOS from 10.7 onwards has it own native libc++, so location will be a default one (in sysroot), unless it is explicitly modified (which is possible via legacysupport port group, for example, but seldom needed).

Normally the compiler will pick correct flags to link with its standard runtime library. If it was not the case, everything would direly fail to link, and we would need to pass flags manually every time. (It is not impossible however that specifically here something breaks linking, or triggers some obscure bug in clang etc.)
I can try switching from Apple clang to LLVM one to see if that makes any difference.

@imwints
Copy link
Contributor

imwints commented Dec 11, 2023

You can have a look here for LLVM

I still would try to be verbose about some options, probably

LDFLAGS="-stdlib=libc++ -v"

and have a look if properly linked with -lc++ and the directories point to the wanted library

@barracuda156
Copy link
Author

@imwints The link you refer is a build on macOS-12 (for some reason using mismatching SDK), unless I miss something. We have no failures on macOS-12, so it is not expected to fail. Could you confirm the build works on 10.15 on your end with any compiler?

@imwints
Copy link
Contributor

imwints commented Dec 12, 2023

for some reason using mismatching SDK

That is good to know, I have no experience with macOS at all, I just played around in CI with it and got it working somehow. (I also feel a bit unqualified answering here). I don't know how I would update or change the SDK.

From the github docs I read that there are only runners for macOS 11 through 13 and I don't have access to Apple hardware... :(

@barracuda156
Copy link
Author

@imwints My point was that we cannot infer from success of builds on macOS 11+ (which are successful both on your CI and our buildbots) that anything prior to macOS 11 shall automatically work, and if it does not, there is something wrong with Macports setup (something could be wrong, as always, but AFAICT it is unlikely here).
I have no testing system for Intel at hand, but I can set it up with Catalina to deal with linking issues. (Since the cause of the error is unclear at the moment, I cannot really make a PR with a blind attempt to fix it, since our CI will not reveal relevant outcomes either, it will be clear only from buildbots.)

@aristocratos
Copy link
Owner

@barracuda156
Brew has a btop build for Catalina, and as far as I remember when we implemented MacOS support, Catalina was the "lowest" possible version I got it to build on.

Not sure if @joske got it to build on anything lower back then?

Might be that changes made since then to for example sensor detection for M1 macs might have hardened the dependencies even more.

I have however never documented a minimum requierd MacOS version, since I haven't invested enough time in exploring if it's possible to get it compiling on Mojave and below.

@joske
Copy link
Collaborator

joske commented Dec 12, 2023

I don't remember what version of macOS I was using at the time. I don't have access to anything below Monterrey ATM. I do have an old El Capitan macbook, but it just can't compile btop (IIRC could not install dependencies via brew, as El Capitan is just too old and unsupported).

@barracuda156
Copy link
Author

So immediate issues are the following:

  1. Non-existing header here:
Compiling src/osx/btop_collect.cpp
Compiling src/osx/sensors.cpp
src/osx/sensors.cpp:22:10: fatal error: IOKit/hidsystem/IOHIDEventSystemClient.h: No such file or directory
   22 | #include <IOKit/hidsystem/IOHIDEventSystemClient.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [obj/osx/sensors.o] Error 1
make: *** Waiting for unfinished jobs....

Missing 32-bit defines here:

src/osx/btop_collect.cpp:680:17: error: 'vm_statistics64' was not declared in this scope; did you mean 'vm_statistics_t'?
  680 |                 vm_statistics64 p;
      |                 ^~~~~~~~~~~~~~~
      |                 vm_statistics_t
src/osx/btop_collect.cpp:681:52: error: 'HOST_VM_INFO64_COUNT' was not declared in this scope; did you mean 'HOST_VM_INFO_COUNT'?
  681 |                 mach_msg_type_number_t info_size = HOST_VM_INFO64_COUNT;
      |                                                    ^~~~~~~~~~~~~~~~~~~~
      |                                                    HOST_VM_INFO_COUNT
src/osx/btop_collect.cpp:682:57: error: 'HOST_VM_INFO64' was not declared in this scope; did you mean 'HOST_VM_INFO'?
  682 |                 if (host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info64_t)&p, &info_size) == 0) {
      |                                                         ^~~~~~~~~~~~~~
      |                                                         HOST_VM_INFO
src/osx/btop_collect.cpp:682:74: error: 'host_info64_t' was not declared in this scope; did you mean 'host_info_t'?
  682 |                 if (host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info64_t)&p, &info_size) == 0) {
      |                                                                          ^~~~~~~~~~~~~
      |                                                                          host_info_t
src/osx/btop_collect.cpp:682:89: error: 'p' was not declared in this scope
  682 |                 if (host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info64_t)&p, &info_size) == 0) {
      |                                                                                         ^
src/osx/btop_collect.cpp:682:21: error: 'host_statistics64' was not declared in this scope; did you mean 'host_statistics'?
  682 |                 if (host_statistics64(mach_host_self(), HOST_VM_INFO64, (host_info64_t)&p, &info_size) == 0) {
      |                     ^~~~~~~~~~~~~~~~~
      |                     host_statistics

And some apparently missing headers here:

src/osx/btop_collect.cpp:1123:17: error: 'rusage_info_current' was not declared in this scope
 1123 |                 rusage_info_current rusage;
      |                 ^~~~~~~~~~~~~~~~~~~
src/osx/btop_collect.cpp:1124:42: error: 'RUSAGE_INFO_CURRENT' was not declared in this scope
 1124 |                 if (proc_pid_rusage(pid, RUSAGE_INFO_CURRENT, (void **)&rusage) == 0) {
      |                                          ^~~~~~~~~~~~~~~~~~~
src/osx/btop_collect.cpp:1124:79: error: expected primary-expression before ')' token
 1124 |                 if (proc_pid_rusage(pid, RUSAGE_INFO_CURRENT, (void **)&rusage) == 0) {
      |                                                                               ^
src/osx/btop_collect.cpp:1124:21: error: 'proc_pid_rusage' was not declared in this scope
 1124 |                 if (proc_pid_rusage(pid, RUSAGE_INFO_CURRENT, (void **)&rusage) == 0) {
      |                     ^~~~~~~~~~~~~~~
src/osx/btop_collect.cpp:1126:69: error: expected primary-expression before '.' token
 1126 |                         detailed.io_read = floating_humanizer(rusage.ri_diskio_bytesread);
      |                                                                     ^
src/osx/btop_collect.cpp:1127:70: error: expected primary-expression before '.' token
 1127 |                         detailed.io_write = floating_humanizer(rusage.ri_diskio_byteswritten);
      |                                                                      ^
src/osx/btop_collect.cpp: In function 'std::vector<Proc::proc_info>& Proc::collect(bool)':
src/osx/btop_collect.cpp:1133:29: warning: possibly dangling reference to a temporary [-Wdangling-reference]
 1133 |                 const auto &sorting = Config::getS("proc_sorting");
      |                             ^~~~~~~
src/osx/btop_collect.cpp:1133:51: note: the temporary was destroyed at the end of the full expression 'Config::getS(std::basic_string<char>(((const char*)"proc_sorting"), std::allocator<char>()))'
 1133 |                 const auto &sorting = Config::getS("proc_sorting");
      |                                       ~~~~~~~~~~~~^~~~~~~~~~~~~~~~
src/osx/btop_collect.cpp:1135:29: warning: possibly dangling reference to a temporary [-Wdangling-reference]
 1135 |                 const auto &filter = Config::getS("proc_filter");
      |                             ^~~~~~
src/osx/btop_collect.cpp:1135:50: note: the temporary was destroyed at the end of the full expression 'Config::getS(std::basic_string<char>(((const char*)"proc_filter"), std::allocator<char>()))'
 1135 |                 const auto &filter = Config::getS("proc_filter");
      |                                      ~~~~~~~~~~~~^~~~~~~~~~~~~~~
make: *** [obj/osx/btop_collect.o] Error 1

Does it have some more generic Unix fallback? That could potentially work on older macOS.

@barracuda156
Copy link
Author

rusage_info_current is available on 10.10+: https://developer.apple.com/documentation/kernel/rusage_info_current

@aristocratos
Copy link
Owner

@barracuda156
Feel free to create a PR if you wanna work on adapting it for more backwards compatibility.

But I'm guessing there will be a wall at some point because of compiler compatibility (needs C++20 support).

@barracuda156
Copy link
Author

barracuda156 commented Dec 12, 2023

We have C++20 up to macOS 10.5 via GCC, and so far it is supported by upstream (PowerPC and Intel, 32- and 64-bit). I use gcc13 on my 10.6 system.
New Clangs (I think clang-16) also build back to 10.5, though only for Intel (PowerPC is broken).

@aristocratos
Copy link
Owner

aristocratos commented Dec 12, 2023

Well, having support for all but the first 5 versions should probably be good enough 😉

Ping me if you get stuck on anything and I'll try to help when I've got time.

Good luck 👍

@barracuda156
Copy link
Author

On a quick look (short on time tonight), it will not suffice just to fix compilation, since CPU detection has to be re-implemented for PowerPC. Not sure to what extent related info can be pulled from the system (some functions are restricted to the kernel), but given that apps like MenuMeters somehow work, rather detailed info can be collected.

For Intel is should be far easier; one thing which may be causing linking errors is that apparently filesystem is needed, and Apple libc++ does not have it until something like Darwin 16. It is likely fixable in Macports, we can use a newer libc++ which supports this extension.

@joske
Copy link
Collaborator

joske commented Dec 15, 2023

I tried building with make and gcc-13 on a Catalina (10.15.7) VM (64 bit), and it just worked. I never tried with any 32 bit version.

@joske
Copy link
Collaborator

joske commented Dec 15, 2023

For the El Capitan issue, IOKit/hidsystem/IOHIDEventSystemClient.h was not documented as far as I know. Also this code is only needed for Apple Silicon, so we can easily make this conditional on macOS versions above Big Sur only.

Edit: see #690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants