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

make: Use (almost) POSIX make for portability #2

Closed
wants to merge 1 commit into from

Conversation

concatime
Copy link

@concatime concatime commented Jan 7, 2022

This is portable, except for the yet-to-be standard !=.
This makefile works with BSD make (bmake) and GNU make (gmake).

# export CFLAGS=... LDFLAGS=...
make -e
sudo make PREFIX=/usr install

WDYT?

Copy link
Owner

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

Hmm, what exactly about the current Makefile isn't POSIX compliant? It already seems to work fine with bmake for me...

What error are you currently getting that this PR fixes and what make implementation are you using?

@concatime
Copy link
Author

Well, the real motivation for this was to support pkg-config for pam (because welcome to 2010). While doing so, I decided to go (almost) full POSIX.

@ifreund
Copy link
Owner

ifreund commented Jan 14, 2022

@concatime again, what concrete problem does this fix? I'm happy to help make this project easier for distros to package but I don't want to maintain a Makefile that's any more complex than necessary.

What is the minimal patch that fixes the concrete issue you are having?

@concatime
Copy link
Author

Sorry for the delay. When building dumb_runtime_dir, I need to specity both C_INCLUDE_PATH and LIBRARY_PATH to make. This way the compiler can find libpam. The natural solution would be to simply rely on pkg-config, and fallback to -lpam.

@ifreund
Copy link
Owner

ifreund commented Jan 20, 2022

@concatime I'm fine with using pkg-config, but there's no need to make things so complicated. Does this fix your use-case? If so I'll push it to master.

diff --git a/Makefile b/Makefile
index 82a9776..d932d10 100644
--- a/Makefile
+++ b/Makefile
@@ -1,11 +1,12 @@
 PREFIX ?= /usr
 CC ?= cc
 CFLAGS ?= -Os -Wall -Wextra -Wpedantic -Wconversion -Werror
+PAMFLAGS = $$(pkg-config --cflags --libs pam)
 
 RUNTIME_DIR_PARENT ?= "\"/run/user\""
 
-pam_dumb_runtime_dir.so:
-       $(CC) -o $@ pam_dumb_runtime_dir.c -lpam -shared -fPIC -std=c99 $(CFLAGS) \
+pam_dumb_runtime_dir.so: pam_dumb_runtime_dir.c
+       $(CC) -o $@ pam_dumb_runtime_dir.c $(PAMFLAGS) -shared -fPIC -std=c99 $(CFLAGS) \
                -DRUNTIME_DIR_PARENT=$(RUNTIME_DIR_PARENT)
 
 .PHONY: all install uninstall clean

@concatime
Copy link
Author

Normally it should do it. Ty.

@concatime concatime closed this Jan 29, 2022
@ifreund
Copy link
Owner

ifreund commented Jan 29, 2022

@concatime Just released 1.0.2 which now uses pkg-config :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants