search

jgarff / rpi_ws281x

Userspace Raspberry Pi PWM library for WS281X LEDs
BSD 2-Clause "Simplified" License
1.76k stars 616 forks source link

Bugs in "make install" #456

Open hajosc opened 3 years ago

hajosc commented 3 years ago

cmake 3.16.3 GNU make 4.2.1 pkg-config 0.29 ldconfig 2.28 Raspbian GNU/Linux 10 (buster)

Gadgetoid commented 3 years ago

Do you know enough about CMake to PR a fix for this?

hajosc commented 3 years ago

Sorry, unfortunately I have no clue about cmake other than how to invoke it :-( But I guess I could look into it if noone else comes forward with some knowledge.

arcturis commented 3 years ago

Hi @hajosc, I've opened PR #463 here that I believe should solve the first part of your issue, looks like the ws2811.pc file was trying to use variables that only exist after including GNUInstallDirs in the CMakeLists.txt file.

For ldconfig, is it standard practice to call it as part of a make install? Doing some research online the answer seems to be that it should be the responsibility of a package manager or the user rather than the make install step.

hajosc commented 3 years ago

I think leaving it to the user is archaic. I remember that I had to run it long time ago after installations (maybe it wasn't used across all distros at the time?) But I can't remember having had to run it for at least 3-4 years although I still do install libraries sporadically. Granted, mostly from rpm. So no definite answer here, but does it hurt to have it done in the Makefile? Alternatively I think it should be mentioned in the README.md Uh, and, thanks a lot for your help on this!

Gadgetoid commented 3 years ago

I'm also pretty sure "ldconfig" gets run as part of the "make install" step for most libraries I've used in the last... forever. I can't seem to find any consensus on it being standard and plenty of impassioned arguments against it.

Given that we know the target for this library is always Linux on the Raspberry Pi we can discount any issues invoking ldconfig might have with shipping library source on macOS. All that remains are issues with non-root installs invoking ldconfig without sufficient privileges to actually run... I'm inclined to just emit a warning in that case and hope any user doing this knows what they're doing.

Might as well do it! :laughing:

arcturis commented 3 years ago

Sounds good! I think that would cover all the problems people were mentioning online. I've added the change to the PR #463, it attempts to run ldconfig and if it gets a non-zero exit code, it prints a warning. I'm happy to modify the message or anything else, but I believe this change will do what we want. I've tested it on Ubuntu Stretch with CMake 3.20.2 as well as Raspbian Stretch with CMake 3.11.1 using make install and sudo make install, and it all behaves as expected.

And no problems with helping out! I've been building some projects recently that use this library pretty heavily so it feels good to contribute something back!