<snip>
I also have a question on copyright. I take it you are doing this work
on behalf of your employer?
Yes
Do you know if ARM Limited (is that the
proper company name?)
That is the proper name AFAICT
has a copyright assignment agreement in place with
the FSF, that covers GMP, of if we need to do some paperwork?
I will find out.
Concrete suggestions:
Please drop the sed, grep and egrep configure checks, I think the job
can be done with portable mainstream features of sed and grep (and in
So, usually in a configure script you check that the tools needed
further down the road are available now rather than failing further down
because of a "command not found" error. For example, one checks that a C
compiler is available, it is assigned to variable CC and invoking it
would always use the CC variable. Likewise, Autoconf provides the
AC_PROG_SED and GREP checks as a standard way of doing this, and to
ensure interoperability across systems. They're even dedicated macros,
not even an auxiliary checks using something like AC_CHECK_PROG or friends.
So existing GMP code, without these patches, could really benefit from
using that autoconf standard mechanism. So I don't know why we would
drop that. I am more than happy to, but its fixing a bug, which is why I
broke it out into a separate patch.
addition, your use egrep looks suspicious, as I read it, is '(' used
both for grouping and as a literal character to match). See
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/autoconf.html#Limitations-of-Usual-Tools
for some advice on which features of sed and grep to avoid.
I'll take a closer look at this. But a quick peruse, I thought I needed
egrep for | but it looks like you just escape it \|. So that should be
easy enough to change to using regular grep.
Please drop configure check for readelf, just let the test script exit
with a log message and exit code 77 if readelf is unavalable.
I can, but I think the issue here is that when readelf isn't there, and
I am running the test in an environment where I want a hard failure, ie
the binary is built with support, I don't want to skip if readelf is
missing as something like a CI will still pass. I think if readelf is
missing, we can make this a hard failure?
It's interesting to me the push back on configure time checks for tools
needed under certain conditions and thus pushing the error further out
instead of providing a clear message to the user. I'm just curious what
the rationale is?
I would prefer to also drop the dependency on bash, are bash features
really essential for this test? From a quick look, I see no use of fancy
things like bash arrays. (I do see a mix of [ ... ] and bash-specific [[
... ]], which is a bit inconsistent).
local, pipefail and declare -g offhand. However, pipefail and local can
go away and declare -g goes back to eval. I had an eval there before and
someone talked me into declare -g. But yeah, we can remove this
requirement and do it POSIX shell.
Regards,
/Niels
Thanks Niels, appreciate the review.Bill
_______________________________________________
gmp-devel mailing list
[email protected]
https://gmplib.org/mailman/listinfo/gmp-devel
