Plans for IcedTea6 1.11

[DJ Lucas]

On 03/09/2011 10:28 AM, Dr Andrew John Hughes wrote:
> On 21:04 Tue 08 Mar     , DJ Lucas wrote:
>>
>> Okay, had a few extra minutes tonight so I reworked the patch for above
>> considerations, along with stream lining it a bit (all conditional on
>> the previous tests as the additional checks are not needed if a cacerts
>> file is supplied). I haven't completed a build yet (ran the configure
>> script and mkcacerts.sh script though various quick tests), but wondered
>> if the conditional ac tests are alright, or should they be run
>> unconditionally (unnecessarily)?
>>
> I've commented inline.
>
> If I follow all this right, it tries to detect a usable cacerts by default,
> right?  generate_cacerts will be set to "forced" and automatic detection
> applied if I specify no additional arguments.  I think that's the right
> thing if that's what this is doing.
>

Exactly.

> There's a general issue with handling yes/no in --with-x calls.  It's just
> a matter of coming up with a sensible way to handle these, depending on
> context and doing so.  It's not a major issue, but it would be better
> to handle this than go looking for 'yes' or 'no' especially if we search
> the patch and find /bin/yes.
>

Sorry so late, had a bunch of other stuff to do tonight. Anyway, fixed 
all cases to handle yes/no where a path value is expected (by empty 
variable where appropriate). There is a question about --with-cacerts: 
Where do the distros who install a system-wide cacerts file put it? Is 
it only distributed via a system certificates package, or in the icedtea 
jre/jdk package itself? If it is installed prior, we could add a 
testcase to find it and create a symlink instead of copy. I'm kind of 
the odd case, and do exactly as an end user would.

<Snip>

>> +  if test -z "${CADIR}"; then
> Better to use test -d here too?
>

No, I'm actually testing explicitly for a zero or empty value to see if 
it is unset, and if so, go through the find. Switched to x"${CADIR}" = 
"x" syntax to make that more clear. BTW, is elif portable? There are no 
current examples so I used a nested else if because I wasn't sure.

>> +    for dir in /etc/pki/tls/certs \
>> +      /usr/share/ca-certificates \
>> +      /etc/ssl/certs \
>> +      /etc/certs ; do
>> +        if test -d "${dir}"; then
>> +          CADIR="${dir}"
>> +          break
>> +        fi
>> +    done
>> +    if test -z "${CADIR}"; then
>> +      CADIR=no
>> +    fi
>> +  fi
>> +  AC_MSG_RESULT(${CADIR})
>> +  AC_SUBST(CADIR)
>> +  if test x"${CADIR}" = "xno"; then
>> +    IT_GET_LOCAL_CAFILE
> Is there a reason directories have preference over files?
>

Yes, the mkcacerts.sh script does not have to separate the certificates 
and shaves a few seconds off of the run on my machine when using 
directory over file, but I've combined both tests as was done 
previously. One of the two has to take precedence if both are found. As 
it is now, that is CADIR. If you wanted to use --with-ca-file=somefile 
then you'd also have to pass --without-ca-dir. Not sure of an elegant 
way to handle that.

>> +  fi
>> +])
>> +
>> +AC_DEFUN([IT_GET_LOCAL_CAFILE],
>> +[
>> +  AC_MSG_CHECKING([for a local x509 certificate file])
>> +  AC_ARG_WITH([ca-file],
>> +              [AS_HELP_STRING(--with-ca-file=FILE, specify a local x509 certificate file for cacerts generation)],
>> +  [
>> +    if test -f "${withval}"; then
>> +      CAFILE="${withval}"
>> +    fi
>> +  ],
>> +  [
>> +    CAFILE=
>> +  ])
> Again, yes/no.
>
>> +  if test -z "${CAFILE}"; then
> test -e.

Again, same thing, CAFILE is set by the testcase (yes/no are fixed) and 
I'm explicitly testing for an empty value, but x"${CAFILE}" = "x" makes 
the intention more clear.


<Snip>


>>   IT_GET_PKGVERSION
>>   IT_GET_LSB_DATA
>>
>> diff -Naur icedtea6-1.10-orig/Makefile.am icedtea6-1.10/Makefile.am
>> --- icedtea6-1.10-orig/Makefile.am	2011-03-02 13:48:14.000000000 -0600
>> +++ icedtea6-1.10/Makefile.am	2011-03-08 20:58:36.000000000 -0600
>> @@ -1384,25 +1384,31 @@
>>
>>   stamps/icedtea-against-icedtea.stamp: stamps/icedtea.stamp \
>>    stamps/add-jamvm.stamp stamps/add-cacao.stamp stamps/add-zero.stamp \
>> - stamps/add-systemtap.stamp stamps/add-pulseaudio.stamp stamps/add-nss.stamp stamps/add-tzdata-support.stamp
>> + stamps/add-systemtap.stamp stamps/add-pulseaudio.stamp stamps/add-nss.stamp \
>> + stamps/add-tzdata-support.stamp stamps/copy-cacerts.stamp \
>> + stamps/generate-cacerts.stamp
> I would do these as one target, stamps/add-cacerts.stamp (rename the current copy-cacerts).
>

Combined and separated. :-)

<Snip>

> You miss cacerts for the -ecj targets.
>

Oops...had only thought of ecj target as stage1 for bootstrap. It simply 
hadn't crossed my mind. :-)  Added.

> I assume the script below works.  It looks ok.  I guess issues will be found in
> testing on various systems.
>

Actually there was an error in the script that was in the previous 
patch...for testing I had temporarily deleted all ouptut in the date 
check (/d) and forgotten to put it back. Now that I have more than a few 
moments free, it's been tested pretty thoroughly. Fixed in revised 
patch.  All other snipped comments covered as suggested.

Thanks for the thorough review. Revised patch attached.

-- DJ


-- 
This message has been scanned for viruses and
dangerous content, and is believed to be clean.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: icedtea6-1.10-cacerts-5.patch
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110310/f9719948/icedtea6-1.10-cacerts-5.patch