configure.ac, src/Makefile.am: Follow GNU coding standards regarding directory variables #1231
Conversation
I bail out; too much autotools for me. @thesamesam please help. :-) |
|
I'll look but probably tomorrow. |
alejandro-colomar
force-pushed
the
ep
branch
from
8840d0e to
6f4e5d6
Compare
thesamesam
left a comment
thesamesam
left a comment
There was a problem hiding this comment.
OK with that one change for {} vs ().
alejandro-colomar
force-pushed
the
ep
branch
2 times, most recently
from
8d0eb98 to
51b0e9f
Compare
| .indent.pro | ||
|
|
||
| ubindir = ${prefix}/bin | ||
| usbindir = ${prefix}/sbin |
There was a problem hiding this comment.
With the current state of MR, the same exec_prefix will be used for ubindir && bindir
- https://www.gnu.org/software/automake/manual/html_node/Standard-Directory-Variables.html
So with the current state of MRubindir==bindirandusbindir==sbindir.
Do we keepubindir and usbindir in such case?
Or they are intentionally separate in Makefile.am so one can use make variables to have different values for ubindir and bindir e.g.
make install ubindir=/tmp/bin usbindir=/tmp/sbin
Otherwise LGTM and +1 for removing hacks :-)
There was a problem hiding this comment.
I want to remove them, but in a separate PR. I'd say even in a separate release, just to make sure we don't break anything at some point.
There was a problem hiding this comment.
@alejandro-colomar Are you intentionally moving all binaries from "/bin" to "/usr/bin"?
It makes sense to move all of them, but then you should use just standard functionality and completely remove use of ubindir and usbindir.
There was a problem hiding this comment.
Yes, I want that; however, I'd like the transition to be gradual, to make sure each patch is easy to review for correctness.
|
As it is now, the patch does not break anything in the fedora packaging. |
Self answer: I still get this. @thesamesam , do you know why? |
It's due to this line: Line 106 in b4beda8 But is that line bad? How should one use |
alejandro-colomar
force-pushed
the
ep
branch
5 times, most recently
from
cc9e79c to
b28fb39
Compare
alejandro-colomar
force-pushed
the
ep
branch
4 times, most recently
from
a7585cb to
4f6598b
Compare
alejandro-colomar
force-pushed
the
ep
branch
2 times, most recently
from
0d718ce to
c3b6e0c
Compare
|
Cc: @Karlson2k Maybe he knows. |
|
There is no need to re-invent the wheel. The useless hacks at the start of the In short: do not use "$exec_prefix/bin/passwd". Both Avoid manipulating or setting any paths in It is used by all autotools projects (except projects which invented their own hacks) and this convention is straightforward for downstream users and package maintainers: they know how to deal with it, how to configure it, and also this removes the need for downstream hacks to override upstream-invented hacks. |
|
If any installation path is assigned directly in To emphasise: DO NOT assign any installation path variables in If you really badly need to hardcode an absolute path in the binary file, you may use DEFS += '-DPASSWD_PROGRAM="$(sbindir)/passwd"'in your |
|
Note: in Makefiles, |
Would you mind sending patches for that? I don't know how to do it. My understanding of configure.ac and Makefile.am is very limited. |
|
Actually this need much deeper fix and hacks removals. Lines 5 to 6 in d7ce7e8 With global usr-merge it is much better to stop trying to differentiate "root" and "/usr". If any downstream still needs to use different "root" and "/usr" it can be easily handled at packaging stage without any hacks. For now, to fix this PR you just need to remove AC_DEFINE_UNQUOTED([PASSWD_PROGRAM], ["$exec_prefix/bin/passwd"],
[Path to passwd program.])from configure.ac and put DEFS += '-DPASSWD_PROGRAM="$(sbindir)/passwd"'to Line 4 in d7ce7e8 Note: empty assignment to "DEFS" in this file is wrong (or another hack?). It must be Alternatively (and less correct) you may just change configure.ac: AS_VAR_IF([exec_prefix],["NONE"],[:],
[AC_DEFINE_UNQUOTED([PASSWD_PROGRAM], ["$exec_prefix/bin/passwd"],
[Path to passwd program.])]
)The code is handling correctly undefined I will not be able to take my hands on it until February. |
alejandro-colomar
force-pushed
the
ep
branch
from
d7b978a to
921e734
Compare
Thanks! I've applied your suggestion.
No hurry. Thanks! You're contributions will be very useful when you have time for them. :) |
alejandro-colomar
requested review from
hallyn
and removed request for
thesamesam
|
You missed the proper quoting. |
Ahh, sorry. Why is this? In regular makefiles, quoting is unnecessary. Is this because of autotools? |
No, it is the regular Makefile rule. |
Let me try to understand it. We have in the Makefile: DEFS = -DPASSWD_PROGRAM="$(sbindir)/passwd"Which presumably is used in a recipe something like $(CC) $(DEFS) $(CFLAGS) ...Before make(1) executes the shell, it expands variables, thus passing the following string to sh(1): cc -DPASSWD_PROGRAM="/usr/local/bin/passwd" ...Then, the shell performs tokenization: And then, the shell expands quoted strings: And then calls execve(2). Which program does need the single quotes and why? |
|
Here's a small example of how I expect it to work: alx@devuan:~/tmp$ cat Makefile
bar = B A R
DEFS = -DFOO="$(bar)"
all:
echo FOO | cc -E $(DEFS) -x c /dev/stdinalx@devuan:~/tmp$ make
echo FOO | cc -E -DFOO="B A R" -x c /dev/stdin
# 0 "/dev/stdin"
# 0 "<built-in>"
# 0 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "<command-line>" 2
# 1 "/dev/stdin"
B A R |
Yes, you get literally |
Ahhh, I see; my bad. Thanks! :) |
alejandro-colomar
force-pushed
the
ep
branch
from
921e734 to
fc101c6
Compare
|
Ughh, now some tests fail. |
|
At least some failures do not look like related. |
Suggested-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev> Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
force-pushed
the
ep
branch
from
fc101c6 to
092b283
Compare
For consistency with <paths.h> definitions. Signed-off-by: Alejandro Colomar <alx@kernel.org>
…directory variables According to Sam, if we just remove the definitions, autotools will provide the right values. Note that this changes the default from /usr to /usr/local, which is the right value according to the GNU conding standards (and also what the BSDs prefer, according to one conversation I had with Ingo Schwarze from OpenBSD). Closes: <shadow-maint#1229> Link: <https://www.gnu.org/software/autoconf/manual/autoconf-2.72/html_node/Default-Prefix.html> Reported-by: Chris Hofstaedtler <zeha@debian.org> Cc: Sam James <sam@gentoo.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Some code was using $() and other ${}. Use the same everywhere.
Reported-by: Sam James <sam@gentoo.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
force-pushed
the
ep
branch
from
092b283 to
b33540f
Compare
6 participants
Closes: #1229
Reported-by: @zeha
Cc: @thesamesam
Is this okay?
Revisions:
v2
v3
${}by$()in surrounding code. [@thesamesam ]v3b
v3c
v3d
v3e
v3f
v3g
v3h
v3i
v3j
v3k
v3l
v3m
v3n
v3o
v3p
v3q
v4
v4b
v4c
v4d
v4e
v4f
v4g
v4h
v5
$(sbindir)in PASSWD_PROGRAM. [@Karlson2k ]v5b
v6
v6b