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.

Sign up for GitHub

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

mupen64plus-input-raphnetraw: don't hard-code pkg-config #296

Merged
merged 1 commit into from
Oct 20, 2024
Merged

mupen64plus-input-raphnetraw: don't hard-code pkg-config #296

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions Source/3rdParty/mupen64plus-input-raphnetraw/projects/unix/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,14 @@ ifeq ($(OS), OSX)
endif
endif

# test for essential build dependencies
ifeq ($(origin PKG_CONFIG), undefined)
PKG_CONFIG = $(CROSS_COMPILE)pkg-config
ifeq ($(shell which $(PKG_CONFIG) 2>/dev/null),)
Copy link

Choose a reason for hiding this comment

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

Please don't use this. "which" is less likely to be installed than "pkg-config".

"command -v" is a portable (POSIX mandated) equivalent to "which".

Copy link
Owner

@Rosalie241 Rosalie241 Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

mupen64plus-core also uses which there though.

Copy link

Choose a reason for hiding this comment

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

Then mupen64plus-core, too, is wrong. :)

My attention was attracted here because @orbea implemented this change for Gentoo and I reviewed that change. I'm not directly familiar with this ecosystem though I am always happy to provide tips such as this one when I'm made aware of it.

Copy link

Choose a reason for hiding this comment

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

Informative reading :) https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then/85250#85250

Copy link
Owner

@Rosalie241 Rosalie241 Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Then mupen64plus-core, too, is wrong. :)

I'm not sure if I'd consider using which to be wrong per se, especially considering how widespread the usage of that command is in general.

It's just a dependency which exists, it's neither right or wrong in my opinion, and changing it for the sake of changing it is possible but I don't feel like it matters much because almost every linux system has which available or installed.

Copy link

Choose a reason for hiding this comment

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

I have used multiple Linux distros where it wasn't available by default.

It is most commonly seen on Debian specifically because Debian has a policy guide saying that the "which" program which they privately wrote from scratch must be installed in the base system in order to be available in dpkg postinst scripts. And sure enough... Debian is trying to purge the use of "which" so they can finally drop that. :)

People often tend to code for Debian specifically. It's unfortunate. The assumption is that since Debian is the numerically largest number of Linux systems, that writing software for "Debian" is the same as writing software for "Linux".

It's just a dependency which exists, it's neither right or wrong in my opinion

That's an opinion.

I informed you of a fact, however. It is "factual* that it's wrong, and that you cannot even assume that "which" will return success when the command exists and an error when it doesn't. You can't assume it only prints stdout on success. You can't assume its results make sense for your current environment (rather than reading the rc scripts from your interactive shell). You can't assume a lot of things.

It's also quite weird to force people to install a specific dependency in order to query if an entirely different command exists on PATH, since if a user has to manually install something anyway they might as well manually install pkg-config itself. Given the shell can do this built-in... why not use the builtin?

What's the appeal of adding an additional third party dependency that is commonly not installed when the base operating system has a more reliable alternative already?

It's an incredibly simple change, anyway. 🤷

Copy link
Owner

Choose a reason for hiding this comment

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

It's an incredibly simple change, anyway. 🤷

This is only true if you don't consider having to upstream the patch for each project which RMG uses which uses that command, it's time consuming for no real gain.

But if you consider this to be a simple change, then feel free to do so for each upstream project yourself.

$(error $(PKG_CONFIG) not found)
endif
endif

ifeq ($(OS), LINUX)
HIDAPI_NAME=hidapi-hidraw
else
Expand All @@ -149,9 +157,9 @@ endif

# test for presence of HIDLIB
ifeq ($(origin HID_CFLAGS) $(origin HID_LDLIBS), undefined undefined)
HIDAPI_CONFIG = $(CROSS_COMPILE)pkg-config $(HIDAPI_NAME)
HIDAPI_CONFIG = $(PKG_CONFIG) $(HIDAPI_NAME)
ifeq ($(shell which $(HIDAPI_CONFIG) 2>/dev/null),)
HIDAPI_CONFIG = $(CROSS_COMPILE)pkg-config $(HIDAPI_NAME)
HIDAPI_CONFIG = $(PKG_CONFIG) $(HIDAPI_NAME)
ifeq ($(shell which $(HIDAPI_CONFIG) 2>/dev/null),)
$(error No HIDAPI development libraries found!)
else
Expand Down