[pacman-dev] [PATCH 0/4] Various fixes for FreeBSD (and perhaps others)

older
[pacman-dev] [PATCH 1/3] Always...

Mark Weiman

16 Apr 2021 16 Apr '21

9:42 p.m.

This is a patch set that fixes a few issues when attempting to build on FreeBSD. Mark Weiman (4): Fix check for FSSTATSTYPE meson.build: Fix detection of symbols Add an include for signal.h when needed Fix build error when SIGPOLL is not available lib/libalpm/util.c | 10 +++++++++- meson.build | 34 +++++++++++++++++++++++++++++++--- src/pacman/conf.c | 4 ++++ 3 files changed, 44 insertions(+), 4 deletions(-) -- 2.31.1

Show replies by date

Mark Weiman

16 Apr 16 Apr

9:42 p.m.

New subject: [pacman-dev] [PATCH 1/4] Fix check for FSSTATSTYPE

On FreeBSD, both `struct statvfs` and `struct statfs` satisfy the conditions where the `f_flag` and `f_flags` fields are present in both respectively. This patch accomplishes a couple of things: 1. Adds a check for `f_mntonname` in both structs and makes a decision to choose statfs if the field is not present in statvfs, but is in statfs. 2. Corrects a small error where the values of those checks are just checked for their presence and not whether its true or false. Signed-off-by: Mark Weiman --- meson.build | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index 579ff2ed..483c4fbd 100644 --- a/meson.build +++ b/meson.build @@ -154,6 +154,9 @@ foreach member : [ ['struct statvfs', 'f_flag', '''#include '''], ['struct statfs', 'f_flags', '''#include #include '''], + ['struct statvfs', 'f_mntonname', '''#include '''], + ['struct statfs', 'f_mntonname', '''#include + #include '''], ] have = cc.has_member(member[0], member[1], prefix : member[2]) conf.set('HAVE_' + '_'.join([member[0], member[1]]).underscorify().to_upper(), have) @@ -174,9 +177,13 @@ foreach type : [ endif endforeach -if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs') -elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif -- 2.31.1

Mark Weiman

9:42 p.m.

New subject: [pacman-dev] [PATCH 2/4] meson.build: Fix detection of symbols

This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined. Signed-off-by: Mark Weiman --- meson.build | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 483c4fbd..14b3381a 100644 --- a/meson.build +++ b/meson.build @@ -146,7 +146,9 @@ foreach sym : [ 'tcflush', ] have = cc.has_function(sym, args : '-D_GNU_SOURCE') - conf.set10('HAVE_' + sym.to_upper(), have) + if have + conf.set10('HAVE_' + sym.to_upper(), have) + endif endforeach foreach member : [ -- 2.31.1

Mark Weiman

9:42 p.m.

New subject: [pacman-dev] [PATCH 3/4] Add an include for signal.h when needed

On Linux, signal.h is not required to have access to the signal constants. On FreeBSD, this is not the case and requires signal.h to be explicitly included. This patch checks if SIGINT can be used without signal.h and if not, make sure its included in the files that need it. Signed-off-by: Mark Weiman --- lib/libalpm/util.c | 4 ++++ meson.build | 6 ++++++ src/pacman/conf.c | 4 ++++ 3 files changed, 14 insertions(+) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index b386fde6..d2a688a2 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -34,6 +34,10 @@ #include #include +#if defined(INCLUDE_SIGNAL_H) +#include +#endif + /* libarchive */ #include #include diff --git a/meson.build b/meson.build index 14b3381a..0dc0d612 100644 --- a/meson.build +++ b/meson.build @@ -189,6 +189,12 @@ if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STR conf.set('FSSTATSTYPE', 'struct statfs') endif +signal_code = '''void foo() { int sig = SIGINT; }''' +signal_test = cc.compiles(signal_code, name : 'test if signal.h is not needed') +if not signal_test + conf.set('INCLUDE_SIGNAL_H', true) +endif + if get_option('debug') extra_cflags = [ '-Wcast-align', diff --git a/src/pacman/conf.c b/src/pacman/conf.c index a4f2ba35..fcb1ccd8 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -32,6 +32,10 @@ #include #include +#if defined(INCLUDE_SIGNAL_H) +#include +#endif + /* pacman */ #include "conf.h" #include "ini.h" -- 2.31.1

Andrew Gregory

10:08 p.m.

New subject: [pacman-dev] [PATCH 3/4] Add an include for signal.h when needed

On 04/16/21 at 03:42pm, Mark Weiman wrote:

...

On Linux, signal.h is not required to have access to the signal constants. On FreeBSD, this is not the case and requires signal.h to be explicitly included.

This patch checks if SIGINT can be used without signal.h and if not, make sure its included in the files that need it.

No need for the check; just include signal.h wherever we use it.

Mark Weiman

17 Apr 17 Apr

4:08 a.m.

New subject: [pacman-dev] [PATCH 3/4] Add an include for signal.h when needed

On Fri, 2021-04-16 at 13:08 -0700, Andrew Gregory wrote:

...

On 04/16/21 at 03:42pm, Mark Weiman wrote:

...
On Linux, signal.h is not required to have access to the signal constants. On FreeBSD, this is not the case and requires signal.h to be explicitly included.

This patch checks if SIGINT can be used without signal.h and if not, make sure its included in the files that need it.

No need for the check; just include signal.h wherever we use it.

Along with the suggested changes, since FreeBSD (and others) typically use clangas the C compiler of choice, a lot of warnings get tossed when compiling pacman.Would it be worth it to address clang warnings as long as it doesn't affect a normal GCC compilation? Mark

Mark Weiman

16 Apr 16 Apr

9:42 p.m.

New subject: [pacman-dev] [PATCH 4/4] Fix build error when SIGPOLL is not available

On Linux, SIGPOLL is a valid signal, but on systems like FreeBSD, it is not. This patch adds a detection within meson.build to check if it's available, and if not, make sure it's not included. Signed-off-by: Mark Weiman --- lib/libalpm/util.c | 6 +++++- meson.build | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index d2a688a2..db07502a 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -561,7 +561,11 @@ static void _alpm_reset_signals(void) int *i, signals[] = { SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGFPE, SIGHUP, SIGILL, SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSTOP, SIGTERM, SIGTSTP, - SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPOLL, SIGPROF, SIGSYS, SIGTRAP, + SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, +#if defined(HAVE_SIGPOLL) + SIGPOLL, +#endif + SIGPROF, SIGSYS, SIGTRAP, SIGURG, SIGVTALRM, SIGXCPU, SIGXFSZ, 0 }; diff --git a/meson.build b/meson.build index 0dc0d612..359e7f05 100644 --- a/meson.build +++ b/meson.build @@ -193,8 +193,21 @@ signal_code = '''void foo() { int sig = SIGINT; }''' signal_test = cc.compiles(signal_code, name : 'test if signal.h is not needed') if not signal_test conf.set('INCLUDE_SIGNAL_H', true) + include_signal = '''#include ''' +else + include_signal = '' endif +foreach sig : [ + 'SIGPOLL', + ] + if cc.compiles(include_signal + ''' + void foo() { int sig = ''' + + sig + '''; }''', name : 'test for ' + sig) + conf.set('HAVE_' + sig, true) + endif +endforeach + if get_option('debug') extra_cflags = [ '-Wcast-align', -- 2.31.1

Andrew Gregory

10:08 p.m.

New subject: [pacman-dev] [PATCH 4/4] Fix build error when SIGPOLL is not available

On 04/16/21 at 03:42pm, Mark Weiman wrote:

...

On Linux, SIGPOLL is a valid signal, but on systems like FreeBSD, it is not. This patch adds a detection within meson.build to check if it's available, and if not, make sure it's not included.

Signed-off-by: Mark Weiman --- lib/libalpm/util.c | 6 +++++- meson.build | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index d2a688a2..db07502a 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -561,7 +561,11 @@ static void _alpm_reset_signals(void) int *i, signals[] = { SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGFPE, SIGHUP, SIGILL, SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSTOP, SIGTERM, SIGTSTP, - SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPOLL, SIGPROF, SIGSYS, SIGTRAP, + SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, +#if defined(HAVE_SIGPOLL) + SIGPOLL, +#endif + SIGPROF, SIGSYS, SIGTRAP, SIGURG, SIGVTALRM, SIGXCPU, SIGXFSZ,

SIGPOLL is a macro, just check for it directly. For the sake of readability, leave the other three signals on the line they're already on and put SIGPOLL at the end. Please also add a comment that this is for FreeBSD so we don't forget and revert it in the future.

Eli Schwartz

11:02 p.m.

New subject: [pacman-dev] [PATCH 4/4] Fix build error when SIGPOLL is not available

On 4/16/21 3:42 PM, Mark Weiman wrote:

...

On Linux, SIGPOLL is a valid signal, but on systems like FreeBSD, it is not. This patch adds a detection within meson.build to check if it's available, and if not, make sure it's not included.

I have seen this brought up before... It solves one of the two problems brought up in https://lists.archlinux.org/pipermail/pacman-dev/2020-September/024525.html And it does so without keying off of #ifndef __APPLE__ :D

...

Signed-off-by: Mark Weiman --- lib/libalpm/util.c | 6 +++++- meson.build | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index d2a688a2..db07502a 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -561,7 +561,11 @@ static void _alpm_reset_signals(void) int *i, signals[] = { SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGFPE, SIGHUP, SIGILL, SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSTOP, SIGTERM, SIGTSTP, - SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPOLL, SIGPROF, SIGSYS, SIGTRAP, + SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, +#if defined(HAVE_SIGPOLL) + SIGPOLL, +#endif + SIGPROF, SIGSYS, SIGTRAP, SIGURG, SIGVTALRM, SIGXCPU, SIGXFSZ, 0 }; diff --git a/meson.build b/meson.build index 0dc0d612..359e7f05 100644 --- a/meson.build +++ b/meson.build @@ -193,8 +193,21 @@ signal_code = '''void foo() { int sig = SIGINT; }''' signal_test = cc.compiles(signal_code, name : 'test if signal.h is not needed') if not signal_test conf.set('INCLUDE_SIGNAL_H', true) + include_signal = '''#include ''' +else + include_signal = '' endif

+foreach sig : [ + 'SIGPOLL', + ] + if cc.compiles(include_signal + ''' + void foo() { int sig = ''' + + sig + '''; }''', name : 'test for ' + sig) + conf.set('HAVE_' + sig, true) + endif +endforeach + if get_option('debug') extra_cflags = [ '-Wcast-align',

-- Eli Schwartz Bug Wrangler and Trusted User

Mark Weiman

17 Apr 17 Apr

5:45 a.m.

New subject: [pacman-dev] [PATCH 0/5] Various fixes for FreeBSD (and perhaps others)

This is a patch set that fixes a few issues when attempting to build on FreeBSD. Mark Weiman (5): Fix check for FSSTATSTYPE meson.build: Fix detection of symbols Add an include for signal.h when needed Fix build error when SIGPOLL is not available meson-make-symlink.sh: Make compatible with non-GNU ln build-aux/meson-make-symlink.sh | 8 ++++++-- lib/libalpm/util.c | 9 +++++++-- meson.build | 15 ++++++++++++--- src/pacman/conf.c | 1 + 4 files changed, 26 insertions(+), 7 deletions(-) -- 2.31.1

Mark Weiman

5:45 a.m.

New subject: [pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

Allan McRae

19 Apr 19 Apr

9:01 a.m.

New subject: [pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

On 17/4/21 1:45 pm, Mark Weiman wrote:

...

On FreeBSD, both `struct statvfs` and `struct statfs` satisfy the conditions where the `f_flag` and `f_flags` fields are present in both respectively.

This patch accomplishes a couple of things:

1. Adds a check for `f_mntonname` in both structs and makes a decision to choose statfs if the field is not present in statvfs, but is in statfs. 2. Corrects a small error where the values of those checks are just checked for their presence and not whether its true or false.

Signed-off-by: Mark Weiman

Thanks for the BSD fixes! It is clear that we do not test there very often...

...

--- meson.build | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build index 579ff2ed..483c4fbd 100644 --- a/meson.build +++ b/meson.build @@ -154,6 +154,9 @@ foreach member : [ ['struct statvfs', 'f_flag', '''#include '''], ['struct statfs', 'f_flags', '''#include #include '''], + ['struct statvfs', 'f_mntonname', '''#include '''], + ['struct statfs', 'f_mntonname', '''#include + #include '''],

...

] have = cc.has_member(member[0], member[1], prefix : member[2]) conf.set('HAVE_' + '_'.join([member[0], member[1]]).underscorify().to_upper(), have) @@ -174,9 +177,13 @@ foreach type : [ endif endforeach

-if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs')

Should this be "if conf.get( ... true)", or "if not conf.get(" ?

...

-elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif

This just seems wrong to me. We require f_mntonname for either statfs or statvfs, so just test for this when making the original assignment to FSSTATSTYPE. It appears the f_flag(s) part is not actually useful in determining which of statfs or statvfs we should use. (As an aside, is there some statvfs without the f_mntonname field?) I also found this looking at the mountpoint code. It appears that the f_flag(s) only gives us access to whether the mountpoint is read-only. That also appears broken since the switch to meson (or earlier!): e.g. lib/libalpm/diskspace.c; #if defined(HAVE_GETMNTINFO_STATVFS) && defined(HAVE_STRUCT_STATVFS_F_FLAG) I don't see that first define being made anywhere. Allan

Mark Weiman

4:25 p.m.

New subject: [pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

On Mon, 2021-04-19 at 17:01 +1000, Allan McRae wrote:

...

On 17/4/21 1:45 pm, Mark Weiman wrote:

...
On FreeBSD, both `struct statvfs` and `struct statfs` satisfy the conditions where the `f_flag` and `f_flags` fields are present in both respectively.

This patch accomplishes a couple of things:

1. Adds a check for `f_mntonname` in both structs and makes a decision      to choose statfs if the field is not present in statvfs, but is in      statfs. 2. Corrects a small error where the values of those checks are just      checked for their presence and not whether its true or false.

Signed-off-by: Mark Weiman

Thanks for the BSD fixes! It is clear that we do not test there very often...

...
--- meson.build | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build index 579ff2ed..483c4fbd 100644 --- a/meson.build +++ b/meson.build @@ -154,6 +154,9 @@ foreach member : [      ['struct statvfs', 'f_flag', '''#include '''],      ['struct statfs', 'f_flags', '''#include                                      #include '''], +    ['struct statvfs', 'f_mntonname', '''#include '''], +    ['struct statfs', 'f_mntonname', '''#include +                                        #include '''],

OK

...
   ]    have = cc.has_member(member[0], member[1], prefix : member[2])    conf.set('HAVE_' + '_'.join([member[0], member[1]]).underscorify().to_upper(), have) @@ -174,9 +177,13 @@ foreach type : [    endif endforeach -if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)    conf.set('FSSTATSTYPE', 'struct statvfs')

Should this be "if conf.get( ... true)", or "if not conf.get(" ?

No, the singature to conf.get() is the key you're checking first, then the default value you want if it doesn't exist. In this case, we want false to be the default.

...

...
-elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif

This just seems wrong to me. We require f_mntonname for either statfs or statvfs, so just test for this when making the original assignment to FSSTATSTYPE. It appears the f_flag(s) part is not actually useful in determining which of statfs or statvfs we should use.

(As an aside, is there some statvfs without the f_mntonname field?)

I didn't quite know how to address this. On Arch Linux, it appears that f_mntonname doesn't exist at all, but on FreeBSD, it exists in statfs, but not statvfs. And yes, statvfs exists without f_mntonname. I figured getting it to build and choosing the correct one on both Linux and FreeBSD was what was important here and doing some homework later on what exactly to do. On Linux, its statvfs (I think), but on FreeBSD, that's statfs.

...

I also found this looking at the mountpoint code. It appears that the f_flag(s) only gives us access to whether the mountpoint is read- only. That also appears broken since the switch to meson (or earlier!):

e.g. lib/libalpm/diskspace.c;

#if defined(HAVE_GETMNTINFO_STATVFS) && defined(HAVE_STRUCT_STATVFS_F_FLAG)

I don't see that first define being made anywhere.

I'll look at it further later.

...

Allan

Mark

Allan McRae

20 Apr 20 Apr

4:26 a.m.

New subject: [pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

On 20/4/21 12:25 am, Mark Weiman wrote:

...

On Mon, 2021-04-19 at 17:01 +1000, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
On FreeBSD, both `struct statvfs` and `struct statfs` satisfy the conditions where the `f_flag` and `f_flags` fields are present in both respectively.

This patch accomplishes a couple of things:

1. Adds a check for `f_mntonname` in both structs and makes a decision      to choose statfs if the field is not present in statvfs, but is in      statfs. 2. Corrects a small error where the values of those checks are just      checked for their presence and not whether its true or false.

Signed-off-by: Mark Weiman

Thanks for the BSD fixes! It is clear that we do not test there very often...

...
--- meson.build | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build index 579ff2ed..483c4fbd 100644 --- a/meson.build +++ b/meson.build @@ -154,6 +154,9 @@ foreach member : [      ['struct statvfs', 'f_flag', '''#include '''],      ['struct statfs', 'f_flags', '''#include                                      #include '''], +    ['struct statvfs', 'f_mntonname', '''#include '''], +    ['struct statfs', 'f_mntonname', '''#include +                                        #include '''],

OK

...
   ]    have = cc.has_member(member[0], member[1], prefix : member[2])    conf.set('HAVE_' + '_'.join([member[0], member[1]]).underscorify().to_upper(), have) @@ -174,9 +177,13 @@ foreach type : [    endif endforeach -if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)    conf.set('FSSTATSTYPE', 'struct statvfs')

Should this be "if conf.get( ... true)", or "if not conf.get(" ?

No, the singature to conf.get() is the key you're checking first, then the default value you want if it doesn't exist. In this case, we want false to be the default.

Thanks - it was suprisingly hard to find documentation for this...

...

...
...
-elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif

This just seems wrong to me. We require f_mntonname for either statfs or statvfs, so just test for this when making the original assignment to FSSTATSTYPE. It appears the f_flag(s) part is not actually useful in determining which of statfs or statvfs we should use.

(As an aside, is there some statvfs without the f_mntonname field?)

I didn't quite know how to address this. On Arch Linux, it appears that f_mntonname doesn't exist at all, but on FreeBSD, it exists in statfs, but not statvfs. And yes, statvfs exists without f_mntonname.

I figured getting it to build and choosing the correct one on both Linux and FreeBSD was what was important here and doing some homework later on what exactly to do. On Linux, its statvfs (I think), but on FreeBSD, that's statfs.

Ah... f_mntonname is only used with getmntinfo. So BSD land. I had forgotten the pain of implementing this... So now I am on the right page, lets go back and look at the patch! Here is the bit I am looking at: -if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs') -elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif My reading is this is about equivalent to: if conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') elif conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) conf.set('FSSTATSTYPE', 'struct statfs') endif Do we just fail on compile if FSSTATSTYPE is undefined? That seems not ideal. This really does not correspond to the checks in the code! HAVE_GETMNTENT -> statvfs HAVE_GETMNTINFO_STATVFS + HAVE_STRUCT_STATVFS_F_FLAG -> statvfs HAVE_GETMNTINFO_STATFS + HAVE_STRUCT_STATFS_F_FLAGS -> statfs I think we want to match the code (and fix any of the code that is wrong): if conf.get('HAVE_GETMNTENT', false) and conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_GETMNTINFO', false) and conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_GETMNTINFO', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif Looks like the F_FLAG/F_FLAGS check within the getmntinfo are only to see if we can detect read-only filesystems, and are not a requirement.

Mark Weiman

5:50 a.m.

New subject: [pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

On Tue, 2021-04-20 at 12:26 +1000, Allan McRae wrote:

...

On 20/4/21 12:25 am, Mark Weiman wrote:

...
On Mon, 2021-04-19 at 17:01 +1000, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
On FreeBSD, both `struct statvfs` and `struct statfs` satisfy the conditions where the `f_flag` and `f_flags` fields are present in both respectively.

This patch accomplishes a couple of things:

1. Adds a check for `f_mntonname` in both structs and makes a decision      to choose statfs if the field is not present in statvfs, but is in      statfs. 2. Corrects a small error where the values of those checks are just      checked for their presence and not whether its true or false.

Signed-off-by: Mark Weiman

Thanks for the BSD fixes! It is clear that we do not test there very often...

...
--- meson.build | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build index 579ff2ed..483c4fbd 100644 --- a/meson.build +++ b/meson.build @@ -154,6 +154,9 @@ foreach member : [      ['struct statvfs', 'f_flag', '''#include '''],      ['struct statfs', 'f_flags', '''#include                                      #include '''], +    ['struct statvfs', 'f_mntonname', '''#include '''], +    ['struct statfs', 'f_mntonname', '''#include +                                        #include '''],

OK

...
   ]    have = cc.has_member(member[0], member[1], prefix : member[2])    conf.set('HAVE_' + '_'.join([member[0], member[1]]).underscorify().to_upper(), have) @@ -174,9 +177,13 @@ foreach type : [    endif endforeach -if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)    conf.set('FSSTATSTYPE', 'struct statvfs')

Should this be "if conf.get( ... true)", or "if not conf.get(" ?

No, the singature to conf.get() is the key you're checking first, then the default value you want if it doesn't exist. In this case, we want false to be the default.

Thanks - it was suprisingly hard to find documentation for this...

...
...
...
-elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false)    conf.set('FSSTATSTYPE', 'struct statfs') endif

This just seems wrong to me. We require f_mntonname for either statfs or statvfs, so just test for this when making the original assignment to FSSTATSTYPE. It appears the f_flag(s) part is not actually useful in determining which of statfs or statvfs we should use.

(As an aside, is there some statvfs without the f_mntonname field?)

I didn't quite know how to address this. On Arch Linux, it appears that f_mntonname doesn't exist at all, but on FreeBSD, it exists in statfs, but not statvfs. And yes, statvfs exists without f_mntonname.

I figured getting it to build and choosing the correct one on both Linux and FreeBSD was what was important here and doing some homework later on what exactly to do. On Linux, its statvfs (I think), but on FreeBSD, that's statfs.

Ah... f_mntonname is only used with getmntinfo. So BSD land. I had forgotten the pain of implementing this...

So now I am on the right page, lets go back and look at the patch! Here is the bit I am looking at:

-if conf.has('HAVE_STRUCT_STATVFS_F_FLAG') +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)    conf.set('FSSTATSTYPE', 'struct statvfs') -elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS') +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) + conf.set('FSSTATSTYPE', 'struct statfs') +endif + +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false)    conf.set('FSSTATSTYPE', 'struct statfs') endif

My reading is this is about equivalent to:

if conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') elif conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false) conf.set('FSSTATSTYPE', 'struct statfs') endif

Do we just fail on compile if FSSTATSTYPE is undefined? That seems not ideal.

Yes, FSSTATSTYPE is used in alpm_mountpoint_t (lib/libalpm/diskspace.h). This *could* be remedied if needed.

...

This really does not correspond to the checks in the code!

HAVE_GETMNTENT -> statvfs HAVE_GETMNTINFO_STATVFS + HAVE_STRUCT_STATVFS_F_FLAG -> statvfs HAVE_GETMNTINFO_STATFS + HAVE_STRUCT_STATFS_F_FLAGS -> statfs

I think we want to match the code (and fix any of the code that is wrong):

if conf.get('HAVE_GETMNTENT', false) and conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_GETMNTINFO', false) and conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statvfs') elif conf.get('HAVE_GETMNTINFO', false) and conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false) conf.set('FSSTATSTYPE', 'struct statfs') endif

This is likely what would be appropriate, but perhaps with an else or some preprocessor check for alpm_mountpoint_t to prevent a compilation error if nothing in that block applies.

...

Looks like the F_FLAG/F_FLAGS check within the getmntinfo are only to see if we can detect read-only filesystems, and are not a requirement.

I can submit a v3 patch for this if you'd like. I wasn't entirely happy with the patches I provided since they felt "hacky" to me, but they did work on both FreeBSD and Arch Linux, so I felt it was at least a start. Mark

Allan McRae

6:30 a.m.

New subject: [pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

On 20/4/21 1:50 pm, Mark Weiman wrote:

...

I can submit a v3 patch for this if you'd like. I wasn't entirely happy with the patches I provided since they felt "hacky" to me, but they did work on both FreeBSD and Arch Linux, so I felt it was at least a start.

That would be great. I think this one was too hacky to put in as is. Allan

Mark Weiman

17 Apr 17 Apr

5:45 a.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

Allan McRae

19 Apr 19 Apr

9:10 a.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On 17/4/21 1:45 pm, Mark Weiman wrote:

...

This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D Patch looks good to me. Allan

...

Signed-off-by: Mark Weiman --- meson.build | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build index 483c4fbd..14b3381a 100644 --- a/meson.build +++ b/meson.build @@ -146,7 +146,9 @@ foreach sym : [ 'tcflush', ] have = cc.has_function(sym, args : '-D_GNU_SOURCE') - conf.set10('HAVE_' + sym.to_upper(), have) + if have + conf.set10('HAVE_' + sym.to_upper(), have) + endif endforeach

foreach member : [

Emil Velikov

1:34 p.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On Mon, 19 Apr 2021 at 08:10, Allan McRae wrote:

...

On 17/4/21 1:45 pm, Mark Weiman wrote:

...
This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D

Patch looks good to me.

Food for thought: Usually the more robust approach is to always set the respective defines to 0/1 and evaluate them directly (aka #if HAVE_). In addition one could set -Werror=undef in the build to catch any issues. This will produce clear traces with potential issues, while #if defined will silently fallback to the "other" path. If people are OK with the above, I will follow-up with some patches. Note: above suggestion is not meant to dismiss the original patch. -Emil

Allan McRae

4:05 p.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On 19/4/21 9:34 pm, Emil Velikov wrote:

...

On Mon, 19 Apr 2021 at 08:10, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D

Patch looks good to me.

Food for thought:

Usually the more robust approach is to always set the respective defines to 0/1 and evaluate them directly (aka #if HAVE_). In addition one could set -Werror=undef in the build to catch any issues.

This will produce clear traces with potential issues, while #if defined will silently fallback to the "other" path. If people are OK with the above, I will follow-up with some patches.

Note: above suggestion is not meant to dismiss the original patch.

I still live in the good old autotools days where there were lots of #undef in config.h... It appears some of that still happens. This is my build/config.h: #undef HAVE_STRUCT_STATFS_F_FLAGS #define HAVE_STRUCT_STATVFS_F_FLAG But it is not consistent. I'm not sure whether the better solution is to add more #undef, or to go to the #if 0/1 style. Allan

Mark Weiman

4:28 p.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On Tue, 2021-04-20 at 00:05 +1000, Allan McRae wrote:

...

On 19/4/21 9:34 pm, Emil Velikov wrote:

...
On Mon, 19 Apr 2021 at 08:10, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D

Patch looks good to me.

Food for thought:

Usually the more robust approach is to always set the respective defines to 0/1 and evaluate them directly (aka #if HAVE_). In addition one could set -Werror=undef in the build to catch any issues.

This will produce clear traces with potential issues, while #if defined will silently fallback to the "other" path. If people are OK with the above, I will follow-up with some patches.

Note: above suggestion is not meant to dismiss the original patch.

I still live in the good old autotools days where there were lots of #undef in config.h...

It appears some of that still happens. This is my build/config.h:

#undef HAVE_STRUCT_STATFS_F_FLAGS #define HAVE_STRUCT_STATVFS_F_FLAG

But it is not consistent.

This can be easily done by not using set10, but instead setting the values to true/false with set. That's why HAVE_STRUCT_STATFS_F_FLAGS is undef'd and those other ones that use set10 don't. I can modify this to mimick the old autotools behavior.

...

I'm not sure whether the better solution is to add more #undef, or to go to the #if 0/1 style.

Allan

Mark

Allan McRae

20 Apr 20 Apr

3:24 a.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On 20/4/21 12:28 am, Mark Weiman wrote:

...

On Tue, 2021-04-20 at 00:05 +1000, Allan McRae wrote:

...
On 19/4/21 9:34 pm, Emil Velikov wrote:

...
On Mon, 19 Apr 2021 at 08:10, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D

Patch looks good to me.

Food for thought:

Usually the more robust approach is to always set the respective defines to 0/1 and evaluate them directly (aka #if HAVE_). In addition one could set -Werror=undef in the build to catch any issues.

This will produce clear traces with potential issues, while #if defined will silently fallback to the "other" path. If people are OK with the above, I will follow-up with some patches.

Note: above suggestion is not meant to dismiss the original patch.

I still live in the good old autotools days where there were lots of #undef in config.h...

It appears some of that still happens. This is my build/config.h:

#undef HAVE_STRUCT_STATFS_F_FLAGS #define HAVE_STRUCT_STATVFS_F_FLAG

But it is not consistent.

This can be easily done by not using set10, but instead setting the values to true/false with set. That's why HAVE_STRUCT_STATFS_F_FLAGS is undef'd and those other ones that use set10 don't. I can modify this to mimick the old autotools behavior.

I have pulled the patch as-is, because it improves the current state. So any further changes can happen on top of that. (currently sitting in my patchqueue branch). I have no strong opinion whether the changes should have #undef, or just always defining as 0/1 and then using #if. It is clear that one of these needs to happen so we have consistency. Allan

Emil Velikov

19 Apr 19 Apr

4:46 p.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On Mon, 19 Apr 2021 at 15:05, Allan McRae wrote:

...

On 19/4/21 9:34 pm, Emil Velikov wrote:

...
On Mon, 19 Apr 2021 at 08:10, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D

Patch looks good to me.

Food for thought:

Usually the more robust approach is to always set the respective defines to 0/1 and evaluate them directly (aka #if HAVE_). In addition one could set -Werror=undef in the build to catch any issues.

This will produce clear traces with potential issues, while #if defined will silently fallback to the "other" path. If people are OK with the above, I will follow-up with some patches.

Note: above suggestion is not meant to dismiss the original patch.

I still live in the good old autotools days where there were lots of #undef in config.h...

It appears some of that still happens. This is my build/config.h:

#undef HAVE_STRUCT_STATFS_F_FLAGS #define HAVE_STRUCT_STATVFS_F_FLAG

But it is not consistent.

Speaking of consistency - here is a quick grep from an old-ish checkout: $ git grep HAVE_LIBGPGME ... lib/libalpm/be_sync.c:#ifndef HAVE_LIBGPGME // not defined, ok ... lib/libalpm/sync.c:#ifdef HAVE_LIBGPGME // is defined, ok meson.build:conf.set('HAVE_LIBGPGME', gpgme.found()) // hmm we always define it, right? Looking at the above one would say - we need CI to catch it. But we do - see the arch-no-gpg config, still has gpgme installed :-\ Since the headers are in the standard location, they end up used. Even if the final binary is missing the DT_NEEDED on gpgme. Mind you that link thing is likely meson just adding a linker flag (don't recall which one) to drop unneeded libraries from the DT_NEEDED list. Disclaimer: above is an educated guess, didn't stare at the build logs (yet). Hmm HAVE_LIBSSL, ENABLE_NLS seems similar.

...

I'm not sure whether the better solution is to add more #undef, or to go to the #if 0/1 style.

IMHO the above example/braindump illustrates why I'm in favour of 0/1 style. In addition, we had far too many bugs in Mesa and X (Xorg or otherwise) so we gradually switched to 0/1. -Emil

Mark Weiman

5:50 p.m.

New subject: [pacman-dev] [PATCH 2/5] meson.build: Fix detection of symbols

On Mon, 2021-04-19 at 15:46 +0100, Emil Velikov wrote:

...

On Mon, 19 Apr 2021 at 15:05, Allan McRae wrote:

...
On 19/4/21 9:34 pm, Emil Velikov wrote:

...
On Mon, 19 Apr 2021 at 08:10, Allan McRae wrote:

...
On 17/4/21 1:45 pm, Mark Weiman wrote:

...
This patch changes the behavior of meson to define configuration options *only* when the symbol checked is present. Currently, it defines all of them in config.h whether the symbol exists or not and the code that looks for it doesn't check the macro's value, but whether it's defined.

Remember back when we used autotools and all this just worked! :D

Patch looks good to me.

Food for thought:

Usually the more robust approach is to always set the respective defines to 0/1 and evaluate them directly (aka #if HAVE_). In addition one could set -Werror=undef in the build to catch any issues.

This will produce clear traces with potential issues, while #if defined will silently fallback to the "other" path. If people are OK with the above, I will follow-up with some patches.

Note: above suggestion is not meant to dismiss the original patch.

I still live in the good old autotools days where there were lots of #undef in config.h...

It appears some of that still happens. This is my build/config.h:

#undef HAVE_STRUCT_STATFS_F_FLAGS #define HAVE_STRUCT_STATVFS_F_FLAG

But it is not consistent.

Speaking of consistency - here is a quick grep from an old-ish checkout:

$ git grep HAVE_LIBGPGME ... lib/libalpm/be_sync.c:#ifndef HAVE_LIBGPGME // not defined, ok ... lib/libalpm/sync.c:#ifdef HAVE_LIBGPGME // is defined, ok meson.build:conf.set('HAVE_LIBGPGME', gpgme.found()) // hmm we always define it, right?

...

From my testing to write these patches, if gpgme.found() is false, an `#undef HAVE_LIBGPGME` will be set in config.h. This is where my note in one of my other emails came from. I would need to test this further, but a random "oh no, I forgot to install gpgme" after building didn't seem to cause any errors on FreeBSD.

...

Looking at the above one would say - we need CI to catch it. But we do - see the arch-no-gpg config, still has gpgme installed :-\ Since the headers are in the standard location, they end up used. Even if the final binary is missing the DT_NEEDED on gpgme. Mind you that link thing is likely meson just adding a linker flag (don't recall which one) to drop unneeded libraries from the DT_NEEDED list.

Disclaimer: above is an educated guess, didn't stare at the build logs (yet).

Hmm HAVE_LIBSSL, ENABLE_NLS seems similar.

...
I'm not sure whether the better solution is to add more #undef, or to go to the #if 0/1 style.

IMHO the above example/braindump illustrates why I'm in favour of 0/1 style. In addition, we had far too many bugs in Mesa and X (Xorg or otherwise) so we gradually switched to 0/1.

-Emil

Mark

Mark Weiman

17 Apr 17 Apr

5:45 a.m.

New subject: [pacman-dev] [PATCH 3/5] Add an include for signal.h when needed

On Linux, signal.h is not required to have access to the signal constants. On FreeBSD, this is not the case and requires signal.h to be explicitly included. This patch adds an include for signal.h in any source file that uses it. Signed-off-by: Mark Weiman --- This is a modified patch that just adds signal.h to required files instead of adding a check within meson. lib/libalpm/util.c | 1 + src/pacman/conf.c | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index b386fde6..46c1d0a1 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -33,6 +33,7 @@ #include #include #include +#include /* libarchive */ #include diff --git a/src/pacman/conf.c b/src/pacman/conf.c index a4f2ba35..cde96716 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -31,6 +31,7 @@ #include /* uname */ #include #include +#include /* pacman */ #include "conf.h" -- 2.31.1

Allan McRae

19 Apr 19 Apr

9:12 a.m.

New subject: [pacman-dev] [PATCH 3/5] Add an include for signal.h when needed

On 17/4/21 1:45 pm, Mark Weiman wrote:

...

On Linux, signal.h is not required to have access to the signal constants. On FreeBSD, this is not the case and requires signal.h to be explicitly included.

This patch adds an include for signal.h in any source file that uses it.

Signed-off-by: Mark Weiman --- This is a modified patch that just adds signal.h to required files instead of adding a check within meson.

Thanks. A

Mark Weiman

17 Apr 17 Apr

5:45 a.m.

New subject: [pacman-dev] [PATCH 4/5] Fix build error when SIGPOLL is not available

On Linux, SIGPOLL is a valid signal, but on systems like FreeBSD, it is not. This patch does a preprocessor check to see if SIGPOLL is available or not. Signed-off-by: Mark Weiman --- This is a modified patch where formatting is addressed and the check for SIGPOLL is done as a preprocessor macro rather than in meson. lib/libalpm/util.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 46c1d0a1..1d9d85dd 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -558,8 +558,12 @@ static void _alpm_reset_signals(void) int *i, signals[] = { SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGFPE, SIGHUP, SIGILL, SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSTOP, SIGTERM, SIGTSTP, - SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPOLL, SIGPROF, SIGSYS, SIGTRAP, - SIGURG, SIGVTALRM, SIGXCPU, SIGXFSZ, + SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPROF, SIGSYS, SIGTRAP, SIGURG, + SIGVTALRM, SIGXCPU, SIGXFSZ, +#if defined(SIGPOLL) + /* this is needed for FreeBSD et al. */ + SIGPOLL, +#endif 0 }; struct sigaction def; -- 2.31.1

Allan McRae

19 Apr 19 Apr

9:21 a.m.

New subject: [pacman-dev] [PATCH 4/5] Fix build error when SIGPOLL is not available

On 17/4/21 1:45 pm, Mark Weiman wrote:

...

On Linux, SIGPOLL is a valid signal, but on systems like FreeBSD, it is not. This patch does a preprocessor check to see if SIGPOLL is available or not.

Signed-off-by: Mark Weiman --- This is a modified patch where formatting is addressed and the check for SIGPOLL is done as a preprocessor macro rather than in meson.

lib/libalpm/util.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 46c1d0a1..1d9d85dd 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -558,8 +558,12 @@ static void _alpm_reset_signals(void) int *i, signals[] = { SIGABRT, SIGALRM, SIGBUS, SIGCHLD, SIGCONT, SIGFPE, SIGHUP, SIGILL, SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSTOP, SIGTERM, SIGTSTP, - SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPOLL, SIGPROF, SIGSYS, SIGTRAP, - SIGURG, SIGVTALRM, SIGXCPU, SIGXFSZ, + SIGTTIN, SIGTTOU, SIGUSR1, SIGUSR2, SIGPROF, SIGSYS, SIGTRAP, SIGURG, + SIGVTALRM, SIGXCPU, SIGXFSZ, +#if defined(SIGPOLL) + /* this is needed for FreeBSD et al. */

This comment made me thing SIGPOLL was a BSD specific signal. I changed the comment to make the issue more clear: + /* Not available on FreeBSD et al. */

...

+ SIGPOLL, +#endif 0 }; struct sigaction def;

Mark Weiman

17 Apr 17 Apr

5:45 a.m.

New subject: [pacman-dev] [PATCH 5/5] meson-make-symlink.sh: Make compatible with non-GNU ln

The "-T" flag is something available with GNU ln, but on other implementations of ln, "-T" is not an option. So, to make this script more portable, a strings check of ln for "GNU coreutils" is done to determine if "-T" should be used. Signed-off-by: Mark Weiman --- build-aux/meson-make-symlink.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/build-aux/meson-make-symlink.sh b/build-aux/meson-make-symlink.sh index 501cd43d..24a0dd2c 100644 --- a/build-aux/meson-make-symlink.sh +++ b/build-aux/meson-make-symlink.sh @@ -4,9 +4,13 @@ set -eu # this is needed mostly because $DESTDIR is provided as a variable, # and we need to create the target directory... +# test if ln is GNU or not +LN_FLAG="-T" +strings /bin/ln | grep -q 'GNU coreutils' || LN_FLAG="" + mkdir -vp "$(dirname "${DESTDIR:-}$2")" if [ "$(dirname $1)" = . ]; then - ln -vfs -T "$1" "${DESTDIR:-}$2" + ln -vfs ${LN_FLAG} "$1" "${DESTDIR:-}$2" else - ln -vfs -T --relative "${DESTDIR:-}$1" "${DESTDIR:-}$2" + ln -vfs ${LN_FLAG} --relative "${DESTDIR:-}$1" "${DESTDIR:-}$2" fi -- 2.31.1

Bjoern Bidar

2:44 p.m.

New subject: [pacman-dev] [PATCH 5/5] meson-make-symlink.sh: Make compatible with non-GNU ln

...

The "-T" flag is something available with GNU ln, but on other implementations of ln, "-T" is not an option. So, to make this script more portable, a strings check of ln for "GNU coreutils" is done to determine if "-T" should be used.

Signed-off-by: Mark Weiman --- build-aux/meson-make-symlink.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/build-aux/meson-make-symlink.sh b/build-aux/meson-make-symlink.sh index 501cd43d..24a0dd2c 100644 --- a/build-aux/meson-make-symlink.sh +++ b/build-aux/meson-make-symlink.sh @@ -4,9 +4,13 @@ set -eu # this is needed mostly because $DESTDIR is provided as a variable, # and we need to create the target directory...

+# test if ln is GNU or not +LN_FLAG="-T" +strings /bin/ln | grep -q 'GNU coreutils' || LN_FLAG="" No strings needed just do: ln --version|grep GNU

Or skip -T even with GNU ln.

Mark Weiman

8:52 p.m.

New subject: [pacman-dev] [PATCH 5/5] meson-make-symlink.sh: Make compatible with non-GNU ln

On Sat, 2021-04-17 at 15:44 +0300, Bjoern Bidar wrote:

...

...
The "-T" flag is something available with GNU ln, but on other implementations of ln, "-T" is not an option. So, to make this script more portable, a strings check of ln for "GNU coreutils" is done to determine if "-T" should be used.

Signed-off-by: Mark Weiman --- build-aux/meson-make-symlink.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/build-aux/meson-make-symlink.sh b/build-aux/meson-make-symlink.sh index 501cd43d..24a0dd2c 100644 --- a/build-aux/meson-make-symlink.sh +++ b/build-aux/meson-make-symlink.sh @@ -4,9 +4,13 @@ set -eu # this is needed mostly because $DESTDIR is provided as a variable, # and we need to create the target directory...

+# test if ln is GNU or not +LN_FLAG="-T" +strings /bin/ln | grep -q 'GNU coreutils' || LN_FLAG="" No strings needed just do: ln --version|grep GNU

Or skip -T even with GNU ln.

ln --version is not valid on all implementations, but I guess that could be a check... I can just remove -T altogether as well, also --relative may be an issue. Mark

Eli Schwartz

18 Apr 18 Apr

4:14 a.m.

New subject: [pacman-dev] [PATCH 5/5] meson-make-symlink.sh: Make compatible with non-GNU ln

On 4/17/21 2:52 PM, Mark Weiman wrote:

...

On Sat, 2021-04-17 at 15:44 +0300, Bjoern Bidar wrote:

...
...
The "-T" flag is something available with GNU ln, but on other implementations of ln, "-T" is not an option. So, to make this script more portable, a strings check of ln for "GNU coreutils" is done to determine if "-T" should be used.

Signed-off-by: Mark Weiman --- build-aux/meson-make-symlink.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/build-aux/meson-make-symlink.sh b/build-aux/meson-make-symlink.sh index 501cd43d..24a0dd2c 100644 --- a/build-aux/meson-make-symlink.sh +++ b/build-aux/meson-make-symlink.sh @@ -4,9 +4,13 @@ set -eu # this is needed mostly because $DESTDIR is provided as a variable, # and we need to create the target directory...

+# test if ln is GNU or not +LN_FLAG="-T" +strings /bin/ln | grep -q 'GNU coreutils' || LN_FLAG="" No strings needed just do: ln --version|grep GNU

Or skip -T even with GNU ln.

ln --version is not valid on all implementations, but I guess that could be a check...

I can just remove -T altogether as well, also --relative may be an issue.

You are warmly encouraged to rewrite this into something that supports FreeBSD ln, or any other simple POSIX ln... however, once you have done so that implementation will work fine even for GNU ln, so you'd better use it rather than some weird string search of the ln binary (which may not even be in /bin). While you're at it, --relative is hardly portable either... but I don't think we use this side of the logic fork? So that can simply be removed... A replacement for -T I guess (rather than simply removing it, which is pointless) would be if [ -d "${DESTDIR:-}$2" ]; then echo "notln: "${DESTDIR:-}$2": cannot overwrite directory" exit 1 fi but on the other hand we could try something really clever by rmdir'ing it so that the symlink succeeds... The FreeBSD ln -F "If the target file already exists and is a directory, then remove it so that the link may occur." seems ever so much more useful than GNU ln -F "allow root to try to hardlink directories, note that it will probably fail due to filesystem semantics". But, this is especially not portable so this is not a suitable replacement for -T either, even though -T is essentially just "FreeBSD's -F flag, but raises an error instead". ln -nf covers the other case where the destination is neither a file (-f) nor a directory (-fT or -F), but a symlink to a directory (-fT works here too). The -n option is implemented on a variety of ln programs, including GNU, busybox, FreeBSD, OpenBSD, NetBSD, macOS, etc. -- Eli Schwartz Bug Wrangler and Trusted User

Eli Schwartz

4:44 a.m.

New subject: [pacman-dev] [PATCH 5/5] meson-make-symlink.sh: Make compatible with non-GNU ln

On 4/17/21 10:14 PM, Eli Schwartz wrote:

...

On 4/17/21 2:52 PM, Mark Weiman wrote:

...
On Sat, 2021-04-17 at 15:44 +0300, Bjoern Bidar wrote:

...
...
The "-T" flag is something available with GNU ln, but on other implementations of ln, "-T" is not an option. So, to make this script more portable, a strings check of ln for "GNU coreutils" is done to determine if "-T" should be used.

Signed-off-by: Mark Weiman --- build-aux/meson-make-symlink.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/build-aux/meson-make-symlink.sh b/build-aux/meson-make-symlink.sh index 501cd43d..24a0dd2c 100644 --- a/build-aux/meson-make-symlink.sh +++ b/build-aux/meson-make-symlink.sh @@ -4,9 +4,13 @@ set -eu # this is needed mostly because $DESTDIR is provided as a variable, # and we need to create the target directory...

+# test if ln is GNU or not +LN_FLAG="-T" +strings /bin/ln | grep -q 'GNU coreutils' || LN_FLAG="" No strings needed just do: ln --version|grep GNU

Or skip -T even with GNU ln.

ln --version is not valid on all implementations, but I guess that could be a check...

I can just remove -T altogether as well, also --relative may be an issue.

You are warmly encouraged to rewrite this into something that supports FreeBSD ln, or any other simple POSIX ln... however, once you have done so that implementation will work fine even for GNU ln, so you'd better use it rather than some weird string search of the ln binary (which may not even be in /bin).

While you're at it, --relative is hardly portable either... but I don't think we use this side of the logic fork? So that can simply be removed...

A replacement for -T I guess (rather than simply removing it, which is pointless) would be

if [ -d "${DESTDIR:-}$2" ]; then echo "notln: "${DESTDIR:-}$2": cannot overwrite directory" exit 1 fi

but on the other hand we could try something really clever by rmdir'ing it so that the symlink succeeds...

The FreeBSD ln -F "If the target file already exists and is a directory, then remove it so that the link may occur." seems ever so much more useful than GNU ln -F "allow root to try to hardlink directories, note that it will probably fail due to filesystem semantics".

But, this is especially not portable so this is not a suitable replacement for -T either, even though -T is essentially just "FreeBSD's -F flag, but raises an error instead".

ln -nf covers the other case where the destination is neither a file (-f) nor a directory (-fT or -F), but a symlink to a directory (-fT works here too). The -n option is implemented on a variety of ln programs, including GNU, busybox, FreeBSD, OpenBSD, NetBSD, macOS, etc.

And I've implemented this all in the patch I just sent. -- Eli Schwartz Bug Wrangler and Trusted User

1106

Age (days ago)

1110

Last active (days ago)

List overview

Download

32 comments

6 participants

participants (6)

Allan McRae
Andrew Gregory
Bjoern Bidar
Eli Schwartz
Emil Velikov
Mark Weiman

[pacman-dev] [PATCH 0/4] Various fixes for FreeBSD (and perhaps others)

tags

participants (6)