-
Notifications
You must be signed in to change notification settings - Fork 254
Make sure that logind is enabled if requested, make --disable-logind mandatory to disable logind integration #1318
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 sure that logind is enabled if requested, make --disable-logind mandatory to disable logind integration #1318
Conversation
The code does not enabled logind unconditionally by default. Instead configure checks for logind (libsystemd) availability and enables it only if found. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
Fail with error if wrong value is provided. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
Improve formatting and readability of single configure check. Also remove unneeded overquoting of "LIBSYSTEMD=-lsystemd". Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
|
Cc: @lslebodn Please review and let me know if you're okay with it. |
|
There are CI failures related to this change. |
|
As I wrote earlier, autotools use non-interactive approach. This is a basis of autotools.
I strongly recommend to reconsider the approach. Using autotools in the way orthogonal to autotools ideas will generate more problems then the preferred way.
So unless you switched to another build system, it is better to adhere to autotools ideas. |
2be2e80 to
3472e36
Compare
|
Autotools are very flexible.
Even your own CI/CD system assumes that Just a for a single parameter, I have to hack three items:
Imagine that you will remove other automatic detections. For each (or most of) removed detection you will need to add more parameters in at least three items. Then something could be changed in defaults and you will need to reflect again all changes in three places. I strongly recommend to use my original PR #1283. That PR does not need any hacks, because autotools work as expected. All use-cases are covered by #1283 in a better way. It is perfectly maintainable and work in the way that people expect from autotools build system. |
When touching default flags, I usually grep for all such flags, so I'd find all three places and update them. I'm used to doing the same for changing dependencies in the build system, where we need to update all images to install them. Not too bad.
I think I prefer the hacked |
|
I've been thinking about what I do in pure-Makefile projects. There, How about |
I can only repeat myself: when you use the build system in the way it was not designed to, by adding a number of hacks, it becomes fragile and will need much more maintenance work compared to normal use. I still very strongly recommend to choose my original PR #1283. It covers all the needs of shadow maintainers, distributions packages maintainers and new contributors without any hacks and in the best possible way. |
Pure Makefile project is possible, but much less easy to handle. You will loose portability, especially regarding dynamic libraries as they are handled very differently on various systems.
First of all,
This is an absolutely wrong idea. Pure makefiles builds are not configured by flags, they are typically configured by variables override, like |
No, a Makefile is more convenient to me. Autotools is kept here as a deference to distro maintainers, but certainly not for making my life as a programmer easier. I can write portable GNU Makefiles. As long as you port GNU Make to other POSIX systems, I can work there just fine (and more fine than with autotools). Your experience and mine are very different.
Of course it does more. But it does that, among the other things.
In fact, it would make more sense to inherit the configure flags, as it would allow testing various configurations deeply on the same system.
Yes. That's what I was talking about. |
Wait, what? I write Makefiles that only build out of tree. Go check the Linux man-pages project build system: |
lslebodn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature-vise LGTM.
Few tiny comments inline and BTW 1st commit fac8f697e58bc7e99a76f8f598eb17ffec24e71c in the patch set is not needed because help string is changed later commit anyway.
@Karlson2k I do not interpret it as something which is not recommended. I read it as one should install all recommended dependencies to run BTW this is exactly the same case as with running "make distcheck" without
So one can install either libbsd to make it pass or to use Using
|
Please point where it is written that "recommended" dependencies must be installed to run |
Yes, if you want to loose another portability level and switch to GNU-make-only . |
|
@alejandro-colomar Looks like you are trying to project kernel techniques to the user-space. It is different. Kernel is self-contained, has no dependencies (just tools to compile), should not adapt to the environment (except build tools and configuration set by user directly or via locally-built tools). You can easily require GNU make only, GCC or clang only, binutils. The apps and libs needs another approach. They should adapt to different environment and be portable in general. |
|
BTW, bash could be not available. sed, awk, grep, find could be non-GNU. The list could be further extended. |
This is not loosing portability. Finding a system where GNU Make is not available is not an easy task. Quoting the maintainer of GNU Make: https://make.mad-scientist.net/papers/rules-of-makefiles/ |
No. The man-pages are not the kernel. They are a user-space project. I've also written very similar build systems for C libraries with dependencies.
The system make is unimportant. GNU make is always available, even if not part of the base system. |
All systems, including the BSDs have bash, AFAIK. It's just an extra package that's not part of the base system. But packagers can perfectly use bash, GNU make, and whatever they need (an OpenBSD maintainer told me it's usual practice for them). |
The best for the user is very basic set of dependencies, which is always available (when build environment is present). It is up to you what you do with the package that your are maintaining. |
"Just an extra package" means pulling the package together with dependencies. The multiply it for every build, for every container which is used to build, for every distribution and so on. You will be surprised how large could be this number. It is waste of time, resources and energy. Is it doable? Yes. |
On the other hand, these scripts will be safer, as the less-basic tools allow writing code that is more readable, and easier to maintain. As always, it comes down to portability vs safety, and I choose safety. |
It does not mean that others will use the same weighting. The risk of manual management of custom build system is much higher that billions times tested standard build system. So it is much safer to use the standard, well tested approach than to re-invent the wheel. |
I'm not responsible for the safety of others' systems. If they want to break their own systems, they're free to do it.
I've found so many bugs in the build system of this project. The Linux man-pages build system works like a charm. Huh? BTW, make(1) is a POSIX standard, but autotools isn't. Are you sure writing Makefiles is unsafe? Many projects rely entirely on it. |
Are you talking as shadow co-maintainer?
And how many of them were security-related?
Does it always work perfectly?
Yes, make is POSIX, but GNU makefiles are not. However, you do not need to install autotools to build the package as soon as you make a tarball. Autotools uses POSIX standard utilities to build packages on many systems, including not-fullt-POSIX compatible and takes care about portability and compatibility. You are ignoring POSIX (and other standards) everywhere, why it is an argument now? |
Maintainer can install all optional dependencies enabled by default (not just sytemd also libbsd), for creating dist tarball. But they still have a possibility to run "make dist" without installed extra dependencies. They can
Based on your interpretation, one should remove BTW you forgot to quote other part from DISTCHECK_CONFIGURE_FLAGS in automake documentation
Anyway; lets return back to the MR. There is not ideal build system. Each one has pros and cons. Extra dependency mostly affect CI. Failures in CI are solved by AM_DISTCHECK_CONFIGURE_FLAGS. Few related question:
|
That's irrelevant. I'm not responsible for the safety of systems that don't use shadow-utils.
Probably none, but you never know for sure.
Very much. And when there's a minor bug, the fix is trivial. It works much better than the build system of shadow-utils, by orders of magnitude.
Very very many. That goes for everything that tries to be best-in-class, of course. But it scales quite well. I can almost paste that build system into new projects, and use it out of the box with minor changes.
Well, I guess. I didn't find issues so far.
The projects I use don't have variable dependencies. But it would be pretty easy to add them, yes. It wouldn't be much different from the tests that I run at the moment for compiler features.
The code is written using autotools, so regardless of what runs on the target machine, the safety concerns during packaging are still there. The Makefiles generated by autotools aren't magically safe.
You mentioned "standards". BTW, I don't ignore POSIX. I selectively decide when I don't need to care about it, which is different. |
|
Looks like you decided already so it does not make sense to give you any arguments. Good luck. |
Which does not make sense as
Probably yes.
Do you see any similarity between these two cases?
I don't.
As my arguments are collectively ignored and just selectively answered, sometimes based only on "I like another solution, because I prefer it", I'm not going to contribute my time anymore in such discussions. |
|
Please don't tell me what code will be easier for me to maintain. Would you mind going back on-topic and discuss the issues raised by @lslebodn ? |
It is all yours. I may close my PRs by myself if you wish. |
I welcome your code contributions, but not that you go off-topic and tell me how much portability we should write, or how easy your favourite language is and how hard mine is. |
3472e36 to
49bf74f
Compare
|
I had an impression that off-topic was started by you. Anyway, the PR has been updated with extra spaces in error message. |
I don't think so. I was mentioning what I do, because that's what we could do here. But okay, let's stop both.
Thanks! |
49bf74f to
8b43895
Compare
|
The PR has been updated with libsystemd-dev in |
Can we please inherit the configure flags in the |
This seems to be an unrelated change as this PR has been build-tested fine. |
Sorry, I find this idea horrible and I do not want to sign by my name a commit with such things. I already crossed the line with what I think is "bad" in my commits and I do not want to go any further. But you can do it.
AC_SUBST([ac_configure_args])And then add to the toplevel Makefile.ac
AC_SUBST([AM_DISTCHECK_CONFIGURE_FLAGS], ["$ac_configure_args"])But in this case
AC_SUBST([ac_configure_args])
AM_SUBST_NOTMAKE([ac_configure_args])In top Makefile.ac |
Sounds reasonable.
Thanks! |
…nable-logind default Before this commit, if configured with --enable-logind, but libsystemd is not found, configure silently succeed, however logind is efficiently disabled. With this commit, the configure fails if logind is not explicitly disabled and libsystemd is not found. --disable-logind is mandatory if logind integration should not be used. Automatic detection is disabled by Alejandro Colomar's request. Extra help in the error message is added by lslebodn's request. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
8b43895 to
1229e56
Compare
|
Updated with extended error message. |
alejandro-colomar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is alternative version of #1283
This PR:
--enable-logindis requested, but libsystemd is missing--enable-loginddefault (most common scenario nowadays). As this option is enabled by default, the help string is changed to--disable-logind(no parameter means the same as--enable-logind).Note: for configure the following parameters are treated equally:
--enable-logindand--enable-logind=yes--disable-logindand--enable-logind=no