d99kris / nchat

Terminal-based Telegram / WhatsApp client for Linux and macOS
MIT License
524 stars 40 forks source link

Do not hardcode brewisms for macOS #194

Closed barracuda156 closed 3 months ago

barracuda156 commented 4 months ago

It is desirable not to hardcode a usage of any package manager (it may not be installed or may not be wanted at all). At the moment, CMakeLists do seem to require brewisms unconditionally: https://github.com/d99kris/nchat/blob/15b08991bfd8ac10440934691a036bd4a959f4dd/CMakeLists.txt#L24-L35 At the very least, it should first check for Homebrew installation, though IMO it is still wrong to coerce using that (Homebrew may be installed for w/e reason, but a user may prefer using Macports, or not relying on any package manager).

d99kris commented 4 months ago

Hi @barracuda156 - yep that's a valid concern. So with a MacPorts system, are you seeing a build error? (if yes, please share the cmake output)

The intention with that piece of code was to use ncurses from brew if available, and otherwise use some fallback path. We can change the fallback path to be MacPorts path to ncurses, or perhaps consider some other solution. The check was not supposed to fail on non-brew systems, but I have not tested it.

barracuda156 commented 4 months ago

@d99kris Thanks for responding!

So with a MacPorts system, are you seeing a build error?

We do not have a port for nchat, so I do not know :) I can try making it, but perhaps once I am back home, so that I can test it on older systems as well.

The intention with that piece of code was to use ncurses from brew if available

In principle, it may be the case that brew is installed (whether for a good reason or merely unused but not removed), but a user does not want to rely on it for a specific port, or prefers to use Macports or Fink (or something else). It seems to be safer to offer a configure option rather than making a choice on a basis of whether something is installed.

We can change the fallback path to be MacPorts path to ncurses

/usr/local/opt seems like a Homebrew prefix; if so, perhaps there is little point to use it when brew is not available? I do not really know if it is desirable to hardcore Macports prefix either, to be honest.

d99kris commented 4 months ago

Thanks for the response!

Right, I understand there's no port, I meant if trying to build nchat manually on a local system with dependencies installed using MacPorts.

Yes, /usr/local/opt is some old hardcoded (Homebrew) prefix that I used prior to adding detection, so I left it as a fallback (in case brew is not in PATH, or whatever issue there may be).

True, it's probably better with dynamic MacPorts detection as well. I'll try do some research, perhaps CMake has better ncurses detection built-in nowadays, so I can remove this hacky detection altogether.

barracuda156 commented 4 months ago

perhaps CMake has better ncurses detection built-in nowadays

Perhaps find_package? https://cmake.org/cmake/help/latest/command/find_package.html

d99kris commented 4 months ago

Yes, nchat uses find_package(Curses REQUIRED) in its CMakeLists.txt, however it used to fail to find ncurses installed using brew, and instead first use macOS provided ncurses (which is a bit outdated). By adding the brew installation path to CMAKE_PREFIX_PATH it would however use brew ncurses. I am not sure whether it is still an issue. I would need to test on a couple of systems to see if this brew specific portion can be omitted.

Another project of mine (nmail which you're familiar with) currently also has some code to use brew paths (but hard-coded):

  list(APPEND CMAKE_PREFIX_PATH /usr/local/opt/ncurses)
  list(APPEND CMAKE_PREFIX_PATH /opt/homebrew/opt/ncurses)

Will do some research and see if this can be avoided.

barracuda156 commented 4 months ago

There are ways to specify paths which CMake should use. For example, Macports has several versions of libfmt, and we can choose the specific one to be used with any given port. The same mechanism should work with arbitrary paths (including those odd ones which Homebrew utilizes).

UPD. Ah, basically this is what already done here in a way, just with hardcoding. Maybe provide configure options to specify a desired package manager? I do not know what CMake does on its own in a case when both Macports and Homebrew are installed and provide some port.

barracuda156 commented 4 months ago

On a side note, I think Macports has the latest release of ncurses: https://ports.macports.org/port/ncurses/details

d99kris commented 4 months ago

There are ways to specify paths which CMake should use. For example, Macports has several versions of libfmt, and we can choose the specific one to be used with any given port. The same mechanism should work with arbitrary paths (including those odd ones which Homebrew utilizes).

UPD. Ah, basically this is what already done here in a way, just with hardcoding. Maybe provide configure options to specify a desired package manager? I do not know what CMake does on its own in a case when both Macports and Homebrew are installed and provide some port.

Ideally it would be nice if it can build on "any" system, without user having to specify what package manager is used. Of course an optional configure option can be added when override is needed.

I think I need to set up a macOS system with MacPorts to see what path it installs ncurses to, then I can probably make some adjustments so both brew and MacPorts are properly supported.

barracuda156 commented 4 months ago

I think I need to set up a macOS system with MacPorts to see what path it installs ncurses to, then I can probably make some adjustments so both brew and MacPorts are properly supported.

You could always look into details for a port: https://ports.macports.org/port/ncurses/details There if you click on Files for any given OS, it will show list of installed files with exact paths.

ncurses uses standard prefix (/opt/local/include, /opt/local/lib).

d99kris commented 4 months ago

Thanks for sharing 👍

d99kris commented 3 months ago

Hi @barracuda156 - I have tested building on a macOS system with dependencies installed using MacPorts and nchat is building fine.

In the make.sh helper build script I've added handling of MacPorts in commit 896c23f for better portability. I've also updated the CMakeLists.txt default/fallback path to match the one of MacPorts. I will proceed to close this issue. Feel free to re-open if you have any follow-up questions or concerns.

barracuda156 commented 3 months ago

@d99kris Great, thank you!