setup

setup

[Achim Gratz]

The new SHA512 checksums are rather lengthy.  Could we switch them to
Base64 (perhaps the URL and file safe variant) instead of the current
hex encoding instead (maybe with an SHA512: prefix if we want to support
both)?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

On May 29 21:37, Achim Gratz wrote:
> 
> The new SHA512 checksums are rather lengthy.  Could we switch them to
> Base64 (perhaps the URL and file safe variant) instead of the current
> hex encoding instead (maybe with an SHA512: prefix if we want to support
> both)?

Not for the time being.  Sombody would have to add code to setup as well
as upset to make this work.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> On May 29 21:37, Achim Gratz wrote:
>> 
>> The new SHA512 checksums are rather lengthy.  Could we switch them to
>> Base64 (perhaps the URL and file safe variant) instead of the current
>> hex encoding instead (maybe with an SHA512: prefix if we want to support
>> both)?
>
> Not for the time being.  Sombody would have to add code to setup as well
> as upset to make this work.

Since it seems you plan to use libcrypto from OpenSSL anyway:
https://gist.github.com/barrysteyn/7308212#file-base64decode-c

Otherwise I'd prefer something less obtuse, there is C++ code with
compatible license or I could roll my own.

For upset, I don't know what interface you're using.  For the object
interface you'd simply switch from hexdigest() to b64digest() and for
the functional interface from sha512_hex() to sha512_base64().


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Jun  1 20:11, Achim Gratz wrote:
> Corinna Vinschen writes:
> > On May 29 21:37, Achim Gratz wrote:
> >> 
> >> The new SHA512 checksums are rather lengthy.  Could we switch them to
> >> Base64 (perhaps the URL and file safe variant) instead of the current
> >> hex encoding instead (maybe with an SHA512: prefix if we want to support
> >> both)?
> >
> > Not for the time being.  Sombody would have to add code to setup as well
> > as upset to make this work.
> 
> Since it seems you plan to use libcrypto from OpenSSL anyway:
> https://gist.github.com/barrysteyn/7308212#file-base64decode-c

I don't.  Why do you think so?

> Otherwise I'd prefer something less obtuse, there is C++ code with
> compatible license or I could roll my own.
> 
> For upset, I don't know what interface you're using.  For the object
> interface you'd simply switch from hexdigest() to b64digest() and for
> the functional interface from sha512_hex() to sha512_base64().

I'm not a perl person.  Upset is using some kind of

  use Digest::SHA;
  [...]
  my $ctx = Digest::SHA->new(512);
  [...]
  } elsif ($f eq "$d0/sha512.sum") {

This was a drop-in replacement for Digest::MD5.  Anything else I'll
let somebody do who better knows perl.

Apart from this, the sah512.sum files are not only generated for the
Cygwin release dir, they are generated on the sourceware ftp area
system-wide.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> Since it seems you plan to use libcrypto from OpenSSL anyway:
>> https://gist.github.com/barrysteyn/7308212#file-base64decode-c
>
> I don't.  Why do you think so?

Because of the openssl branch in Git.

> This was a drop-in replacement for Digest::MD5.  Anything else I'll
> let somebody do who better knows perl.

THen the call $ctx->hexdigest() would need to be replaced by
$ctx->b64digest().

> Apart from this, the sah512.sum files are not only generated for the
> Cygwin release dir, they are generated on the sourceware ftp area
> system-wide.

I'm only talking about SHA512 in the setup.ini files, though.  The
sha512.sum files have no bearing on that.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Jun  1 21:49, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> Since it seems you plan to use libcrypto from OpenSSL anyway:
> >> https://gist.github.com/barrysteyn/7308212#file-base64decode-c
> >
> > I don't.  Why do you think so?
> 
> Because of the openssl branch in Git.

Oh, that was a bad idea.  I won't remove the branch for historical
reasons, but we don't really want to link agaionst OpenSSL when we
already link against gcrypt.

> > This was a drop-in replacement for Digest::MD5.  Anything else I'll
> > let somebody do who better knows perl.
> 
> THen the call $ctx->hexdigest() would need to be replaced by
> $ctx->b64digest().

No, it would need to support this additionally.

> > Apart from this, the sah512.sum files are not only generated for the
> > Cygwin release dir, they are generated on the sourceware ftp area
> > system-wide.
> 
> I'm only talking about SHA512 in the setup.ini files, though.  The
> sha512.sum files have no bearing on that.

So what exactly is the problem with the longer checksums?  I don't
see any problem here, especially when the file is being compressed
anyway.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> Because of the openssl branch in Git.
>
> Oh, that was a bad idea.  I won't remove the branch for historical
> reasons, but we don't really want to link agaionst OpenSSL when we
> already link against gcrypt.

OK, makes sense.

>> > This was a drop-in replacement for Digest::MD5.  Anything else I'll
>> > let somebody do who better knows perl.
>> 
>> THen the call $ctx->hexdigest() would need to be replaced by
>> $ctx->b64digest().
>
> No, it would need to support this additionally.

Why?  In any case it'd be easy enough to make it switchable.

>> > Apart from this, the sah512.sum files are not only generated for the
>> > Cygwin release dir, they are generated on the sourceware ftp area
>> > system-wide.
>> 
>> I'm only talking about SHA512 in the setup.ini files, though.  The
>> sha512.sum files have no bearing on that.
>
> So what exactly is the problem with the longer checksums?  I don't
> see any problem here, especially when the file is being compressed
> anyway.

I'm not concerned about the size (although I note that even if
compression recognized it's looking at an SHA512 it would at best be
able to compress it down to 64 bytes anyway).  The difference between
128 and 86 (or 88 padded) characters is pretty noticeable on screen.
It's probably a bit unusual that I look at setup files so often (since I
generate my own), but it still disturbes me.  The other thing (but that
really is unrelated, as I could take care of it on other ways) is that I
still plan to move to a repository format that allows multiple versions
to be active at the same time (via different setup.ini files pointing
into the same repo hierarchy) and I was thinking of using the SHA512 to
do directory balancing like it's done for the object directory in Git.

~ (2001) du -sh /mnt/mirror/cygwin
52G     /mnt/mirror/cygwin
~ (2002) find /mnt/mirror/cygwin -type f |wc
  44778   44778 3340383
~ (2003) find /mnt/mirror/cygwin -type d | wc
  11079   11079  602401
~ (2004) ls -d /mnt/mirror/cygwin/x86/release/* | wc
   2071    2071   89831

Using a 6bit key into a two level tree would seem to provide a better
balance for this structure than 8bit or I could go single level with a
12bit key.  It needs to provide optimum performance over network, so the
optimum likely is at a different point than a local file system.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3121 bytes --]

On Jun  2 07:17, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> Because of the openssl branch in Git.
> >
> > Oh, that was a bad idea.  I won't remove the branch for historical
> > reasons, but we don't really want to link agaionst OpenSSL when we
> > already link against gcrypt.
> 
> OK, makes sense.
> 
> >> > This was a drop-in replacement for Digest::MD5.  Anything else I'll
> >> > let somebody do who better knows perl.
> >> 
> >> THen the call $ctx->hexdigest() would need to be replaced by
> >> $ctx->b64digest().
> >
> > No, it would need to support this additionally.
> 
> Why?  In any case it'd be easy enough to make it switchable.

Transition period.  We should do changes like that in two steps, first
updating to a setup which handles the new checksums, then changing the
generation of checksums to the new method.  Like we did with
MD5->SHA512.  It's smoother that way.  It's also certainly not a bad
idea to continue supporting the older checksum methods.  You're very
likely not the only person creating his/her own setup.ini files and
every change may break a script elsewhere :)

> >> > Apart from this, the sah512.sum files are not only generated for the
> >> > Cygwin release dir, they are generated on the sourceware ftp area
> >> > system-wide.
> >> 
> >> I'm only talking about SHA512 in the setup.ini files, though.  The
> >> sha512.sum files have no bearing on that.
> >
> > So what exactly is the problem with the longer checksums?  I don't
> > see any problem here, especially when the file is being compressed
> > anyway.
> 
> I'm not concerned about the size (although I note that even if
> compression recognized it's looking at an SHA512 it would at best be
> able to compress it down to 64 bytes anyway).  The difference between
> 128 and 86 (or 88 padded) characters is pretty noticeable on screen.
> It's probably a bit unusual that I look at setup files so often (since I
> generate my own),

You seem to be doing this on a regular basis with your own scripts.  Is
that right?  Would you think your own method is just hacked to work, or
do you think your own ini creation scripts are clean enough to release
them to the public?

I'm asking because, right now, we're relying on a convoluted perl script
set which is hard to understand (at least for non-perl guys), is missing
comments, has no maintainer, and above all, has a questionable license.

Upset is a beast.  It handles ini file creation as well as creating the
package information for https://cygwin.com/cgi-bin2/package-grep.cgi,
as well as the package upload post-processing.

I would very much appreciate if we could split these tasks into separate,
independent scripts or tools under GPL or BSD license.

So, what about your scripts?  Do you think they could be used as a
start?  Do you have, perhaps, fun to rewrite the upset functionality in
a maintainable form and *gasp* maintain it?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> Why?  In any case it'd be easy enough to make it switchable.
>
> Transition period.  We should do changes like that in two steps, first
> updating to a setup which handles the new checksums, then changing the
> generation of checksums to the new method.  Like we did with
> MD5->SHA512.  It's smoother that way.  It's also certainly not a bad
> idea to continue supporting the older checksum methods.  You're very
> likely not the only person creating his/her own setup.ini files and
> every change may break a script elsewhere :)

Yes, but in this case you're really talking about the setup.exe side,
not the generation of the setup via upset or genini (which I thought you
were talking about, since that's the part implemented in Perl).  If we
use the SHA512: prefix then both hex and base64 encoded checksum are
distinguishable easily and we might even throw in MD5 just for fun.  I
don't see off-hand why either upset or genini should be capable of
emitting both styles into the same setup.ini, but it is easy enough to
do this per setup.ini file creation by adding an option to specify which
digest you want.

> You seem to be doing this on a regular basis with your own scripts.  Is
> that right?  Would you think your own method is just hacked to work, or
> do you think your own ini creation scripts are clean enough to release
> them to the public?

No, it's fairly sophisticated already, but not yet feature-complete and
requires a few clean-ups (don't they always?).

> I'm asking because, right now, we're relying on a convoluted perl script
> set which is hard to understand (at least for non-perl guys), is missing
> comments, has no maintainer, and above all, has a questionable license.

What I have is also mostly a Perl script plus a few CMD files around it,
so that part likely wouldn't change.

> Upset is a beast.  It handles ini file creation as well as creating the
> package information for https://cygwin.com/cgi-bin2/package-grep.cgi,
> as well as the package upload post-processing.

Since I can't even look at upset, I use genini for the initial setup.ini
creation for any local packages (I have many of those).  The rest of the
process is taking multiple setup.ini files and rewrites them into a new
one, then installs the necessary files into a staging area, which can
optionally be cleaned of files not being referenced anymore after an
update or change of packages to install.  The staging area is then
replicated onto the real installation directory.  This is under control
of a setup.conf file that looks more or less exactly like the name
implies.  You define your package sources and their dependency order
(should they provide the same package, which is what happens for
packages that I carry local patches for), and then list the packages you
want to have available (you can inject new group names so it's easier to
install via setup.exe).  Dependencies are pulled in automatically and
you can specify that you want an experimental or previous package
instead of current.  It can also automatically pull devel and debug
packages into groups so that they do not clutter the install for
non-developer installs and are by default available on those install
that are for developers.  You can also blacklist a package in some
package source if you want to reverse the order just for one package or
if there's a packaging error in an external package source.

> I would very much appreciate if we could split these tasks into separate,
> independent scripts or tools under GPL or BSD license.

Well, if we keep this as a task for Perl it would make most sense to put
those things into Perl modules, one per task.  If a standalone script is
preferred in some place we can always fatpack those modules again.

> So, what about your scripts?  Do you think they could be used as a
> start?  Do you have, perhaps, fun to rewrite the upset functionality in
> a maintainable form and *gasp* maintain it?

As I said, I'm coming at this from a different angle: the need to
integrate multiple package sources and make a new install hierarchy that
can be used locally, without being connected to the Internet and without
carrying all the baggage around that a full mirror would have.  Plus, I
need the install to be fully scriptable to install for users that have
no admin rights.

The features that I'm missing is explicit version pinning (unfortunately
I'll have to do some extensive changes under the hood to get rid of the
current prev-curr-test structure, didn't think of that early enough) and
the ability to keep different versions of an installation around.  I
sort of can do that already if I'm careful enough not to clean away
files that are still needed, but it's too fragile even for very sparse
usage, so I use the staging directory for release tests at the moment.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

On Jun  2 18:24, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> Why?  In any case it'd be easy enough to make it switchable.
> >
> > Transition period.  We should do changes like that in two steps, first
> > updating to a setup which handles the new checksums, then changing the
> > generation of checksums to the new method.  Like we did with
> > MD5->SHA512.  It's smoother that way.  It's also certainly not a bad
> > idea to continue supporting the older checksum methods.  You're very
> > likely not the only person creating his/her own setup.ini files and
> > every change may break a script elsewhere :)
> 
> Yes, but in this case you're really talking about the setup.exe side,

Yes, sure.

> not the generation of the setup via upset or genini (which I thought you
> were talking about, since that's the part implemented in Perl).  If we
> use the SHA512: prefix then both hex and base64 encoded checksum are
> distinguishable easily and we might even throw in MD5 just for fun.

There's no reason for upset to create more than one style of checksum.
MD5, SHA512, SHA512/base64, whatever.  What I'm talking about is the
client side.  The setup tool should understand all types of checksums
used.  Right now it identifies MD5 vs. SHA512 just by the length of
the checksum.

> > You seem to be doing this on a regular basis with your own scripts.  Is
> > that right?  Would you think your own method is just hacked to work, or
> > do you think your own ini creation scripts are clean enough to release
> > them to the public?
> 
> No, it's fairly sophisticated already, but not yet feature-complete and
> requires a few clean-ups (don't they always?).
> 
> > I'm asking because, right now, we're relying on a convoluted perl script
> > set which is hard to understand (at least for non-perl guys), is missing
> > comments, has no maintainer, and above all, has a questionable license.
> 
> What I have is also mostly a Perl script plus a few CMD files around it,
> so that part likely wouldn't change.

CMD is out of the question, this running on a Linux box.

> > Upset is a beast.  It handles ini file creation as well as creating the
> > package information for https://cygwin.com/cgi-bin2/package-grep.cgi,
> > as well as the package upload post-processing.
> 
> Since I can't even look at upset, I use genini for the initial setup.ini

I forgot that the upset repo has been hidden from the public for a long
time, and these days there isn't even a repo since it has been deleted :-P

If you're willing to take a look into these upset perl scripts, please
apply for a normal ssh account on sourceware.org via

  https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Use my private email address as approver.  And make sure to use
*another* public SSH key for shell access than the one you're using
for Cygwin uploads to avoid confusion.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Marco Atzeri]

On 6/3/2015 10:19 AM, Corinna Vinschen wrote:

>
> If you're willing to take a look into these upset perl scripts, please
> apply for a normal ssh account on sourceware.org via
>
>    https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>
> Use my private email address as approver.  And make sure to use
> *another* public SSH key for shell access than the one you're using
> for Cygwin uploads to avoid confusion.

Why ?
I am using the same KEY for both and never had an issue.

> Thanks,
> Corinna

Regards
Marco

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

On Jun  3 10:31, Marco Atzeri wrote:
> On 6/3/2015 10:19 AM, Corinna Vinschen wrote:
> 
> >
> >If you're willing to take a look into these upset perl scripts, please
> >apply for a normal ssh account on sourceware.org via
> >
> >   https://sourceware.org/cgi-bin/pdw/ps_form.cgi
> >
> >Use my private email address as approver.  And make sure to use
> >*another* public SSH key for shell access than the one you're using
> >for Cygwin uploads to avoid confusion.
> 
> Why ?
> I am using the same KEY for both and never had an issue.

I had problems with the home directory, but maybe that was just me.  I
think Eric had some problem as well, but maybe that's just a problem for
the cygwin with admin rights, I'm not sure.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On 06/03/2015 05:07 AM, Corinna Vinschen wrote:
> On Jun  3 10:31, Marco Atzeri wrote:
>> On 6/3/2015 10:19 AM, Corinna Vinschen wrote:
>>
>>>
>>> If you're willing to take a look into these upset perl scripts, please
>>> apply for a normal ssh account on sourceware.org via
>>>
>>>   https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>>>
>>> Use my private email address as approver.  And make sure to use
>>> *another* public SSH key for shell access than the one you're using
>>> for Cygwin uploads to avoid confusion.
>>
>> Why ?
>> I am using the same KEY for both and never had an issue.
> 
> I had problems with the home directory, but maybe that was just me.  I
> think Eric had some problem as well, but maybe that's just a problem for
> the cygwin with admin rights, I'm not sure.

Same key for both personal login and package upload is fine. The only
time two keys are needed is if you are granted admin rights as the
'cygwin' user.  In that case, package upload and admin login need
different keys, but it is then up to you whether your personal login
shares with the upload key or with the admin access.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> There's no reason for upset to create more than one style of checksum.
> MD5, SHA512, SHA512/base64, whatever.  What I'm talking about is the
> client side.  The setup tool should understand all types of checksums
> used.  Right now it identifies MD5 vs. SHA512 just by the length of
> the checksum.

OK, that's easy, then.  Just check for 86 and 88  characters for the
non-padded and padded variant of the base64 digest.

> CMD is out of the question, this running on a Linux box.

The scripts just set up the environment and then call to Perl.  It's
really a lot easier to do in (ba)sh than in cmd, so that doesn't need to
worry you.

Again, the origin of those scripts was a different task, so I'd like to
spend some time on discussion of the feature set you're envisioning for
this.

> I forgot that the upset repo has been hidden from the public for a long
> time, and these days there isn't even a repo since it has been deleted :-P
>
> If you're willing to take a look into these upset perl scripts, please
> apply for a normal ssh account on sourceware.org via
>
>   https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>
> Use my private email address as approver.  And make sure to use
> *another* public SSH key for shell access than the one you're using
> for Cygwin uploads to avoid confusion.

I'll do that, but expect no comment before Perl 5.22 gets out the door.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Yaakov Selkowitz]

On Wed, 2015-06-03 at 10:19 +0200, Corinna Vinschen wrote:
> On Jun  2 18:24, Achim Gratz wrote:
> > Corinna Vinschen writes:
> > > Upset is a beast.  It handles ini file creation as well as creating the
> > > package information for https://cygwin.com/cgi-bin2/package-grep.cgi,
> > > as well as the package upload post-processing.
> > 
> > Since I can't even look at upset, I use genini for the initial setup.ini
> 
> I forgot that the upset repo has been hidden from the public for a long
> time, and these days there isn't even a repo since it has been deleted :-P
> 
> If you're willing to take a look into these upset perl scripts,

Anyone looking at the sources would be tainted thereby.  Because we are
concerned about the licensing of upset, AFAICS we are going to have to
clean-room reverse-engineer this.

--
Yaakov

Re: setup

[Achim Gratz]

Yaakov Selkowitz writes:
> Anyone looking at the sources would be tainted thereby.  Because we are
> concerned about the licensing of upset, AFAICS we are going to have to
> clean-room reverse-engineer this.

So what exactly does upset that genini doesn't when it comes to
generating setup.ini and what other functions does it do in the
maintenance of the download area (which I assume is the part where it
gets hairy)?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Achim Gratz]

Achim Gratz writes:
> The new SHA512 checksums are rather lengthy.  Could we switch them to
> Base64 (perhaps the URL and file safe variant) instead of the current
> hex encoding instead (maybe with an SHA512: prefix if we want to support
> both)?

I've implemented this in setup and genini to create base64url encoded
checksums.  Setup deals nicely with a setup.ini that has a mix of MD5
(some packages from ports), SHA512-hex (all of cygwin) and SHA512-Base64
(my local packages) checksums.  The latter ones are base64url as defined
in RFC4648, which means the '+' and '/' characters get replaced by '-'
and '_' to be filename and URL safe.  Like this:

--8<---------------cut here---------------start------------->8---
install: ../x86/cygwin/docbook2X/docbook2X-0.8.8-1.tar.bz2 286260 a2b46ebd6aeb73eebe3f001ee847fa9bc1b46231b58fc728c25197e7750cfd55285d57e77dbf5d093f80362396fa4bc4dde740cc0d0f8fcb1561278410dbebba
install: ../noarch/cygport/docbook2odf/docbook2odf-0.244-1.tar.bz2 25411 289b39695a7e388c2f2ac5b8c752fa6b
install: ../x86/patches/perl-Class-Factory-Util/perl-Class-Factory-Util-1.7-3.tar.xz 12420 FUfRHLQiyuK8PT4OOd7TH-2y4IgexoUxCVPJo-1_-pGAkyIQh0sbth92Nrk_wdA-YMdW3BSOLpa12gfoEJsh8g
--8<---------------cut here---------------end--------------->8---

I'll have to write a ChangeLog and clean up the code a bit before I'll
post the patches.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: setup

[Achim Gratz]

Achim Gratz writes:
> I'll have to write a ChangeLog and clean up the code a bit before I'll
> post the patches.

Here's the patch for setup:

--8<---------------cut here---------------start------------->8---
From 3ce2ac3109f7acb38491b74bd48751ef76eae42c Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Sun, 7 Jun 2015 09:10:13 +0200
Subject: [PATCH] Implement Base64URL-encoded SHA512 checksums

	* ini.h: Add macros for use within the implementation of the
	checksum parsers.  Hexdigest requires a 2-to-1 and Base64 a 4-to-3
	conversion.  Base64 uses the filename and URL safe alphabet from
	RFC4648.  It would be trivial to additionally process the normal
	Base64 alphabet but we don't want to allow that in order to be
	able to use the checksums as filenames without further conversion.
	(hexnibble, b64url): Process single input character to input value.
	(nibbled1): Convert 2 processed input values into 1 output value.
	(b64d1, b64d2, b64d3): Convert 4 input values into 3 output
	values.
	* iniparse.yy: Add SHA512B64URL checksum in addition to MD5 and
	SHA512.  Keep symmetry with MD5LINE and also define SHA512LINE
	syntax.
	* inilex.ll: Implement existing MD5 and SHA512 checksum parsers
	using new macros.  Implement new SHA512B64URL parser using new
	macros.  Enforce Base64 alphabet as defined in RFC4648 with no
	padding to enable direct use of checksum values as filenames.
	Implement SHA512LINE parser.
---
 ChangeLog   | 21 +++++++++++++++++++++
 ini.h       | 14 ++++++++++++++
 inilex.ll   | 55 +++++++++++++++++++++++++++++++++++++++----------------
 iniparse.yy |  9 +++++++--
 4 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 854ba05..b2b3ae0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,24 @@
+2015-06-07  Achim Gratz  <Stromeko@NexGo.DE>
+
+	* ini.h: Add macros for use within the implementation of the
+	checksum parsers.  Hexdigest requires a 2-to-1 and Base64 a 4-to-3
+	conversion.  Base64 uses the filename and URL safe alphabet from
+	RFC4648.  It would be trivial to additionally process the normal
+	Base64 alphabet but we don't want to allow that in order to be
+	able to use the checksums as filenames without further conversion.
+	(hexnibble, b64url): Process single input character to input value.
+	(nibbled1): Convert 2 processed input values into 1 output value.
+	(b64d1, b64d2, b64d3): Convert 4 input values into 3 output
+	values.
+	* iniparse.yy: Add SHA512B64URL checksum in addition to MD5 and
+	SHA512.  Keep symmetry with MD5LINE and also define SHA512LINE
+	syntax.
+	* inilex.ll: Implement existing MD5 and SHA512 checksum parsers
+	using new macros.  Implement new SHA512B64URL parser using new
+	macros.  Enforce Base64 alphabet as defined in RFC4648 with no
+	padding to enable direct use of checksum values as filenames.
+	Implement SHA512LINE parser.
+
 2015-03-25  Corinna Vinschen  <corinna@vinschen.de>
 
 	* install.cc (sha512_one): Raise buffer size to 64K for performance
diff --git a/ini.h b/ini.h
index 232c600..ec09def 100644
--- a/ini.h
+++ b/ini.h
@@ -52,4 +52,18 @@ extern std::string current_ini_sig_name;  /* current filename/URL for sig file *
 extern std::string yyerror_messages;  /* textual parse error messages */
 extern int yyerror_count;             /* number of parse errors */
 
+/* The following definitions are used in the parser implementation */
+
+#define hexnibble(val) (255 & (val > '9') ? val - 'a' + 10 : val - '0');
+#define nibbled1(v1,v2) (255 & ((v1 << 4) & v2));
+#define b64url(val)						\
+  (63 & ((  val == '_') ? 63					\
+	 : (val == '-') ? 62					\
+	 : (val >= 'a') ? val - 'a' + 26			\
+	 : (val >= 'A') ? val - 'A' +  0			\
+	 :                val - '0' + 52))
+#define b64d1(v1,v2,v3,v4) (255 & ((v1 << 2) | (v2 >> 4)));
+#define b64d2(v1,v2,v3,v4) (255 & ((v2 << 4) | (v3 >> 2)));
+#define b64d3(v1,v2,v3,v4) (255 & ((v3 << 6) |  v4));
+
 #endif /* SETUP_INI_H */
diff --git a/inilex.ll b/inilex.ll
index 81a7f24..2e767ec 100644
--- a/inilex.ll
+++ b/inilex.ll
@@ -48,36 +48,58 @@ STR	[!a-zA-Z0-9_./:\+~-]+
 
 %%
 
-[0123456789abcdef]{32,32}       {
+[0-9a-f]{32} {
     yylval = (char *) new unsigned char[16];
     memset (yylval, 0, 16);
-    for (int i = 0; i < 32; ++i)
+    int i, j;
+    unsigned char v1, v2;
+    for (i = 0, j = 0; i < 32; i += 2, ++j)
       {
-	unsigned char val = (unsigned char) yytext[i];
-	if (val > '9')
-	  val = val - 'a' + 10;
-	else
-	  val = val - '0';
-	((unsigned char *) yylval) [i / 2] += val << ((i % 2) ? 0 : 4);
+	v1 = hexnibble((unsigned char) yytext[i+0]);
+	v2 = hexnibble((unsigned char) yytext[i+1]);
+	((unsigned char *) yylval) [j] = nibbled1(v1, v2);
       }
     return MD5;
 }
 
-[0123456789abcdef]{128,128}       {
+[0-9a-f]{128} {
     yylval = (char *) new unsigned char[SHA512_DIGEST_LENGTH];
     memset (yylval, 0, SHA512_DIGEST_LENGTH);
-    for (int i = 0; i < SHA512_BLOCK_LENGTH; ++i)
+    int i, j;
+    unsigned char v1, v2;
+    for (i = 0, j = 0; i < SHA512_BLOCK_LENGTH; i += 2, ++j)
       {
-	unsigned char val = (unsigned char) yytext[i];
-	if (val > '9')
-	  val = val - 'a' + 10;
-	else
-	  val = val - '0';
-	((unsigned char *) yylval) [i / 2] += val << ((i % 2) ? 0 : 4);
+	v1 = hexnibble((unsigned char) yytext[i+0]);
+	v2 = hexnibble((unsigned char) yytext[i+1]);
+	((unsigned char *) yylval) [j] = nibbled1(v1, v2);
       }
     return SHA512;
 }
 
+[a-zA-Z0-9_-]{86} {
+    /* base64url as defined in RFC4648 */
+    yylval = (char *) new unsigned char[SHA512_DIGEST_LENGTH];
+    memset (yylval, 0, SHA512_DIGEST_LENGTH);
+    int i, j;
+    unsigned char v1, v2, v3, v4;
+    for (i = 0, j = 0; i < 4*(SHA512_DIGEST_LENGTH/3); i += 4, j += 3)
+      {
+	v1 = b64url(((unsigned char) yytext[i+0]));
+	v2 = b64url(((unsigned char) yytext[i+1]));
+	v3 = b64url(((unsigned char) yytext[i+2]));
+	v4 = b64url(((unsigned char) yytext[i+3]));
+	((unsigned char *) yylval) [j+0] = b64d1(v1, v2, v3, v4);
+	((unsigned char *) yylval) [j+1] = b64d2(v1, v2, v3, v4);
+	((unsigned char *) yylval) [j+2] = b64d3(v1, v2, v3, v4);
+      }
+    v1 = b64url((unsigned char) yytext[i+0]);
+    v2 = b64url((unsigned char) yytext[i+1]);
+    v3 = 0;
+    v4 = 0;
+    ((unsigned char *) yylval) [j+0] = b64d1(v1, v2, v3, v4);
+    return SHA512B64URL;
+}
+
 \"[^"]*\"		{ yylval = new char [strlen (yytext+1) + 1];
 			  strcpy (yylval, yytext+1);
 			  yylval[strlen (yylval)-1] = 0;
@@ -98,6 +120,7 @@ STR	[!a-zA-Z0-9_./:\+~-]+
 "Description:"		BEGIN (descriptionstate); return DESCTAG;
 "Size:"			return FILESIZE;
 "MD5sum:"		return MD5LINE;
+"SHA512:"		return SHA512LINE;
 "Installed-Size:"	return INSTALLEDSIZE;
 "Maintainer:"		BEGIN (eolstate); return MAINTAINER;
 "Architecture:"		return ARCHITECTURE;
diff --git a/iniparse.yy b/iniparse.yy
index cab84f2..0cd1c64 100644
--- a/iniparse.yy
+++ b/iniparse.yy
@@ -42,8 +42,9 @@ void add_correct_version();
 %token CATEGORY DEPENDS REQUIRES
 %token APATH PPATH INCLUDE_SETUP EXCLUDE_PACKAGE DOWNLOAD_URL
 %token T_PREV T_CURR T_TEST
-%token SHA512 MD5 INSTALLEDSIZE MAINTAINER PRIORITY
-%token DESCTAG DESCRIPTION FILESIZE ARCHITECTURE SOURCEPACKAGE MD5LINE 
+%token SHA512 SHA512B64URL MD5 INSTALLEDSIZE MAINTAINER PRIORITY
+%token MD5LINE SHA512LINE
+%token DESCTAG DESCRIPTION FILESIZE ARCHITECTURE SOURCEPACKAGE
 %token RECOMMENDS PREDEPENDS
 %token SUGGESTS CONFLICTS REPLACES PROVIDES PACKAGENAME STRTOEOL PARAGRAPH
 %token EMAIL COMMA OR NL AT
@@ -107,6 +108,8 @@ singleitem /* non-empty */
  | DIRECTORY STRING NL		{ /* TODO */ }
  | STANDARDSVERSION STRING NL	{ /* TODO */ }
  | MD5LINE MD5 NL	{ iniBuilder->buildInstallMD5 ((unsigned char *)$2); }
+ | MD5LINE SHA512 NL	{ iniBuilder->buildInstallSHA512 ((unsigned char *)$2); }
+ | MD5LINE SHA512B64URL NL	{ iniBuilder->buildInstallSHA512 ((unsigned char *)$2); }
  | SOURCEPACKAGE source NL
  | CATEGORY categories NL
  | INSTALL STRING { iniBuilder->buildPackageInstall ($2); } installmeta NL
@@ -146,11 +149,13 @@ installmeta: /* empty */
 installchksum: /* empty */
  | MD5 			{ iniBuilder->buildInstallMD5 ((unsigned char *)$1);}
  | SHA512		{ iniBuilder->buildInstallSHA512 ((unsigned char *)$1);}
+ | SHA512B64URL		{ iniBuilder->buildInstallSHA512 ((unsigned char *)$1);}
  ;
 
 sourcechksum: /* empty */
  | MD5 			{ iniBuilder->buildSourceMD5 ((unsigned char *)$1); }
  | SHA512 		{ iniBuilder->buildSourceSHA512 ((unsigned char *)$1); }
+ | SHA512B64URL		{ iniBuilder->buildSourceSHA512 ((unsigned char *)$1); }
  ;
 
 source /* non-empty */
-- 
2.3.3.GIT
--8<---------------cut here---------------end--------------->8---

And this is the change to genini so you can produce whichever digest you
want and default to SHA512-hex:

--8<---------------cut here---------------start------------->8---
Index: genini
===================================================================
RCS file: /cvs/cygwin-apps/genini/genini,v
retrieving revision 1.15
diff -u -p -r1.15 genini
--- genini      7 Oct 2013 21:11:35 -0000       1.15
+++ genini      7 Jun 2015 20:14:29 -0000
@@ -7,6 +7,7 @@
 #
 use File::Basename;
 use Digest::MD5;
+use Digest::SHA;
 use Getopt::Long;
 
 use strict;
@@ -19,10 +20,17 @@ sub arch_handler(@);
 my @okmissing = qw'message ldesc';
 my ($outfile, $help, $recursive);
 my $arch = 'x86';
+my $digest = 'sha512';
 my $release;
-my @cmp_fmts = qw(gz bz2 lzma xz);
+my @cmp_fmts = qw(xz bz2 lzma gz);
 
-GetOptions('okmissing=s'=>\@okmissing, 'output=s'=>\$outfile, 'help'=>\$help, 'release=s'=>\$release, 'arch=s'=>\&arch_handler, 'recursive'=>\$recursive) or usage;
+GetOptions('okmissing=s'=>\@okmissing,
+          'output=s'=>\$outfile,
+          'help'=>\$help,
+          'release=s'=>\$release,
+          'arch=s'=>\&arch_handler,
+          'digest=s'=>\&digest_handler,
+          'recursive'=>\$recursive) or usage;
 $help and usage;
 
 @main::okmissing{@okmissing} = @okmissing;
@@ -31,7 +39,14 @@ sub arch_handler (@) {
    my ($opt_name, $opt_value) = @_;
    die "invalid arch specified: '$opt_value'"
       unless $main::valid_arch{lc $opt_value};
-   $arch = $opt_value;
+   $arch = lc $opt_value;
+}
+
+sub digest_handler (@) {
+   my ($opt_name, $opt_value) = @_;
+   die "invalid digest specified: '$opt_value'"
+      unless $main::valid_digest{lc $opt_value};
+   $digest = lc $opt_value;
 }
 
 if (defined($outfile)) {
@@ -190,6 +205,7 @@ sub parsedir {
     my $setup_hint = "$d/setup.hint";
     return unless -e $setup_hint;
     parse("$setup_hint", $pname);
+    next unless exists $pkg{$pname};
     my $explicit = 0;
     for my $what ('', "[prev]\n", "[test]\n") {
        my $x = $pkg{$pname}{$what};
@@ -246,9 +262,20 @@ sub filer {
        myerror "can't open $f - $!" unless $main::okmissing{$what};
        return undef;
     };
-    my $md5 = Digest::MD5->new;
-    $md5->addfile(\*F);
-    $x->{$what} = join(' ', $f, -s $f, $md5->hexdigest);
+    my ( $chk, $sum );
+    if ('md5' eq $digest) {
+       $chk = Digest::MD5->new;
+    } elsif ('sha512' eq substr($digest, 0, 6)) {
+       $chk = Digest::SHA->new(512);
+    }
+    $chk->addfile(\*F);
+    if ('sha512b64url' eq $digest) {
+       $sum = $chk->b64digest;
+       $sum =~ tr:+/:-_:; # convert to base64url as defined in RFC4648
+    } else {
+       $sum = $chk->hexdigest;
+    }
+    $x->{$what} = join(' ', $f, -s $f, $sum);
 }
 
 sub tarball {
@@ -260,8 +287,8 @@ sub tarball {
         return "$f";
       }
     }
-    # default to .bz2 (even though we know it is missing)
-    return "$d/" . "$b" . "bz2";
+    # default to .nf (because we know it is missing)
+    return "$d/" . "$b" . "nf";
 }
 
 sub fnln {
@@ -312,13 +339,17 @@ EOF
 
 BEGIN {
     my @cats = qw'
-     Admin Archive Audio Base Comm Database Debug Devel Doc Editors Games
-     Gnome Graphics Interpreters KDE Libs Mail Math Mingw Net Perl
-     Publishing Python Science Shells Sound System Text Utils Web X11
-     _obsolete _PostInstallLast
+     Accessibility Admin Archive Audio Base Comm Database Devel Doc
+     Editors Games GNOME Graphics Interpreters KDE Libs LXDE Mail MATE
+     Math Mingw Net Perl PHP Python Publishing Ruby Science Shells
+     System Text Utils Video Web X11 Xfce
+     Debug _obsolete _PostInstallLast
      ';
     @main::categories{map {lc $_} @cats} = @cats;
 
     my @archs = qw'x86 x86_64';
     @main::valid_arch{map {lc $_} @archs} = @archs;
+
+    my @digests = qw'md5 sha512 sha512b64url';
+    @main::valid_digest{map {lc $_} @digests} = @digests;
 }
--8<---------------cut here---------------end--------------->8---

Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

DIY Stuff:
http://Synth.Stromeko.net/DIY.html

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Jun  3 19:11, Achim Gratz wrote:
> Yaakov Selkowitz writes:
> > Anyone looking at the sources would be tainted thereby.  Because we are
> > concerned about the licensing of upset, AFAICS we are going to have to
> > clean-room reverse-engineer this.
> 
> So what exactly does upset that genini doesn't when it comes to
> generating setup.ini and what other functions does it do in the
> maintenance of the download area (which I assume is the part where it
> gets hairy)?

Basically upset generates setup.ini and setup.bz2 files and a makefile
signs the files.  But additionally upset also creates the data for the
package search and it does the uploading from the package staging area.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

On Jun  7 22:21, Achim Gratz wrote:
> Achim Gratz writes:
> > I'll have to write a ChangeLog and clean up the code a bit before I'll
> > post the patches.
> 
> Here's the patch for setup:

I'm still not quite sure what problem this solves.  Is it faster than
plain SHA512 checksums?  Just the reduction from 128 to 86 chars doesn't
look like it's really worth the changes required in setup and upset.
What's the actual gain?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

On Jun  8 14:56, Corinna Vinschen wrote:
> On Jun  3 19:11, Achim Gratz wrote:
> > Yaakov Selkowitz writes:
> > > Anyone looking at the sources would be tainted thereby.  Because we are
> > > concerned about the licensing of upset, AFAICS we are going to have to
> > > clean-room reverse-engineer this.
> > 
> > So what exactly does upset that genini doesn't when it comes to
> > generating setup.ini and what other functions does it do in the
> > maintenance of the download area (which I assume is the part where it
> > gets hairy)?
> 
> Basically upset generates setup.ini and setup.bz2 files and a makefile
> signs the files.  But additionally upset also creates the data for the
> package search and it does the uploading from the package staging area.

...it also handles the "autodep" and "incver_ifdep" stuff in setup.hint.

"autodep" (and "noautodep") seems to have a bug, though, as we discussed
a while back.  It seems to miss adding some of the required dependencies.

"incver_ifdep" is still used in _update-info-dir unfortunately.  It would
be nice to make this a perpetual postinstall script instead.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> I'm still not quite sure what problem this solves.

Nothing earth-shattering, I just intensely dislike those long hex
strings.  But thanks for asking, I just had a closer look at the
real-world results and it turns out that there is an effect beyond
(questionable, I know) aesthetical considerations.

> Is it faster than plain SHA512 checksums?

I'd say it's a noticeable speed improvement, depending on where you get
that file from.  There are 17146 checksums in the 32bit setup.ini at the
moment.  That makes for a saving of 720132 bytes with Base64-URL vs. Hex
encoding.  I have another patch in testing that allows to use compressed
setup.ini (and xz compression) from "local" directories and since the
normal install is across the network, the reduced number of bytes to
push over the slow connection is making quite an impact.  But I was more
surprised to see that it is actually noticeably faster even when run
locally from an SSD.  Decompression in memory is still about an order of
magnitude faster than that disk read.  Now, even to the compressed files
are smaller, although that small difference is clearly not very
bothersome:

  5429375 setup32.ini
  4709243 setup32.b64url.ini (86.7%, -720132 bytes)
  
  1469297 setup32.bz2
  1440667 setup32.b64url.bz2 (98.0%,  -28630 bytes)
  
  1177528 setup32.xz
  1164856 setup32.b64url.xz  (98.9%,  -12672 bytes)

…which makes it all the more useful, I'd say.

> Just the reduction from 128 to 86 chars doesn't look like it's really
> worth the changes required in setup and upset.  What's the actual
> gain?

The changes in setup are really small.  Upset doesn't need to change, at
least not immediately.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: setup

[Yaakov Selkowitz]

On Mon, 2015-06-08 at 15:49 +0200, Corinna Vinschen wrote:
> On Jun  8 14:56, Corinna Vinschen wrote:
> > On Jun  3 19:11, Achim Gratz wrote:
> > > Yaakov Selkowitz writes:
> > > > Anyone looking at the sources would be tainted thereby.  Because we are
> > > > concerned about the licensing of upset, AFAICS we are going to have to
> > > > clean-room reverse-engineer this.
> > > 
> > > So what exactly does upset that genini doesn't when it comes to
> > > generating setup.ini and what other functions does it do in the
> > > maintenance of the download area (which I assume is the part where it
> > > gets hairy)?
> > 
> > Basically upset generates setup.ini and setup.bz2 files and a makefile
> > signs the files.  But additionally upset also creates the data for the
> > package search and it does the uploading from the package staging area.
> 
> ...it also handles the "autodep" and "incver_ifdep" stuff in setup.hint.
> 
> "autodep" (and "noautodep") seems to have a bug, though, as we discussed
> a while back.  It seems to miss adding some of the required dependencies.

Do we really need autodep anymore?

> "incver_ifdep" is still used in _update-info-dir unfortunately.  It would
> be nice to make this a perpetual postinstall script instead.

Ack.

--
Yaakov

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

On Jun  8 13:34, Yaakov Selkowitz wrote:
> On Mon, 2015-06-08 at 15:49 +0200, Corinna Vinschen wrote:
> > On Jun  8 14:56, Corinna Vinschen wrote:
> > > On Jun  3 19:11, Achim Gratz wrote:
> > > > Yaakov Selkowitz writes:
> > > > > Anyone looking at the sources would be tainted thereby.  Because we are
> > > > > concerned about the licensing of upset, AFAICS we are going to have to
> > > > > clean-room reverse-engineer this.
> > > > 
> > > > So what exactly does upset that genini doesn't when it comes to
> > > > generating setup.ini and what other functions does it do in the
> > > > maintenance of the download area (which I assume is the part where it
> > > > gets hairy)?
> > > 
> > > Basically upset generates setup.ini and setup.bz2 files and a makefile
> > > signs the files.  But additionally upset also creates the data for the
> > > package search and it does the uploading from the package staging area.
> > 
> > ...it also handles the "autodep" and "incver_ifdep" stuff in setup.hint.
> > 
> > "autodep" (and "noautodep") seems to have a bug, though, as we discussed
> > a while back.  It seems to miss adding some of the required dependencies.
> 
> Do we really need autodep anymore?

IDK.  autodep is only used to create the auto-dependency to the "cygwin"
package, except for the two "noautodep" packages _update-info-dir and
_autorebase.  The reason, afair, was to make sure the dependency chain
created by setup when (un-)installing in the right order makes sense.

If you think we don't need it anymore, please explain.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Basically upset generates setup.ini and setup.bz2 files and a makefile
> signs the files.

OK.  That task is fairly easy to isolate.

> But additionally upset also creates the data for the
> package search and it does the uploading from the package staging area.

That's a different task, but I guess I know where this is coming from
since I have arrived at a similar architecture.  It is separate enough
to be modularizable, but there's enough benefit from using the data
you've just brought into shape for the setup.ini generation that it
makes no sense to throw it all away and then generate it again for
staging the files.

> ...it also handles the "autodep" and "incver_ifdep" stuff in setup.hint.

Do we still need this?  What is it _actually_ used for at the moment?

> "autodep" (and "noautodep") seems to have a bug, though, as we discussed
> a while back.  It seems to miss adding some of the required dependencies.

I have some code to ignore dependencies.  Adding dependencies at the
same place would be a doddle, but I can't really think of a good reason
to do this.  Most if not all of the autodep stuff could be replaced by
perpetual postinstall scripts and there's still the option to actually
implement pre/post install triggers in setup if that's not enough.

> "incver_ifdep" is still used in _update-info-dir unfortunately.  It would
> be nice to make this a perpetual postinstall script instead.

Yes, that seems preferrable.  It's still on my list of things to look
at, but pretty far down.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Yaakov Selkowitz]

On Mon, 2015-06-08 at 20:47 +0200, Corinna Vinschen wrote:
> On Jun  8 13:34, Yaakov Selkowitz wrote:
> > On Mon, 2015-06-08 at 15:49 +0200, Corinna Vinschen wrote:
> > > ...it also handles the "autodep" and "incver_ifdep" stuff in setup.hint.
> > > 
> > > "autodep" (and "noautodep") seems to have a bug, though, as we discussed
> > > a while back.  It seems to miss adding some of the required dependencies.
> > 
> > Do we really need autodep anymore?
> 
> IDK.  autodep is only used to create the auto-dependency to the "cygwin"
> package, except for the two "noautodep" packages _update-info-dir and
> _autorebase.  The reason, afair, was to make sure the dependency chain
> created by setup when (un-)installing in the right order makes sense.

I thought the reason was to (attempt to) assure proper ordering of
postinstall scripts, which (besides not working properly, as we
discovered a while ago) we shouldn't need to rely on any more with the
current setup.

--
Yaakov

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On Jun  8 20:07, Achim Gratz wrote:
> Corinna Vinschen writes:
> > I'm still not quite sure what problem this solves.
> 
> Nothing earth-shattering, I just intensely dislike those long hex
> strings.  But thanks for asking, I just had a closer look at the
> real-world results and it turns out that there is an effect beyond
> (questionable, I know) aesthetical considerations.
> 
> > Is it faster than plain SHA512 checksums?
> 
> I'd say it's a noticeable speed improvement, depending on where you get
> that file from.  There are 17146 checksums in the 32bit setup.ini at the
> moment.  That makes for a saving of 720132 bytes with Base64-URL vs. Hex
> encoding.  I have another patch in testing that allows to use compressed
> setup.ini (and xz compression) from "local" directories and since the
> normal install is across the network, the reduced number of bytes to
> push over the slow connection is making quite an impact.  But I was more
> surprised to see that it is actually noticeably faster even when run
> locally from an SSD.

I'm certainly not de-valuing your patch, but that makes me suspicious.
The impact of reading from a local SSD should be rather low.  That
reminds me of the buffer size problem slowing down SHA512 evaluation,
see git diff 0c7564..8648b05.

greping setup sources, it seems setup is using smaller buffers in more
places, e.g.

compress_bz::read uses 4K buffer
compress_gz::read 16K
io_stream::copy   16K
YY_INPUT	   8K

Especially YY_READ_BUF_SIZE defined as 8K might be a culprit here.

Can you speed up reading the ini file noticably by defining
YY_READ_BUF_SIZE to 65536 in inilex.ll?  If so, is the impact of using
base64 still an issue, performance-wise?

> The changes in setup are really small.  Upset doesn't need to change, at
> least not immediately.

Ok.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Jun  8 14:05, Yaakov Selkowitz wrote:
> On Mon, 2015-06-08 at 20:47 +0200, Corinna Vinschen wrote:
> > On Jun  8 13:34, Yaakov Selkowitz wrote:
> > > On Mon, 2015-06-08 at 15:49 +0200, Corinna Vinschen wrote:
> > > > ...it also handles the "autodep" and "incver_ifdep" stuff in setup.hint.
> > > > 
> > > > "autodep" (and "noautodep") seems to have a bug, though, as we discussed
> > > > a while back.  It seems to miss adding some of the required dependencies.
> > > 
> > > Do we really need autodep anymore?
> > 
> > IDK.  autodep is only used to create the auto-dependency to the "cygwin"
> > package, except for the two "noautodep" packages _update-info-dir and
> > _autorebase.  The reason, afair, was to make sure the dependency chain
> > created by setup when (un-)installing in the right order makes sense.
> 
> I thought the reason was to (attempt to) assure proper ordering of
> postinstall scripts,

That's what I meant, just in so many different words ;)

> which (besides not working properly, as we discovered a while ago) we
> shouldn't need to rely on any more with the current setup.

That's where you lost me.  Why is it NOT necessary anymore?  Was there
some patch to setup I'm missing?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> I'm certainly not de-valuing your patch, but that makes me suspicious.
> The impact of reading from a local SSD should be rather low.  That
> reminds me of the buffer size problem slowing down SHA512 evaluation,
> see git diff 0c7564..8648b05.
>
> greping setup sources, it seems setup is using smaller buffers in more
> places, e.g.
>
> compress_bz::read uses 4K buffer
> compress_gz::read 16K

Not using any of these anymore.

> io_stream::copy   16K

I've already enlarged that to 64kiB, but I couldn't really tell the
difference.

> YY_INPUT	   8K
>
> Especially YY_READ_BUF_SIZE defined as 8K might be a culprit here.

At least from the network the slow part is the loading of the file
(while the progress bar updates).  Parsing should happen in that split
second when the loading is complete and before it switches to the
chooser pane.

> Can you speed up reading the ini file noticably by defining
> YY_READ_BUF_SIZE to 65536 in inilex.ll?  If so, is the impact of using
> base64 still an issue, performance-wise?

That's not the right place to do the override, since it ends up after
the default definition.  In any case, from the Flex manual it doesn't
make sense to increase that buffer ("it's almost always too large") and
it is being fed from an io_stream, which is buffered itself.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]

On Jun  8 22:54, Achim Gratz wrote:
> Corinna Vinschen writes:
> > I'm certainly not de-valuing your patch, but that makes me suspicious.
> > The impact of reading from a local SSD should be rather low.  That
> > reminds me of the buffer size problem slowing down SHA512 evaluation,
> > see git diff 0c7564..8648b05.
> >[...]
> > YY_INPUT	   8K
> >
> > Especially YY_READ_BUF_SIZE defined as 8K might be a culprit here.
> 
> At least from the network the slow part is the loading of the file
> (while the progress bar updates).  Parsing should happen in that split
> second when the loading is complete and before it switches to the
> chooser pane.

Right, in the networking case the download should be the major factor,
but I was referring to the SSD case.

> > Can you speed up reading the ini file noticably by defining
> > YY_READ_BUF_SIZE to 65536 in inilex.ll?  If so, is the impact of using
> > base64 still an issue, performance-wise?
> 
> That's not the right place to do the override, since it ends up after
> the default definition.

Huh?  Not for me:

diff --git a/inilex.ll b/inilex.ll
index 81a7f24..b5ea619 100644
--- a/inilex.ll
+++ b/inilex.ll
@@ -29,6 +29,7 @@
 #include "sha2.h"
 
 #define YY_INPUT(buf,result,max_size) { result = ini_getchar(buf, max_size); }
+#define YY_READ_BUF_SIZE 65536
 
 static int ini_getchar(char *buf, int max_size);
 static void ignore_line (void);

> In any case, from the Flex manual it doesn't
> make sense to increase that buffer ("it's almost always too large") and
> it is being fed from an io_stream, which is buffered itself.

Yes, but it's using the default buffersize of the underlying MSVCRT
stdio, which is using some value I don't know off the top of my head.
Typically this would be 1K or 4K.  YY_INPUT is reading in 8K chunks, so
the underlying stdio is very likely already skipping its buffering.
So changing YY_READ_BUF_SIZE to 64K *should* have an impact.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Right, in the networking case the download should be the major factor,
> but I was referring to the SSD case.

>> > Can you speed up reading the ini file noticably by defining
>> > YY_READ_BUF_SIZE to 65536 in inilex.ll?  If so, is the impact of using
>> > base64 still an issue, performance-wise?
>> 
>> That's not the right place to do the override, since it ends up after
>> the default definition.
>
> Huh?  Not for me:

I had been trying to put it before the includes and that buggered up
things.  Ain't that wonderful?

>> In any case, from the Flex manual it doesn't
>> make sense to increase that buffer ("it's almost always too large") and
>> it is being fed from an io_stream, which is buffered itself.
>
> Yes, but it's using the default buffersize of the underlying MSVCRT
> stdio, which is using some value I don't know off the top of my head.
> Typically this would be 1K or 4K.  YY_INPUT is reading in 8K chunks, so
> the underlying stdio is very likely already skipping its buffering.
> So changing YY_READ_BUF_SIZE to 64K *should* have an impact.

It still looks that the loading of the file is the culprit and not the
parsing, but it's hard to make out the difference on that system here.

In any case, setup now manages to write to SSD with about 15MiB/s on
average and about 40MiB/s tops (using the gcc-debuginfo package since
that has a good mixture of a few very large files and many small files).
I'd think that task manager should also show some reading activity (the
repo is on the same disk) sinc the compression ration is about 5:1 on
avaerage, but nothing.  The read activity may generally be too bursty
for the performance counters to notice.  Making the xz buffers a lot
larger (1MiB) didn't change both numbers too much, but unpacking the
small files became more "chunky".  That may have a benefit with either a
beefier CPU (the build system is using a 1037U Sandy Bridge Celeron) or
threading setup so that it uses the two cores available (the second core
is ATM only utilized by the system, so the load is about 70% on
average).


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

On Jun  9 18:54, Achim Gratz wrote:
> Corinna Vinschen writes:
> > Right, in the networking case the download should be the major factor,
> > but I was referring to the SSD case.
> 
> >> > Can you speed up reading the ini file noticably by defining
> >> > YY_READ_BUF_SIZE to 65536 in inilex.ll?  If so, is the impact of using
> >> > base64 still an issue, performance-wise?
> >> 
> >> That's not the right place to do the override, since it ends up after
> >> the default definition.
> >
> > Huh?  Not for me:
> 
> I had been trying to put it before the includes and that buggered up
> things.  Ain't that wonderful?
> 
> >> In any case, from the Flex manual it doesn't
> >> make sense to increase that buffer ("it's almost always too large") and
> >> it is being fed from an io_stream, which is buffered itself.
> >
> > Yes, but it's using the default buffersize of the underlying MSVCRT
> > stdio, which is using some value I don't know off the top of my head.
> > Typically this would be 1K or 4K.  YY_INPUT is reading in 8K chunks, so
> > the underlying stdio is very likely already skipping its buffering.
> > So changing YY_READ_BUF_SIZE to 64K *should* have an impact.
> 
> It still looks that the loading of the file is the culprit and not the
> parsing, but it's hard to make out the difference on that system here.
> 
> In any case, setup now manages to write to SSD with about 15MiB/s on
> average and about 40MiB/s tops (using the gcc-debuginfo package since
> that has a good mixture of a few very large files and many small files).

I'm a bit puzzled at this point.  Changing YY_READ_BUF_SIZE wouldn't
change unpacking tar files.  What am I missing?

Either way, please go ahead and apply your patch.  While you're at it,
would you mind to change YY_READ_BUF_SIZE as above and raise the buffer
size in io_stream::copy to 64K as well?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> I'm a bit puzzled at this point.  Changing YY_READ_BUF_SIZE wouldn't
> change unpacking tar files.  What am I missing?

Nothing, I just mentioned the results I got after your biffer size
change and that they hadn't been affected by anything I've done
elsewhere.

> Either way, please go ahead and apply your patch.  While you're at it,
> would you mind to change YY_READ_BUF_SIZE as above and raise the buffer
> size in io_stream::copy to 64K as well?

Yes, but I'll probably put each of those in a separate commit.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Jun 10 19:46, Achim Gratz wrote:
> Corinna Vinschen writes:
> > I'm a bit puzzled at this point.  Changing YY_READ_BUF_SIZE wouldn't
> > change unpacking tar files.  What am I missing?
> 
> Nothing, I just mentioned the results I got after your biffer size
> change and that they hadn't been affected by anything I've done
> elsewhere.
> 
> > Either way, please go ahead and apply your patch.  While you're at it,
> > would you mind to change YY_READ_BUF_SIZE as above and raise the buffer
> > size in io_stream::copy to 64K as well?
> 
> Yes, but I'll probably put each of those in a separate commit.

Sure, thank you.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> > Either way, please go ahead and apply your patch.  While you're at it,

Done.

>> > would you mind to change YY_READ_BUF_SIZE as above and raise the buffer
>> > size in io_stream::copy to 64K as well?
>>
>> Yes, but I'll probably put each of those in a separate commit.
>
> Sure, thank you.

I'll do that later since I have to re-shuffle some local commits and
then test everything again first.


If you're so inclined you could perhaps have a look at the next commit
on my local branch:

http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/5b4e8b928c7fe16b2813b5f1bdbf27b49c8e0d57

This implements xz compressed setup.xz files over network in addition to
setup.bz2.  The difference between bz2 and xz is pretty consistently
about 10% smaller files for xz, but the main envisioned advantage some
time down the road (years, most likely) would be to get rid of the
additional compression libraries in setup once everything is compressed
with xz.  The best compression setting with the setup files I have
looked at is "-6e".  I've also checked that setup can deal correctly
with different checksums for the xz blocks (standard is crc64, but
sha256 works).


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for KORG EX-800 and Poly-800MkII V0.9:
http://Synth.Stromeko.net/Downloads.html#KorgSDada

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Jun 10 22:50, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> > Either way, please go ahead and apply your patch.  While you're at it,
> 
> Done.
> 
> >> > would you mind to change YY_READ_BUF_SIZE as above and raise the buffer
> >> > size in io_stream::copy to 64K as well?
> >>
> >> Yes, but I'll probably put each of those in a separate commit.
> >
> > Sure, thank you.
> 
> I'll do that later since I have to re-shuffle some local commits and
> then test everything again first.
> 
> 
> If you're so inclined you could perhaps have a look at the next commit
> on my local branch:
> 
> http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/5b4e8b928c7fe16b2813b5f1bdbf27b49c8e0d57
> 
> This implements xz compressed setup.xz files over network in addition to
> setup.bz2.  The difference between bz2 and xz is pretty consistently
> about 10% smaller files for xz, but the main envisioned advantage some
> time down the road (years, most likely) would be to get rid of the
> additional compression libraries in setup once everything is compressed
> with xz.  The best compression setting with the setup files I have
> looked at is "-6e".  I've also checked that setup can deal correctly
> with different checksums for the xz blocks (standard is crc64, but
> sha256 works).

Ok, I took a look.  Basically it looks fine, but it seems there's a bug
in terms of sig_fail.

You're giving sig_fail as argument to fetch_remote_ini.  Inside
fetch_remote_ini, you're setting sig_fail, but you never test it.
However, since sig_fail has been given to fetch_remote_ini by value,
not by reference, the caller will never see the change to sig_fail.
So in the expression

  if (!ini_file && !sig_fail)

sig_fail is always false.

I'm also wondering of the new logic at this point.  If you fix the above
call by value of sig_fail, the new logic will only continue if there
was no sig test fail.  It will not continue if there was no ini file,
as it did before.  Why?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]

On Jun 11 12:06, Corinna Vinschen wrote:
> On Jun 10 22:50, Achim Gratz wrote:
> > Corinna Vinschen writes:
> > >> > Either way, please go ahead and apply your patch.  While you're at it,
> > 
> > Done.
> > 
> > >> > would you mind to change YY_READ_BUF_SIZE as above and raise the buffer
> > >> > size in io_stream::copy to 64K as well?
> > >>
> > >> Yes, but I'll probably put each of those in a separate commit.
> > >
> > > Sure, thank you.
> > 
> > I'll do that later since I have to re-shuffle some local commits and
> > then test everything again first.
> > 
> > 
> > If you're so inclined you could perhaps have a look at the next commit
> > on my local branch:
> > 
> > http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/5b4e8b928c7fe16b2813b5f1bdbf27b49c8e0d57
> > 
> > This implements xz compressed setup.xz files over network in addition to
> > setup.bz2.  The difference between bz2 and xz is pretty consistently
> > about 10% smaller files for xz, but the main envisioned advantage some
> > time down the road (years, most likely) would be to get rid of the
> > additional compression libraries in setup once everything is compressed
> > with xz.  The best compression setting with the setup files I have
> > looked at is "-6e".  I've also checked that setup can deal correctly
> > with different checksums for the xz blocks (standard is crc64, but
> > sha256 works).
> 
> Ok, I took a look.  Basically it looks fine, but it seems there's a bug
> in terms of sig_fail.
> 
> You're giving sig_fail as argument to fetch_remote_ini.  Inside
> fetch_remote_ini, you're setting sig_fail, but you never test it.
> However, since sig_fail has been given to fetch_remote_ini by value,
> not by reference, the caller will never see the change to sig_fail.
> So in the expression
> 
>   if (!ini_file && !sig_fail)
> 
> sig_fail is always false.
> 
> I'm also wondering of the new logic at this point.  If you fix the above
> call by value of sig_fail, the new logic will only continue if there
> was no sig test fail.  It will not continue if there was no ini file,
                                              ^^^^^^^^^^^^^^^^^^^^^^^^
                                              Doh.  Make that
                                              "if there was a signature
                                               test fail"

> as it did before.  Why?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Ok, I took a look.  Basically it looks fine, but it seems there's a bug
> in terms of sig_fail.

Oh yes, that's buggered up.

> You're giving sig_fail as argument to fetch_remote_ini.  Inside
> fetch_remote_ini, you're setting sig_fail, but you never test it.
> However, since sig_fail has been given to fetch_remote_ini by value,
> not by reference, the caller will never see the change to sig_fail.

Yes, that should have been a bool&.

> So in the expression
>
>   if (!ini_file && !sig_fail)
>
> sig_fail is always false.
>
> I'm also wondering of the new logic at this point.  If you fix the above
> call by value of sig_fail, the new logic will only continue if there
> was no sig test fail.  It will not continue if there was no ini file,
> as it did before.  Why?

When I pulled the two conditions into one, I just didn't get the logic
right.  I'll rework that later.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Achim Gratz]

Achim Gratz writes:
> I'll do that later since I have to re-shuffle some local commits and
> then test everything again first.

Done.

> If you're so inclined you could perhaps have a look at the next commit
> on my local branch:

The re-worked version including local compressed setup.ini files is
available at:
http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/d289e1824d44b0e75115e2207770335b905183e0

I haven't tested signatures since I am not yet signing my own
installations.  The chooser window now comes up almost instantaneously
in chooser mode from "known good" local mirror even when getting the
files over slow or congested network.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

Hi Achim,

On Jun 11 23:03, Achim Gratz wrote:
> Achim Gratz writes:
> > I'll do that later since I have to re-shuffle some local commits and
> > then test everything again first.
> 
> Done.
> 
> > If you're so inclined you could perhaps have a look at the next commit
> > on my local branch:
> 
> The re-worked version including local compressed setup.ini files is
> available at:
> http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/d289e1824d44b0e75115e2207770335b905183e0
> 
> I haven't tested signatures since I am not yet signing my own
> installations.  The chooser window now comes up almost instantaneously
> in chooser mode from "known good" local mirror even when getting the
> files over slow or congested network.

"known good" == -m?

Thank you for getting rid of IniParseFindVisitor :)

However, IniParseFindVisitor searched the tree for ini files, so, does
the new code still work for the standard local install with ini files
copied from various mirrors, w/o -m option?  It seems the answer is yes
(you're iterating over the site list), but it would be helpful if this
had been tested.

The patch is a bit big and mixes several things into one.  It would
be helpful for potential later blaming to split it into independent
chunks to commit separately.  E.g., in at least one instance you just
reordered a few lines without functional change.  This may better go
in as a separate style patch.

Otherwise it looks ok, but I'd be more comfortable to see the splitted
patches before ok'ing it.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> "known good" == -m?

Yes.

> Thank you for getting rid of IniParseFindVisitor :)

I wasn't all that sure about this, but it simply made no sense to me for
the task at hand.

> However, IniParseFindVisitor searched the tree for ini files, so, does
> the new code still work for the standard local install with ini files
> copied from various mirrors, w/o -m option?  It seems the answer is yes
> (you're iterating over the site list), but it would be helpful if this
> had been tested.

What's the expected directory structure here?  The FindVisitor searched
the tree (two levels deep), but it rejected almost all files anyway.  I
might have to go back to the original code to see what exactly it would
accept, but an almost exact copy of it is still in fromcwd (we might
want to get rid of that too at one point).  From just reading that
pointer orgy it would appear that if dir doesn't end in a slash or
backslash and doesn't end in SETUP_INI_DIR (x86 or x86_64), then it's
not recognized anyway.  That's the long winded way of saying that it
should look for a setup file either in the current directory or one
level down, I guess.

I think as it stands at the moment the search one level down from the
current directory is actually cut, since it was done again in the
IniParseFindVistor that I've removed.  I think this should be added into
fromcwd to build a list that then gets iterated over in do_local_ini
just like the sitelist in the remote case.

> The patch is a bit big and mixes several things into one.  It would
> be helpful for potential later blaming to split it into independent
> chunks to commit separately.  E.g., in at least one instance you just
> reordered a few lines without functional change.  This may better go
> in as a separate style patch.

I'll see how to split it once I have thought about the search again.  It
ended up that way since I didn't find an easy way to keep the separate
parts both compiling and working, but since it's now in a more stable
form I can revisit that question again.

> Otherwise it looks ok, but I'd be more comfortable to see the splitted
> patches before ok'ing it.

No need for hurrying.  I'm on the opposite side of the planet for a
week, so I'll take another look when I'm back.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Jun 12 19:05, Achim Gratz wrote:
> Corinna Vinschen writes:
> > "known good" == -m?
> 
> Yes.
> 
> > Thank you for getting rid of IniParseFindVisitor :)
> 
> I wasn't all that sure about this, but it simply made no sense to me for
> the task at hand.
> 
> > However, IniParseFindVisitor searched the tree for ini files, so, does
> > the new code still work for the standard local install with ini files
> > copied from various mirrors, w/o -m option?  It seems the answer is yes
> > (you're iterating over the site list), but it would be helpful if this
> > had been tested.
> 
> What's the expected directory structure here?

In the usual case ("Install from Internet") the local package directory
contains a separate subdir for each mirror chosen from the mirror dialog:

 Local package directory
 |
 +-> Mirror1 subdir
 |   |
 |   +-> x86
 |   |    |
 |   |    +-> setup.ini
 |   |
 |   +-> x86_64
 |        |
 |        +-> setup.ini
 |
 +-> Mirror2 subdir
 |   |
 |   +-> x86
 |   |   |
 |   |    +-> setup.ini
 |   |
 |   +-> x86_64
 |        |
 |        +-> setup.ini
 .
 .
 .

> > The patch is a bit big and mixes several things into one.  It would
> > be helpful for potential later blaming to split it into independent
> > chunks to commit separately.  E.g., in at least one instance you just
> > reordered a few lines without functional change.  This may better go
> > in as a separate style patch.
> 
> I'll see how to split it once I have thought about the search again.  It
> ended up that way since I didn't find an easy way to keep the separate
> parts both compiling and working, but since it's now in a more stable
> form I can revisit that question again.
> 
> > Otherwise it looks ok, but I'd be more comfortable to see the splitted
> > patches before ok'ing it.
> 
> No need for hurrying.  I'm on the opposite side of the planet for a
> week, so I'll take another look when I'm back.

Thanks a lot,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> What's the expected directory structure here?
>
> In the usual case ("Install from Internet") the local package directory
> contains a separate subdir for each mirror chosen from the mirror dialog:
>
>  Local package directory
>  |
>  +-> Mirror1 subdir
>  |   |
>  |   +-> x86
>  |   |    |
>  |   |    +-> setup.ini
>  |   |
>  |   +-> x86_64
>  |        |
>  |        +-> setup.ini
>  |
>  +-> Mirror2 subdir
>  |   |
>  |   +-> x86
>  |   |   |
>  |   |    +-> setup.ini
>  |   |
>  |   +-> x86_64
>  |        |
>  |        +-> setup.ini
>  .
>  .
>  .

I've looked at what the original code was doing (or what I think it was
doing).  Besides the case that you show, it would also find a setup.ini
if the local directory was set one or two levels down (into the mirror
level or the architectire subtree level, respectively).  In the latter
case, the local directory was required to have its last component the
correct architecture.  I think I have cooked up a solution to do this
(not yet fully implemented nor tested), but the code would be cleaner if
the local package directory would never be at the same level where
setup.ini resides (which necessitates to extract the last path component
and check for the correct architecture).

So the question is if that was maybe an accidental feature that can be
removed?  The code would become a bit cleaner.  If anybody was using it
inadvertently, they just need to remove that last component form the
local package directory, so there's no loss of functionality that I can
see.

There's another accidental feature (I think) in that old code in that
you could mix two package sources at different levels like this:

>  Local package directory (ending in /x86)
>  |
>  +-> Mirror1 subdir
>  |   |
>  |   +-> x86
>  |   |    |
>  |   |    +-> setup.ini
>  |   |
>  |   +-> x86_64
>  |        |
>  |        +-> setup.ini
>  +-> x86
>  |    |
>  |    +-> setup.ini
>  |
>  +-> setup.ini

and setup-x86.exe would find three setup.ini.  I think that's a mistake
and I would want to truncate the search at the first level a setup.ini
has been found (in this example only the one in the local package
directory).


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 5021 bytes --]

On Jun 24 22:00, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> What's the expected directory structure here?
> >
> > In the usual case ("Install from Internet") the local package directory
> > contains a separate subdir for each mirror chosen from the mirror dialog:
> >
> >  Local package directory
> >  |
> >  +-> Mirror1 subdir
> >  |   |
> >  |   +-> x86
> >  |   |    |
> >  |   |    +-> setup.ini
> >  |   |
> >  |   +-> x86_64
> >  |        |
> >  |        +-> setup.ini
> >  |
> >  +-> Mirror2 subdir
> >  |   |
> >  |   +-> x86
> >  |   |   |
> >  |   |    +-> setup.ini
> >  |   |
> >  |   +-> x86_64
> >  |        |
> >  |        +-> setup.ini
> >  .
> >  .
> >  .
> 
> I've looked at what the original code was doing (or what I think it was
> doing).  Besides the case that you show, it would also find a setup.ini
> if the local directory was set one or two levels down (into the mirror
> level or the architectire subtree level, respectively).

The problem is that the existing algorithm is rather dump.  The
recursive search class is, IMHO, C++ at its worst.  In fact, the
original code (you should be able to look this up in the git history if
I didn't screw this up during the cvs->git move) didn't stop at the 2nd
level but searched the whole tree below the local package dir.  I added
the "search only two levels" at one point as a band-aid since I wasn't
sure if ripping out the class wouldn't break anything.

Having said that, any more intelligent algorithm searching only where it
actually makes sense, would be real progress.  More C than C++ wouldn't
hurt either.

> In the latter
> case, the local directory was required to have its last component the
> correct architecture.  I think I have cooked up a solution to do this
> (not yet fully implemented nor tested), but the code would be cleaner if
> the local package directory would never be at the same level where
> setup.ini resides (which necessitates to extract the last path component
> and check for the correct architecture).

I'm not sure I understand what you're saying.  Setup.ini should never be
at the local package directory level is my understanding.  Local package
directory is either a local mirror, in which case you'd have to expect
this structure:

  Local MIRROR directory
  |
  +-> x86
  |    |
  |    +-> setup.ini
  |
  +-> x86_64
  |    |
  |    +-> setup.ini
  |
  +-> noarch (just would be nice at one point)

or it's the normal user case of a local package directory containing
(partial) mirrors of Cygwin mirrors as outlined in my original ASCII
art.  AFAICS we can expect these two and don't have to care for anything
else.

> So the question is if that was maybe an accidental feature that can be
> removed?  The code would become a bit cleaner.  If anybody was using it
> inadvertently, they just need to remove that last component form the
> local package directory, so there's no loss of functionality that I can
> see.
> 
> There's another accidental feature (I think) in that old code in that
> you could mix two package sources at different levels like this:
> 
> >  Local package directory (ending in /x86)
                             ^^^^^^^^^^^^^^^^
			     This is a no-no

> >  |
> >  +-> Mirror1 subdir
> >  |   |
> >  |   +-> x86
> >  |   |    |
> >  |   |    +-> setup.ini
              ^^^^^^^^^^^^^
	      Downloading from mirrors

> >  |   |
> >  |   +-> x86_64
> >  |        |
> >  |        +-> setup.ini
> >  +-> x86
> >  |    |
> >  |    +-> setup.ini
          ^^^^^^^^^^^^^
	  Local package directory is a mirror itself

> >  |
> >  +-> setup.ini
         ^^^^^^^^^
	 Ignore this

> and setup-x86.exe would find three setup.ini.  I think that's a mistake

It was not a mistake a couple of years ago.  Keep in mind that we
originally only had one platform and no x86 and x86_64 subdirs.
Therefore, finding the top-level setup.ini (the bottom one in your ASCII
art) was required when we moved to a multi-arch approach.

However.

- It would only have been correct for x86, never for x86_64.

- It's absolutely outdated these days.

> and I would want to truncate the search at the first level a setup.ini
> has been found (in this example only the one in the local package
> directory).

Strictly, we don't have to support updates from the old non-multi-arch
setup anymore, therefore any top-level setup.ini can be ignored.

So, from my POV, the search should ideally work like this:

- Start at Local package directory.

- Look if a $arch subdir exists.  If so, does it have setup.ini?
  If so collect it and stop here.

- Otherwise search all immediate subdirs.  Look if they have a
  $arch subdir.  If so, does it have setup.ini?  If so, collect it.

Does that make sense?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Having said that, any more intelligent algorithm searching only where it
> actually makes sense, would be real progress.  More C than C++ wouldn't
> hurt either.

The pattern is just misapplied in this particular case, trying to take a
gatherer on a hunt.

> I'm not sure I understand what you're saying.  Setup.ini should never be
> at the local package directory level is my understanding.

Great, then I can just skip that wierdness of the old code.

> Strictly, we don't have to support updates from the old non-multi-arch
> setup anymore, therefore any top-level setup.ini can be ignored.
>
> So, from my POV, the search should ideally work like this:
>
> - Start at Local package directory.
>
> - Look if a $arch subdir exists.  If so, does it have setup.ini?
>   If so collect it and stop here.
>
> - Otherwise search all immediate subdirs.  Look if they have a
>   $arch subdir.  If so, does it have setup.ini?  If so, collect it.
>
> Does that make sense?

Yes and I just have to rip out the compat code that dealt with the other
cases.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Achim Gratz]

Achim Gratz writes:
> Yes and I just have to rip out the compat code that dealt with the other
> cases.

The file search works now after fixing a thinko, I'll yet have to
actually relay the list of found setup.ini to the later stages of the
installation.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Jun 25 22:16, Achim Gratz wrote:
> Achim Gratz writes:
> > Yes and I just have to rip out the compat code that dealt with the other
> > cases.
> 
> The file search works now after fixing a thinko, I'll yet have to
> actually relay the list of found setup.ini to the later stages of the
> installation.

No hurry.  I'm very happy that you're working on this stuff :)


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> No hurry.  I'm very happy that you're working on this stuff :)

Hurry or not, I'll have to implement it before the MTTF(*) expires.
:-P


(*) Mean Time To Forget


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Jun 26 20:07, Achim Gratz wrote:
> Corinna Vinschen writes:
> > No hurry.  I'm very happy that you're working on this stuff :)
> 
> Hurry or not, I'll have to implement it before the MTTF(*) expires.
> :-P
> 
> 
> (*) Mean Time To Forget

Haha


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Achim Gratz writes:
>> Otherwise it looks ok, but I'd be more comfortable to see the splitted
>> patches before ok'ing it.

It would be a lot of work to split it into separate commits that both
compile and run OK, so I'm hoping the patch is acceptable as is:

http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/df7b8c2aeb52d27abb2bf8d02c1fb702ee751dd2

The directory structure for the local mirror is now strictly enforced
and you can't mix single-mirror and multi-mirror directories anymore.
Signatures will be checked in local mirrors as well unless you invoke
with the -X/--no-verify option.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf Blofeld V1.15B11:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Jun 28 15:40, Achim Gratz wrote:
> Achim Gratz writes:
> >> Otherwise it looks ok, but I'd be more comfortable to see the splitted
> >> patches before ok'ing it.
> 
> It would be a lot of work to split it into separate commits that both
> compile and run OK, so I'm hoping the patch is acceptable as is:

With `git gui' you can easily split a patch in multiple parts by using
the ability to commit single patch hunks.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> With `git gui' you can easily split a patch in multiple parts by using
> the ability to commit single patch hunks.

I can do this just fine with Emacs and magit.  Again, the problem really
is that the resulting individual commits would either not compile or not
work correctly unless I would write and test yet more code to make it
so.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

On Jun 30 06:59, Achim Gratz wrote:
> Corinna Vinschen writes:
> > With `git gui' you can easily split a patch in multiple parts by using
> > the ability to commit single patch hunks.
> 
> I can do this just fine with Emacs and magit.  Again, the problem really
> is that the resulting individual commits would either not compile or not
> work correctly unless I would write and test yet more code to make it
> so.

Ok, for once.  But please make sure that you split the commit into
functional chunks next time it's so large.  And send it to this list, so
code snippets can be referenced in the review.

A few more nits, mostly on style:

- What I'm missing are code comments in do_local_ini and do_remote_ini
  describing what the new methods do.  Yes, the old code didn't have a
  lot of comments either, but it would be nice to have this better
  explained for future reference.

- Do you plan to keep the DEBUG_FROMCWD bracketed code in there?  If so,
  it should probably just use `#if DEBUG' as elsewhere.

- The call to yydebug=1 is almost invisible now.  Please revert that
  to the old 

    [empty line]
    /*yydebug = 1; */
    [empty line]

- ini_decompress as a function name may be a bit misleading.  What
  about decompress_ini_file instead?

Otherwise it looks ok, especially splitting the code into the new
functions ini_decompress/decompress_ini_file and check_ini_sig and
removing IniParseFindVisitor is a blessing.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Ok, for once.  But please make sure that you split the commit into
> functional chunks next time it's so large.

Well, the original patch was a lot smaller and I didn't really expect
that I'd have to replace such a large chunk of the old code to make
things work correctly.  I've thought about it some more and I guess I
can split off the search implementation in fromcwd.  The meat of the
patch would still be quite large, though.

> And send it to this list, so
> code snippets can be referenced in the review.

Sure.

> A few more nits, mostly on style:
>
> - What I'm missing are code comments in do_local_ini and do_remote_ini
>   describing what the new methods do.  Yes, the old code didn't have a
>   lot of comments either, but it would be nice to have this better
>   explained for future reference.

I'll add some.

> - Do you plan to keep the DEBUG_FROMCWD bracketed code in there?  If so,
>   it should probably just use `#if DEBUG' as elsewhere.

I was using similar code from crypto.cc as a guide.  I can remove that
code as it's debugged now.  In the long run it'd be better to have
Log(DEBUG_...) calls that get optimized out when doing the final build
indtead of conditional compilation.

> - The call to yydebug=1 is almost invisible now.  Please revert that
>   to the old 
>
>     [empty line]
>     /*yydebug = 1; */
>     [empty line]

OK.

> - ini_decompress as a function name may be a bit misleading.  What
>   about decompress_ini_file instead?

OK.

> Otherwise it looks ok, especially splitting the code into the new
> functions ini_decompress/decompress_ini_file and check_ini_sig and
> removing IniParseFindVisitor is a blessing.

I'd have really liked to refactor local and remote as well as they are
almost exact copies of each other.  I didn't try if file:// URLs would
have worked, maybe they do.  If so, the search part for the remote init
would need to be lifted so that both code paths would just process a
list of URLs.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2517 bytes --]

On Jun 30 19:14, Achim Gratz wrote:
> Corinna Vinschen writes:
> > Ok, for once.  But please make sure that you split the commit into
> > functional chunks next time it's so large.
> 
> Well, the original patch was a lot smaller and I didn't really expect
> that I'd have to replace such a large chunk of the old code to make
> things work correctly.  I've thought about it some more and I guess I
> can split off the search implementation in fromcwd.  The meat of the
> patch would still be quite large, though.
> 
> > And send it to this list, so
> > code snippets can be referenced in the review.
> 
> Sure.
> 
> > A few more nits, mostly on style:
> >
> > - What I'm missing are code comments in do_local_ini and do_remote_ini
> >   describing what the new methods do.  Yes, the old code didn't have a
> >   lot of comments either, but it would be nice to have this better
> >   explained for future reference.
> 
> I'll add some.

Thanks.

> > - Do you plan to keep the DEBUG_FROMCWD bracketed code in there?  If so,
> >   it should probably just use `#if DEBUG' as elsewhere.
> 
> I was using similar code from crypto.cc as a guide.  I can remove that
> code as it's debugged now.

If you're *really* sure we don't need the debug statements anymore,
then, yes, remove them.

> In the long run it'd be better to have
> Log(DEBUG_...) calls that get optimized out when doing the final build
> indtead of conditional compilation.

ACK

> > - The call to yydebug=1 is almost invisible now.  Please revert that
> >   to the old 
> >
> >     [empty line]
> >     /*yydebug = 1; */
> >     [empty line]
> 
> OK.
> 
> > - ini_decompress as a function name may be a bit misleading.  What
> >   about decompress_ini_file instead?
> 
> OK.
> 
> > Otherwise it looks ok, especially splitting the code into the new
> > functions ini_decompress/decompress_ini_file and check_ini_sig and
> > removing IniParseFindVisitor is a blessing.
> 
> I'd have really liked to refactor local and remote as well as they are
> almost exact copies of each other.  I didn't try if file:// URLs would
> have worked, maybe they do.  If so, the search part for the remote init
> would need to be lifted so that both code paths would just process a
> list of URLs.

Sounds good to me for some later patch.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

Corinna Vinschen writes:
> Ok, for once.  But please make sure that you split the commit into
> functional chunks next time it's so large.  And send it to this list, so
> code snippets can be referenced in the review.

I've split it up into three parts that at least compile cleanly.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Implement-option-i-ini-basename-to-set-the-basename-.patch --]
[-- Type: text/x-patch, Size: 3461 bytes --]

From eed5e869d0d7c42c8759f03abe3687b616b89d29 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Sun, 28 Jun 2015 13:34:11 +0200
Subject: [PATCH 1/5] Implement option "-i/--ini-basename" to set the basename
 for setup

	* ini.h: Declare extern SetupBaseName, SetupArch and SetupIniDir.
	Redefine macros to use them.

	* main.cc: Provide option "-i/--ini-basename" to set the basename
	for setup, keep the default as "setup" and store in SetupBaseName.
	Initialize SetupArch and SetupIniDir based on effective
	architecture.
---
 ChangeLog | 10 ++++++++++
 ini.h     | 17 +++++++++++++----
 main.cc   |  8 ++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1c50bfc..cb49b7e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
+
+	* ini.h: Declare extern SetupBaseName, SetupArch and SetupIniDir.
+	Redefine macros to use them.
+
+	* main.cc: Provide option "-i/--ini-basename" to set the basename
+	for setup, keep the default as "setup" and store in SetupBaseName.
+	Initialize SetupArch and SetupIniDir based on effective
+	architecture.
+
 2015-06-11  Achim Gratz  <Stromeko@NexGo.DE>
 
 	* inilex.ll: Increase read buffer size for the lexer input buffer
diff --git a/ini.h b/ini.h
index ec09def..c2903ef 100644
--- a/ini.h
+++ b/ini.h
@@ -18,6 +18,16 @@
 
 class io_stream;
 #include <string>
+#include <vector>
+
+typedef std::vector <std::string> IniList;
+extern IniList found_ini_list, setup_ext_list;
+const std::string setup_exts[] = { "xz", "bz2", "ini" };
+extern bool is_64bit;
+extern std::string SetupArch;
+extern std::string SetupIniDir;
+extern std::string SetupBaseName;
+
 class IniState;
 class IniDBBuilder;
 class IniParseFeedback;
@@ -38,10 +48,9 @@ typedef enum
   EXCLUDE_NOT_FOUND
 } excludes;
 
-extern bool is_64bit;
-#define SETUP_INI_DIR	   (is_64bit ? "x86_64/" : "x86/")
-#define SETUP_INI_FILENAME "setup.ini"
-#define SETUP_BZ2_FILENAME "setup.bz2"
+#define SETUP_INI_DIR	   SetupIniDir.c_str()
+#define SETUP_INI_FILENAME (SetupBaseName+".ini").c_str()
+#define SETUP_BZ2_FILENAME (SetupBaseName+".bz2").c_str()
 
 /* The following three vars are used to facilitate error handling between the
    parser/lexer and its callers, namely ini.cc:do_remote_ini() and
diff --git a/main.cc b/main.cc
index e968d93..46b0618 100644
--- a/main.cc
+++ b/main.cc
@@ -86,6 +86,8 @@ extern char **_argv;
 #endif
 
 bool is_64bit;
+std::string SetupArch;
+std::string SetupIniDir;
 
 using namespace std;
 
@@ -97,6 +99,8 @@ static BoolOption PackageManagerOption (false, 'M', "package-manager", "Semi-att
 static BoolOption NoAdminOption (false, 'B', "no-admin", "Do not check for and enforce running as Administrator");
 static BoolOption WaitOption (false, 'W', "wait", "When elevating, wait for elevated child process");
 static BoolOption HelpOption (false, 'h', "help", "print help");
+static StringOption SetupBaseNameOpt ("setup", 'i', "ini-basename", "Use a different basename instead of setup", false);
+std::string SetupBaseName;
 
 static void inline
 set_cout ()
@@ -262,6 +266,10 @@ WinMain (HINSTANCE h,
     if (unattended_mode || help_option)
       set_cout ();
 
+    SetupBaseName = SetupBaseNameOpt;
+    SetupArch = is_64bit ? "x86_64" : "x86";
+    SetupIniDir = SetupArch+"/";
+
     /* Get System info */
     OSVERSIONINFO version;
     version.dwOSVersionInfoSize = sizeof version;
-- 
2.4.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Correct-local-directory-search.patch --]
[-- Type: text/x-patch, Size: 7928 bytes --]

From 1650bc6ce7c4804ea35e539a808c7d708b131e87 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Wed, 1 Jul 2015 21:36:12 +0200
Subject: [PATCH 2/5] Correct local directory search

	* fromcwd.cc: Remove unused includes.  Add global found_ini_list
	to record the search result.
	(SetupFindVisitor): Make setup.{ini,bz2,xz} known and provide bool
	private variables to record whether we found them.  Another bool
	inidir to indicate whether we are currently inside a directory
	where setup.ini files should exist.
	(SetupFindVisitor::visitFile): When inidir is true, check if a
	setup file with one of the known extensions was found and set the
	corresponding bool variables.
        (SetupFindVisitor::visitDirectory): Set inidir when appropriate.
	Recurse into directories only if they are potential mirror dirs,
	based on level. Truncate search and recurse into inidir.  Record
	any setup files in found_ini_list while preferring ".xz" over
	".bz2" over ".ini" extension.
	(SetupFindVisitor::operator bool): Return true when found_ini_list
	is non-empty.
	(do_from_local_dir): Restrict search to either a single mirror
	hierarchy or multiple mirror hierarchy; setup files directly in
	local directory or mixed hierarchies are no longer recognized.
	The setup files must be present in an architecture dependent
	directory of "x86/" or "x86_64/", either in the local directory
	for single mirror or one level down for multiple mirrors.

This patch already finds setup.xz files, but the code to deal with
them will be in a later patch.  The current code re-does the search
and ignores the results from here.
---
 ChangeLog  |  25 +++++++++++++
 fromcwd.cc | 125 ++++++++++++++++++++++++++++++++++++++++---------------------
 2 files changed, 107 insertions(+), 43 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cb49b7e..e21e2e6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,30 @@
 2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* fromcwd.cc: Remove unused includes.  Add global found_ini_list
+	to record the search result.
+	(SetupFindVisitor): Make setup.{ini,bz2,xz} known and provide bool
+	private variables to record whether we found them.  Another bool
+	inidir to indicate whether we are currently inside a drectory
+	where setup.ini files should exist.
+	(SetupFindVisitor::visitFile): When inidir is true, check if a
+	setup file with one of the known extensions was found and set the
+	corresponding bool variables.
+        (SetupFindVisitor::visitDirectory): Set inidir when appropriate.
+	Recurse into directories only if they are potential mirror dirs,
+	based on level. Truncate search and recurse into inidir.  Record
+	any setup files in found_ini_list while preferring ".xz" over
+	".bz2" over ".ini" extension.
+	(SetupFindVisitor::operator bool): Return true when found_ini_list
+	is non-empty.
+	(do_from_local_dir): Restrict search to either a single mirror
+	hierarchy or multiple mirror hierarchy; setup files directly in
+	local directory or mixed hierarchies are no longer recognized.
+	The setup files must be present in an architecture dependent
+	directory of "x86/" or "x86_64/", either in the local directory
+	for single mirror or one level down for multiple mirrors.
+
+2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* ini.h: Declare extern SetupBaseName, SetupArch and SetupIniDir.
 	Redefine macros to use them.
 
diff --git a/fromcwd.cc b/fromcwd.cc
index 87da0ad..0a77474 100644
--- a/fromcwd.cc
+++ b/fromcwd.cc
@@ -25,74 +25,113 @@
    (otherwise, why would you have asked to install it?).  Note
    that we search recursively. */
 
-#if 0
-static const char *cvsid =
-  "\n%%% $Id$\n";
-#endif
-
-#include "win32.h"
 
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-
-#include "resource.h"
-#include "state.h"
-#include "dialog.h"
-#include "msg.h"
+#include "String++.h"
 #include "find.h"
-#include "ScanFindVisitor.h"
-#include "filemanip.h"
 #include "ini.h"
 
+#include "ScanFindVisitor.h"
 #include "IniDBBuilderPackage.h"
 #include "IniParseFeedback.h"
 
+#undef DEBUG_FROMCWD
+#ifdef DEBUG_FROMCWD
+#include "LogFile.h"
+#endif
+
 /* Trivial class for detecting the existence of setup.ini */
 
 class SetupFindVisitor : public FindVisitor
 {
 public:
-  SetupFindVisitor (): found(false){}
+  SetupFindVisitor ()
+    : inidir(false), found_xz(false), found_bz2(false), found_ini(false)
+    {}
   virtual void visitFile(const std::string& basePath,
                          const WIN32_FIND_DATA *theFile)
-    {
-      if (!casecompare (SETUP_INI_FILENAME, theFile->cFileName) && 
-	  (theFile->nFileSizeLow || theFile->nFileSizeHigh))
-	{
-	  /* Check if base dir ends in SETUP_INI_DIR. */
-	  const char *dir = basePath.c_str() + basePath.size ()
-			    - strlen (SETUP_INI_DIR);
-	  if (dir < basePath.c_str ())
-	    return;
-	  if ((dir != basePath.c_str () && dir[-1] != '/' && dir[-1] != '\\')
-	      || casecompare (SETUP_INI_DIR, dir))
-	    return;
-	  found = true;
-	}
-    }
+  {
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "examining file: "
+		    << basePath << "./" << theFile->cFileName << endLog;
+#endif
+    if (inidir &&
+	(theFile->nFileSizeLow || theFile->nFileSizeHigh))
+      {
+	if (!casecompare (SetupBaseName + ".xz",  theFile->cFileName))
+	  found_xz  = true;
+	if (!casecompare (SetupBaseName + ".bz2", theFile->cFileName))
+	  found_bz2 = true;
+	if (!casecompare (SetupBaseName + ".ini", theFile->cFileName))
+	  found_ini = true;
+      }
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "state: "
+		    << " inidir="    << inidir
+		    << " found_xz="  << found_xz
+		    << " found_bz2=" << found_bz2
+		    << " found_ini=" << found_ini
+		    << endLog;
+#endif
+  }
+  virtual void visitDirectory(const std::string& basePath,
+			      WIN32_FIND_DATA const *aDir, int level)
+  {
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "examining directory: "
+		    << basePath << "./" << aDir->cFileName
+		    << endLog;
+#endif
+    if ( level <= 0 )
+      return;
+    inidir = !casecompare (SetupArch, aDir->cFileName);
+    if	(level == 1 && !inidir)
+      return;
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "  recurse into: "
+		    << (inidir ? "inidir" : "mirror") << endLog;
+#endif
+    Find aFinder (basePath + aDir->cFileName);
+    aFinder.accept (*this, inidir ? 0 : --level);
+    if (found_xz)
+      found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" + SetupBaseName + ".xz");
+    else if (found_bz2)
+      found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" + SetupBaseName + ".bz2");
+    else if (found_ini)
+      found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" + SetupBaseName + ".ini");
+    inidir = found_xz = found_bz2 = found_ini = false;
+  }
   virtual ~ SetupFindVisitor (){}
-  operator bool () const {return found;}
+  operator bool () const
+  {
+    return !found_ini_list.empty();
+  }
 protected:
   SetupFindVisitor (SetupFindVisitor const &);
   SetupFindVisitor & operator= (SetupFindVisitor const &);
 private:
-  bool found;
+  bool inidir, found_xz, found_bz2, found_ini;
 };
-  
+
+IniList found_ini_list;
+
 bool
 do_from_local_dir (HINSTANCE h, HWND owner, std::string &local_dir)
 {
+#ifdef DEBUG_FROMCWD
+  Log (LOG_PLAIN) << "do_from_local_dir: "
+		  << local_dir << endLog;
+#endif
   // Assume we won't find the INI file.
-  SetupFindVisitor found_ini;
-  // Only search two levels deep.
-  Find (local_dir.c_str ()).accept(found_ini, 2);
-  if (found_ini)
-    {
-      // Found INI, load it.
+  SetupFindVisitor found;
+  // single mirror?
+  Find (local_dir.c_str ()).accept(found, 1);
+  if (found)
       return true;
-    }
-
+  // multi-mirror?
+  Find (local_dir.c_str ()).accept(found, 2);
+  if (found)
+      return true;
+  // nope, do full scan.
   IniParseFeedback myFeedback;
   IniDBBuilderPackage myBuilder(myFeedback);
   ScanFindVisitor myVisitor (myBuilder);
-- 
2.4.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Refactor-setup-search-and-implement-XZ-compressed-se.patch --]
[-- Type: text/x-patch, Size: 21909 bytes --]

From 831c862a7fcd20137cb4002c6ea062011522f907 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Wed, 1 Jul 2015 22:05:10 +0200
Subject: [PATCH 3/5] Refactor setup search and implement XZ compressed setup
 files

	* ini.cc: Construct setup_ext_list from array until we can use
	C++11 aggregate initializers.
	(decompress_ini): Refactored for use from do_local_ini and
	do_remote_ini.  Change outdated comment about setup.ini
	uncompressed size.
	(check_ini_sig): Factor out signature check.
	(fetch_remote_ini): Refactored for use from do_remote_ini.
	(do_local_ini): Iterate over search results in found_ini_list.
	Use ini_decompress and check_ini_sig.
	(do_remote_ini): Iterate over known setup file extensions from
	setup_ext_list with early-out semantics, preferring ".xz" over
	".bz2" over ".ini" extension.  Use fetch_remote_ini and
	check_ini_sig.
	* ini.h: Remove unused macros.
	* IniParseFindVisitor.cc: Remove, the search is already done by
	SetupFindVisitor in do_from_local_dir.
	* IniParseFindVisitor.cc: Ditto.
	* Makefile.am (@SETUP@_SOURCES): Ditto.
---
 ChangeLog              |  21 ++++
 IniParseFindVisitor.cc | 116 ------------------
 IniParseFindVisitor.h  |  50 --------
 Makefile.am            |   2 -
 ini.cc                 | 319 +++++++++++++++++++++++++++++--------------------
 ini.h                  |   4 -
 6 files changed, 209 insertions(+), 303 deletions(-)
 delete mode 100644 IniParseFindVisitor.cc
 delete mode 100644 IniParseFindVisitor.h

diff --git a/ChangeLog b/ChangeLog
index e21e2e6..0d12e9b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,26 @@
 2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* ini.cc: Construct setup_ext_list from array until we can use
+	C++11 aggregate initializers.
+	(decompress_ini): Refactored for use from do_local_ini and
+	do_remote_ini.  Change outdated comment about setup.ini
+	uncompressed size.
+	(check_ini_sig): Factor out signature check.
+	(fetch_remote_ini): Refactored for use from do_remote_ini.
+	(do_local_ini): Iterate over search results in found_ini_list.
+	Use ini_decompress and check_ini_sig.
+	(do_remote_ini): Iterate over known setup file extensions from
+	setup_ext_list with early-out semantics, preferring ".xz" over
+	".bz2" over ".ini" extension.  Use fetch_remote_ini and
+	check_ini_sig.
+	* ini.h: Remove unused macros.
+	* IniParseFindVisitor.cc: Remove, the search is already done by
+	SetupFindVisitor in do_from_local_dir.
+	* IniParseFindVisitor.cc: Ditto.
+	* Makefile.am (@SETUP@_SOURCES): Ditto.
+
+2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* fromcwd.cc: Remove unused includes.  Add global found_ini_list
 	to record the search result.
 	(SetupFindVisitor): Make setup.{ini,bz2,xz} known and provide bool
diff --git a/IniParseFindVisitor.cc b/IniParseFindVisitor.cc
deleted file mode 100644
index 871d6de..0000000
--- a/IniParseFindVisitor.cc
+++ /dev/null
@@ -1,116 +0,0 @@
-/*
- * Copyright (c) 2002,2007 Robert Collins.
- *
- *     This program is free software; you can redistribute it and/or modify
- *     it under the terms of the GNU General Public License as published by
- *     the Free Software Foundation; either version 2 of the License, or
- *     (at your option) any later version.
- *
- *     A copy of the GNU General Public License can be found at
- *     http://www.gnu.org/
- *
- * Written by Robert Collins <robertc@hotmail.com>
- *
- */
-
-#if 0
-static const char *cvsid =
-  "\n%%% $Id$\n";
-#endif
-
-#include "IniParseFindVisitor.h"
-
-#include "csu_util/rfc1738.h"
-
-#include "IniParseFeedback.h"
-#include "IniDBBuilder.h"
-#include "io_stream.h"
-#include "ini.h"
-#include <stdexcept>
-
-using namespace std;
-
-extern int yyparse ();
-
-IniParseFindVisitor::IniParseFindVisitor(IniDBBuilder &aBuilder,
-                                         const std::string& localroot,
-                                         IniParseFeedback &feedback)
-  : _Builder (aBuilder), _feedback (feedback), baseLength (localroot.size()),
-    local_ini(0), setup_timestamp (0), setup_version()
-{}
-
-IniParseFindVisitor::~IniParseFindVisitor(){}
-
-/* look for potential packages we can add to the in-memory package
- * database
- */
-void
-IniParseFindVisitor::visitFile(const std::string& basePath,
-                               const WIN32_FIND_DATA *theFile)
-{
-  //TODO: Test for case sensitivity issues
-  if (casecompare(SETUP_INI_FILENAME, theFile->cFileName))
-    return;
-  
-  const char *dir = basePath.c_str () + basePath.size() - strlen (SETUP_INI_DIR);
-  if (dir < basePath.c_str ())
-    return;
-  if ((dir != basePath.c_str () && dir[-1] != '/' && dir[-1] != '\\') || casecompare (SETUP_INI_DIR, dir))
-    return;
-
-  current_ini_name = basePath + theFile->cFileName;
-  
-  io_stream *ini_file = io_stream::open("file://" + current_ini_name, "rb", 0);
-
-  if (!ini_file)
-    // We don't throw an exception, because while this is fatal to parsing, it
-    // isn't to the visitation.
-    {
-      // This should never happen
-      // If we want to handle it happening, use the log strategy call
-      throw new runtime_error ("IniParseFindVisitor: failed to open ini file, which should never happen");
-      return;
-    }
-  
-  _feedback.babble("Found ini file - " + current_ini_name);
-  _feedback.iniName (current_ini_name);
-  
-  /* Copy leading part of path to temporary buffer and unescape it */
-  
-  size_t pos = baseLength + 1;
-  size_t len = basePath.size () - (pos + strlen (SETUP_INI_DIR) + 1);
-  _Builder.parse_mirror = len <= 0 ? ""
-    			  : rfc1738_unescape (basePath.substr (pos, len));
-  ini_init (ini_file, &_Builder, _feedback);
-  
-  /*yydebug = 1; */
-
-  if (yyparse () || yyerror_count > 0)
-    _feedback.error (yyerror_messages);
-  else
-    local_ini++;
-
-  if (_Builder.timestamp > setup_timestamp)
-    {
-      setup_timestamp = _Builder.timestamp;
-      setup_version = _Builder.version;
-    }
-}
-
-int
-IniParseFindVisitor::iniCount() const
-{
-  return local_ini;
-}
-
-unsigned int 
-IniParseFindVisitor::timeStamp () const
-{
-  return setup_timestamp;
-}
-
-std::string
-IniParseFindVisitor::version() const
-{
-  return setup_version;
-}
diff --git a/IniParseFindVisitor.h b/IniParseFindVisitor.h
deleted file mode 100644
index ff48f70..0000000
--- a/IniParseFindVisitor.h
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * Copyright (c) 2002,2007 Robert Collins.
- *
- *     This program is free software; you can redistribute it and/or modify
- *     it under the terms of the GNU General Public License as published by
- *     the Free Software Foundation; either version 2 of the License, or
- *     (at your option) any later version.
- *
- *     A copy of the GNU General Public License can be found at
- *     http://www.gnu.org/
- *
- * Written by Robert Collins <robertc@hotmail.com>
- *
- */
-
-#ifndef SETUP_INIPARSEFINDVISITOR_H
-#define SETUP_INIPARSEFINDVISITOR_H
-
-#include "FindVisitor.h"
-
-
-/* parse passed in setup.ini files from disk. */
-class IniDBBuilder;
-class IniParseFeedback;
-/* IniParse files and create a package db when no cached .ini exists */
-class IniParseFindVisitor : public FindVisitor
-{
-public:
-  IniParseFindVisitor (IniDBBuilder &aBuilder,
-                       const std::string& localroot,
-                       IniParseFeedback &);
-  virtual void visitFile(const std::string& basePath, const WIN32_FIND_DATA *);
-  virtual ~ IniParseFindVisitor ();
-
-  unsigned int timeStamp() const;
-  std::string version() const;
-  int iniCount() const;
-protected:
-  IniParseFindVisitor (IniParseFindVisitor const &);
-  IniParseFindVisitor & operator= (IniParseFindVisitor const &);
-private:
-  IniDBBuilder &_Builder;
-  IniParseFeedback &_feedback;
-  unsigned int baseLength;
-  int local_ini;
-  unsigned int setup_timestamp;
-  std::string setup_version;
-};
-
-#endif /* SETUP_INIPARSEFINDVISITOR_H */
diff --git a/Makefile.am b/Makefile.am
index 88330d5..e9263cc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -163,8 +163,6 @@ inilint_SOURCES = \
 	iniparse.yy \
 	IniParseFeedback.cc \
 	IniParseFeedback.h \
-	IniParseFindVisitor.cc \
-	IniParseFindVisitor.h \
 	install.cc \
 	io_stream.cc \
 	io_stream.h \
diff --git a/ini.cc b/ini.cc
index e90f98c..92642b3 100644
--- a/ini.cc
+++ b/ini.cc
@@ -39,7 +39,6 @@
 #include "mount.h"
 #include "site.h"
 #include "find.h"
-#include "IniParseFindVisitor.h"
 #include "IniParseFeedback.h"
 
 #include "io_stream.h"
@@ -57,11 +56,12 @@ extern ThreeBarProgressPage Progress;
 
 unsigned int setup_timestamp = 0;
 std::string ini_setup_version;
-std::string current_ini_sig_name;
+IniList setup_ext_list( setup_exts, setup_exts+3); // TODO: use C++11x initializer lists
 
 static BoolOption NoVerifyOption (false, 'X', "no-verify", "Don't verify setup.ini signatures");
 
 extern int yyparse ();
+
 /*extern int yydebug;*/
 
 class GuiParseFeedback : public IniParseFeedback
@@ -119,16 +119,145 @@ private:
   unsigned int lastpct;
 };
 
+static io_stream*
+decompress_ini (io_stream *ini_file)
+{
+  // Replace the current compressed setup stream with its decompressed
+  // version.  Which decompressor to use is determined by file magic.
+  io_stream *compressed_stream = compress::decompress (ini_file);
+  if (!compressed_stream)
+    {
+      /* This isn't a valid compressed file.  */
+      // TODO: if we could determine that the original input stream
+      // was an uncompressed file to begin with we could always try to
+      // uncompress and skip the corresponding check.
+      delete compressed_stream;
+      delete ini_file;
+      ini_file = NULL;
+    }
+  else
+    {
+      /* Decompress the entire file in memory.  This has the advantage
+	 that input_stream->get_size() will work during parsing and
+	 we'll have an accurate status bar.  Also, we can't seek
+	 compressed streams, so when we write out a local cached copy
+	 of the .ini file below, we'd otherwise have to delete this
+	 stream and uncompress it again from the start, which is
+	 wasteful.  The current uncompressed size of the setup.ini
+	 file as of 2015 is about 5 MiB, so this is not a great deal
+	 of memory.  */
+      io_stream *uncompressed = new io_stream_memory ();
+      /* Note that the decompress io_stream now "owns" the underlying
+	 compressed io_stream instance, so it need not be deleted
+	 explicitly. */
+      if ((io_stream::copy (compressed_stream, uncompressed) != 0) ||
+	  (compressed_stream->error() != 0))
+	{
+	  /* There was a problem decompressing compressed_stream.  */
+	  Log (LOG_PLAIN) <<
+	    "Warning: Error code " << compressed_stream->error() <<
+	    " occurred while uncompressing " << current_ini_name <<
+	    " - possibly truncated or corrupt file. "
+	    " Retrying with uncompressed version." << endLog;
+	  delete uncompressed;
+	  ini_file = NULL;
+	}
+      else
+	{
+	  ini_file = uncompressed;
+	  ini_file->seek (0, IO_SEEK_SET);
+	}
+    }
+  return ini_file;
+}
+
+static io_stream*
+check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
+	       bool& sig_fail, const char* site, const char* sig_name, HWND owner)
+{
+  // Unless the NoVerifyOption is set, check the signature for the
+  // current setup and record the result.  On a failed signature check
+  // the streams are invalidated so even if we tried to read in the
+  // setup anyway there's be nothing to parse.
+  if (!NoVerifyOption && ini_file)
+    {
+      if (!ini_sig_file)
+	{
+	  note (owner, IDS_SETUPINI_MISSING, sig_name, site);
+	  delete ini_file;
+	  ini_file = NULL;
+	  sig_fail = true;
+	}
+      else if (!verify_ini_file_sig (ini_file, ini_sig_file, owner))
+	{
+	  note (owner, IDS_SIG_INVALID, sig_name, site);
+	  delete ini_sig_file;
+	  ini_sig_file = NULL;
+	  delete ini_file;
+	  ini_file = NULL;
+	  sig_fail = true;
+	}
+    }
+  return ini_file;
+}
+
 static int
-do_local_ini (HWND)
+do_local_ini (HWND owner)
 {
+  size_t ini_count = 0;
   GuiParseFeedback myFeedback;
-  IniDBBuilderPackage findBuilder(myFeedback);
-  IniParseFindVisitor myVisitor (findBuilder, local_dir, myFeedback);
-  Find (local_dir).accept(myVisitor, 2);	// Only search two levels deep.
-  setup_timestamp = myVisitor.timeStamp();
-  ini_setup_version = myVisitor.version();
-  return myVisitor.iniCount();
+  IniDBBuilderPackage aBuilder(myFeedback);
+  io_stream *ini_file, *ini_sig_file;
+  // iterate over all setup files found in do_from_local_dir
+  for (IniList::const_iterator n = found_ini_list.begin();
+       n != found_ini_list.end(); ++n)
+    {
+      bool sig_fail = false;
+      std::string current_ini_ext, current_ini_name, current_ini_sig_name;
+
+      current_ini_name = *n;
+      current_ini_sig_name = current_ini_name + ".sig";
+      current_ini_ext = current_ini_name.substr(current_ini_name.rfind(".") + 1);
+      ini_sig_file = io_stream::open("file://" + current_ini_sig_name, "rb", 0);
+      ini_file = io_stream::open("file://" + current_ini_name, "rb", 0);
+      ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
+				"localdir", current_ini_sig_name.c_str(), owner);
+      // did we find a compressed setup?
+      if (ini_file &&
+	  !(casecompare (current_ini_ext, "xz") &&
+	    casecompare (current_ini_ext, "bz2")))
+	ini_file = decompress_ini(ini_file);
+      if (!ini_file || sig_fail)
+	{
+	  // no setup found or signature invalid
+	  note (owner, IDS_SETUPINI_MISSING, SetupBaseName.c_str(),
+		"localdir");
+	}
+      else
+	{
+	  // grok information from setup
+	  myFeedback.babble("Found ini file - " + current_ini_name);
+	  myFeedback.iniName (current_ini_name);
+	  aBuilder.parse_mirror = "";
+	  ini_init (ini_file, &aBuilder, myFeedback);
+	  
+	  /*yydebug = 1; */
+
+	  if (yyparse () || yyerror_count > 0)
+	    myFeedback.error (yyerror_messages);
+	  else
+	    ++ini_count;
+
+	  if (aBuilder.timestamp > setup_timestamp)
+	    {
+	      setup_timestamp = aBuilder.timestamp;
+	      ini_setup_version = aBuilder.version;
+	    }
+	  delete ini_file;
+	  ini_file = NULL;
+	}
+    }
+  return ini_count;
 }
 
 static int
@@ -140,146 +269,75 @@ do_remote_ini (HWND owner)
   io_stream *ini_file, *ini_sig_file;
 
   /* FIXME: Get rid of this io_stream pointer travesty.  The need to
-     explicitly delete these things is ridiculous.  Note that the
-     decompress io_stream "owns" the underlying compressed io_stream
-     instance, so it should not be deleted explicitly.  */
+     explicitly delete these things is ridiculous. */
 
+  // iterate over all sites
   for (SiteList::const_iterator n = site_list.begin();
        n != site_list.end(); ++n)
     {
       bool sig_fail = false;
-      /* First try to fetch the .bz2 compressed ini file.  */
-      current_ini_name = n->url + SETUP_INI_DIR + SETUP_BZ2_FILENAME;
-      current_ini_sig_name = n->url + SETUP_INI_DIR + SETUP_BZ2_FILENAME + ".sig";
-      ini_file = get_url_to_membuf (current_ini_name, owner);
-      if (!NoVerifyOption)
-	ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
-      if (!NoVerifyOption && ini_file && !ini_sig_file)
+      std::string current_ini_ext, current_ini_name, current_ini_sig_name;
+      // iterate over known extensions for setup
+      for (IniList::const_iterator ext = setup_ext_list.begin();
+	   ext != setup_ext_list.end();
+	   ext++)
 	{
-	  note (owner, IDS_SETUPINI_MISSING, current_ini_sig_name.c_str(), n->url.c_str());
-	  delete ini_file;
-	  ini_file = NULL;
-	  sig_fail = true;
+	  current_ini_ext = *ext;
+	  current_ini_name = n->url + SetupIniDir + SetupBaseName + "." + current_ini_ext;
+	  current_ini_sig_name = current_ini_name + ".sig";
+	  ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
+	  ini_file = get_url_to_membuf (current_ini_name, owner);
+	  ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
+				    n->url.c_str(), current_ini_sig_name.c_str(), owner);
+	  // stop searching as soon as we find a setup file
+	  if (ini_file)
+	    break;
 	}
-      else if (!NoVerifyOption && ini_file && !verify_ini_file_sig (ini_file, ini_sig_file, owner))
+      // did we find a compressed setup?
+      if (ini_file &&
+	  !(casecompare (current_ini_ext, "xz") &&
+	    casecompare (current_ini_ext, "bz2")))
+	ini_file = decompress_ini(ini_file);
+      if (!ini_file || sig_fail)
 	{
-	  note (owner, IDS_SIG_INVALID, current_ini_sig_name.c_str(), n->url.c_str());
-	  delete ini_file;
-	  ini_file = NULL;
-	  delete ini_sig_file;
-	  ini_sig_file = NULL;
-	  sig_fail = true;
+	  // no setup found or signature invalid
+	  note (owner, IDS_SETUPINI_MISSING, SetupBaseName.c_str(), n->url.c_str());
 	}
-      if (ini_file)
+      else
 	{
-	  /* Decompress the entire file in memory right now.  This has the
-	     advantage that input_stream->get_size() will work during parsing
-	     and we'll have an accurate status bar.  Also, we can't seek
-	     bz2 streams, so when it comes time to write out a local cached
-	     copy of the .ini file below, we'd otherwise have to delete this
-	     stream and uncompress it again from the start, which is wasteful.
-	     The current uncompresed size of the .ini file as of 2007 is less
-	     than 600 kB, so this is not a great deal of memory.  */
-	  io_stream *bz2_stream = compress::decompress (ini_file);
-	  if (!bz2_stream)
-	    {
-	      /* This isn't a valid bz2 file.  */
-	      delete ini_file;
-	      ini_file = NULL;
-	    }
+	  // grok information from setup
+	  myFeedback.iniName (current_ini_name);
+	  aBuilder.parse_mirror = n->url;
+	  ini_init (ini_file, &aBuilder, myFeedback);
+	  
+	  /*yydebug = 1; */
+
+	  if (yyparse () || yyerror_count > 0)
+	    myFeedback.error (yyerror_messages);
 	  else
 	    {
-	      io_stream *uncompressed = new io_stream_memory ();
-
-	      if ((io_stream::copy (bz2_stream, uncompressed) != 0) ||
-                  (bz2_stream->error() != 0))
-		{
-		  /* There was a problem decompressing bz2.  */
-		  ini_file = NULL;
-		  Log (LOG_PLAIN) <<
-		    "Warning: Error code " << bz2_stream->error() << " occurred while uncompressing " <<
-		    current_ini_name << " - possibly truncated or corrupt bzip2"
-		    " file.  Retrying with uncompressed version." << endLog;
-		  delete bz2_stream;
-		  delete uncompressed;
-		}
-	      else
+	      /* save known-good setup.ini locally */
+	      const std::string fp = "file://" + local_dir + "/" +
+				      rfc1738_escape_part (n->url) +
+				      "/" + SetupIniDir + SetupBaseName + ".ini";
+	      io_stream::mkpath_p (PATH_TO_FILE, fp, 0);
+	      if (io_stream *out = io_stream::open (fp, "wb", 0))
 		{
-		  delete bz2_stream;
-		  ini_file = uncompressed;
 		  ini_file->seek (0, IO_SEEK_SET);
+		  if (io_stream::copy (ini_file, out) != 0)
+		    io_stream::remove (fp);
+		  delete out;
 		}
+	      ++ini_count;
 	    }
-	}
-
-      if (!ini_file)
-	{
-	  /* Try to look for a plain .ini file because one of the following
-	     happened above:
-	       - there was no .bz2 file found on the mirror.
-	       - the .bz2 file didn't look like a valid bzip2 file.
-	       - there was an error during bzip2 decompression.  */
-	  current_ini_name = n->url + SETUP_INI_DIR + SETUP_INI_FILENAME;
-	  current_ini_sig_name = n->url + SETUP_INI_DIR + SETUP_INI_FILENAME + ".sig";
-	  ini_file = get_url_to_membuf (current_ini_name, owner);
-	  if (!NoVerifyOption)
-	    ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
-
-	  if (!NoVerifyOption && ini_file && !ini_sig_file)
-	    {
-	      note (owner, IDS_SETUPINI_MISSING, current_ini_sig_name.c_str(), n->url.c_str());
-	      delete ini_file;
-	      ini_file = NULL;
-	      sig_fail = true;
-	    }
-	  else if (!NoVerifyOption && ini_file && !verify_ini_file_sig (ini_file, ini_sig_file, owner))
-	    {
-	      note (owner, IDS_SIG_INVALID, current_ini_sig_name.c_str(), n->url.c_str());
-	      delete ini_file;
-	      ini_file = NULL;
-	      delete ini_sig_file;
-	      ini_sig_file = NULL;
-	      sig_fail = true;
-	    }
-	}
-
-      if (!ini_file)
-	{
-	  if (!sig_fail)
-	    note (owner, IDS_SETUPINI_MISSING, SETUP_INI_FILENAME, n->url.c_str());
-	  continue;
-	}
-
-      myFeedback.iniName (current_ini_name);
-      aBuilder.parse_mirror = n->url;
-      ini_init (ini_file, &aBuilder, myFeedback);
-
-      /*yydebug = 1; */
-
-      if (yyparse () || yyerror_count > 0)
-	myFeedback.error (yyerror_messages);
-      else
-	{
-	  /* save known-good setup.ini locally */
-	  const std::string fp = "file://" + local_dir + "/" +
-				   rfc1738_escape_part (n->url) +
-				   "/" + SETUP_INI_DIR + SETUP_INI_FILENAME;
-	  io_stream::mkpath_p (PATH_TO_FILE, fp, 0);
-	  if (io_stream *out = io_stream::open (fp, "wb", 0))
+	  if (aBuilder.timestamp > setup_timestamp)
 	    {
-	      ini_file->seek (0, IO_SEEK_SET);
-	      if (io_stream::copy (ini_file, out) != 0)
-		io_stream::remove (fp);
-	      delete out;
+	      setup_timestamp = aBuilder.timestamp;
+	      ini_setup_version = aBuilder.version;
 	    }
-	  ++ini_count;
-	}
-      if (aBuilder.timestamp > setup_timestamp)
-	{
-	  setup_timestamp = aBuilder.timestamp;
-	  ini_setup_version = aBuilder.version;
+	  delete ini_file;
+	  ini_file = NULL;
 	}
-      delete ini_file;
     }
   return ini_count;
 }
@@ -374,4 +432,3 @@ do_ini (HINSTANCE h, HWND owner)
   DWORD threadID;
   CreateThread (NULL, 0, do_ini_thread_reflector, context, 0, &threadID);
 }
-
diff --git a/ini.h b/ini.h
index c2903ef..164e3d2 100644
--- a/ini.h
+++ b/ini.h
@@ -48,10 +48,6 @@ typedef enum
   EXCLUDE_NOT_FOUND
 } excludes;
 
-#define SETUP_INI_DIR	   SetupIniDir.c_str()
-#define SETUP_INI_FILENAME (SetupBaseName+".ini").c_str()
-#define SETUP_BZ2_FILENAME (SetupBaseName+".bz2").c_str()
-
 /* The following three vars are used to facilitate error handling between the
    parser/lexer and its callers, namely ini.cc:do_remote_ini() and
    IniParseFindVisitor::visitFile().  */
-- 
2.4.3


[-- Attachment #5: Type: text/plain, Size: 199 bytes --]



Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 6180 bytes --]

Hi Achim,

On Jul  1 22:37, Achim Gratz wrote:
> Corinna Vinschen writes:
> > Ok, for once.  But please make sure that you split the commit into
> > functional chunks next time it's so large.  And send it to this list, so
> > code snippets can be referenced in the review.
> 
> I've split it up into three parts that at least compile cleanly.

Thanks!

> +extern IniList found_ini_list, setup_ext_list;
> +const std::string setup_exts[] = { "xz", "bz2", "ini" };

See (*) below.

> @@ -97,6 +99,8 @@ static BoolOption PackageManagerOption (false, 'M', "package-manager", "Semi-att
>  static BoolOption NoAdminOption (false, 'B', "no-admin", "Do not check for and enforce running as Administrator");
>  static BoolOption WaitOption (false, 'W', "wait", "When elevating, wait for elevated child process");
>  static BoolOption HelpOption (false, 'h', "help", "print help");
> +static StringOption SetupBaseNameOpt ("setup", 'i', "ini-basename", "Use a different basename instead of setup", false);

The word setup needs quoting of some sort I think.  An example wouldn't
hurt either.  What about

  "Use a different ini file basename instead of \"setup\", e.g. \"foo\".ini"

?

> +	(theFile->nFileSizeLow || theFile->nFileSizeHigh))
> +      {
> +	if (!casecompare (SetupBaseName + ".xz",  theFile->cFileName))
> +	  found_xz  = true;
> +	if (!casecompare (SetupBaseName + ".bz2", theFile->cFileName))
> +	  found_bz2 = true;
> +	if (!casecompare (SetupBaseName + ".ini", theFile->cFileName))
> +	  found_ini = true;
> +      }

(*) This puzzles me a bit.  You're keeping arrays and lists in terms of
the file suffix (setup_ext, setup_ext_list), but you don't use the
information here and elsewhere.  The setup_exts array already contains
the suffixes and constitutes an order.  If you extend the array of
strings to a struct, you could just run a loop over it in places like
the above and generalize the suffix handling.  For instance...

  struct ini_suffix_t
  {
    char *suffix;
    bool needs_decompressing;
    [...]
  } 

  ini_suffix_t ini_suffixes[] = {
    { "xz", true },
    { "bz2", true },
    { "ini", false },
    { NULL, false } };

  bool found_ini[];  // <- just as example

  for (i = 0; ini_suffixes[i].suffix; ++i)
    if (!casecompare (SetupBaseName + "." + ini_suffixes[i].suffix, ...)
      found_ini[i] = true;

This would work OOTB, without C++11x initializer lists, and you could
drop setup_ext_list entirely.

This would also simplify things if there's a new compression we want to
support.

> +    if (found_xz)
> +      found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" + SetupBaseName + ".xz");
> +    else if (found_bz2)
> +      found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" + SetupBaseName + ".bz2");
> +    else if (found_ini)
> +      found_ini_list.push_back(basePath + "/" + aDir->cFileName + "/" + SetupBaseName + ".ini");

Same kind of loop here.

  for (i = 0; ini_suffixes[i].suffix; ++i)
    if (found_ini[i])
      found_ini_list.push_back (... + ini_suffixes[i].suffix);

Ideally found_ini_list keeps a pointer into ini_suffixes as well so
you don't have to extract the suffix again at (**).

> Subject: [PATCH 3/5] Refactor setup search and implement XZ compressed setup
>  files
> 
> 	* ini.cc: Construct setup_ext_list from array until we can use
> 	C++11 aggregate initializers.
> 	(decompress_ini): Refactored for use from do_local_ini and
> 	do_remote_ini.  Change outdated comment about setup.ini
> 	uncompressed size.
> 	(check_ini_sig): Factor out signature check.
> 	(fetch_remote_ini): Refactored for use from do_remote_ini.
> 	(do_local_ini): Iterate over search results in found_ini_list.
> 	Use ini_decompress and check_ini_sig.
	    ^^^^^^^^^^^^^^
            missing name change

> +  // Unless the NoVerifyOption is set, check the signature for the
> +  // current setup and record the result.  On a failed signature check
> +  // the streams are invalidated so even if we tried to read in the
> +  // setup anyway there's be nothing to parse.

I'd prefer /* */ for multiline comments, but that's used pretty
inconsistently anyway, so, never mind.

> +  IniDBBuilderPackage aBuilder(myFeedback);
> +  io_stream *ini_file, *ini_sig_file;
> +  // iterate over all setup files found in do_from_local_dir
> +  for (IniList::const_iterator n = found_ini_list.begin();
> +       n != found_ini_list.end(); ++n)
> +    {
> +      bool sig_fail = false;
> +      std::string current_ini_ext, current_ini_name, current_ini_sig_name;
> +
> +      current_ini_name = *n;
> +      current_ini_sig_name = current_ini_name + ".sig";
> +      current_ini_ext = current_ini_name.substr(current_ini_name.rfind(".") + 1);

(**) See above.

> +      ini_sig_file = io_stream::open("file://" + current_ini_sig_name, "rb", 0);
> +      ini_file = io_stream::open("file://" + current_ini_name, "rb", 0);
> +      ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
> +				"localdir", current_ini_sig_name.c_str(), owner);

The existing code is inconsistently formatted, but for new code it would
be nice if we could try to be more consistent.  Always prepend a space
to a left parenthesis, please.

> +      // did we find a compressed setup?
> +      if (ini_file &&
> +	  !(casecompare (current_ini_ext, "xz") &&
> +	    casecompare (current_ini_ext, "bz2")))
> +	ini_file = decompress_ini(ini_file);

(*) And this could simply use the information from ini_suffixes if
"current_ini_ext" wouldn't be the actual extension, but rather a pointer
into ini_suffixes.  Kind of like

  if (ini_file && current_ini_ext->needs_decompressing)
    ini_file = decompress_ini (ini_file);


Sorry if that's a lot.  It just occured to me while reading your code.
I'm not adamant about the structural change I outlined above, but to
me it seems better to do it that way.  What do you think?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> (*) This puzzles me a bit.  You're keeping arrays and lists in terms of
> the file suffix (setup_ext, setup_ext_list), but you don't use the
> information here and elsewhere.

THat vector is a relatively late addition to the code when the rest was
already using literals as before.  It's something that I want to clean
up, but not necessarily right now.  As you noted, I'd probably need to
use a more structured data type.

> I'd prefer /* */ for multiline comments, but that's used pretty
> inconsistently anyway, so, never mind.

If you insist… but setup is C++ and not C anyway.

> The existing code is inconsistently formatted, but for new code it would
> be nice if we could try to be more consistent.  Always prepend a space
> to a left parenthesis, please.

I'll check that.

> Sorry if that's a lot.  It just occured to me while reading your code.
> I'm not adamant about the structural change I outlined above, but to
> me it seems better to do it that way.  What do you think?

It will just have to wait a bit, I think.  But yes, these are all good
suggestions.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Jul  2 23:03, Achim Gratz wrote:
> Corinna Vinschen writes:
> > (*) This puzzles me a bit.  You're keeping arrays and lists in terms of
> > the file suffix (setup_ext, setup_ext_list), but you don't use the
> > information here and elsewhere.
> 
> THat vector is a relatively late addition to the code when the rest was
> already using literals as before.  It's something that I want to clean
> up, but not necessarily right now.  As you noted, I'd probably need to
> use a more structured data type.
> 
> > I'd prefer /* */ for multiline comments, but that's used pretty
> > inconsistently anyway, so, never mind.
> 
> If you insist… but setup is C++ and not C anyway.

Not so much insisting, just a wish.

> > The existing code is inconsistently formatted, but for new code it would
> > be nice if we could try to be more consistent.  Always prepend a space
> > to a left parenthesis, please.
> 
> I'll check that.
> 
> > Sorry if that's a lot.  It just occured to me while reading your code.
> > I'm not adamant about the structural change I outlined above, but to
> > me it seems better to do it that way.  What do you think?
> 
> It will just have to wait a bit, I think.  But yes, these are all good
> suggestions.

Cool.  If you want to check in the current state (plus formatting
tweaks), feel free.  It would just be nice if you would consider
to make the structural changes at one point.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

Corinna Vinschen writes:
> Sorry if that's a lot.  It just occured to me while reading your code.
> I'm not adamant about the structural change I outlined above, but to
> me it seems better to do it that way.  What do you think?

I've implemented your suggestions except the struct stuff.  I've not as
thoroughly tested it as last time, though.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Implement-option-i-ini-basename-to-set-the-basename-.patch --]
[-- Type: text/x-patch, Size: 3489 bytes --]

From 296c4272b13d8fdd3a16bb231fc2d2bfe3ba6c29 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Sun, 28 Jun 2015 13:34:11 +0200
Subject: [PATCH 1/6] Implement option "-i/--ini-basename" to set the basename
 for setup

	* ini.h: Declare extern SetupBaseName, SetupArch and SetupIniDir.
	Redefine macros to use them.

	* main.cc: Provide option "-i/--ini-basename" to set the basename
	for setup, keep the default as "setup" and store in SetupBaseName.
	Initialize SetupArch and SetupIniDir based on effective
	architecture.
---
 ChangeLog | 10 ++++++++++
 ini.h     | 17 +++++++++++++----
 main.cc   |  8 ++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1c50bfc..cb49b7e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
+
+	* ini.h: Declare extern SetupBaseName, SetupArch and SetupIniDir.
+	Redefine macros to use them.
+
+	* main.cc: Provide option "-i/--ini-basename" to set the basename
+	for setup, keep the default as "setup" and store in SetupBaseName.
+	Initialize SetupArch and SetupIniDir based on effective
+	architecture.
+
 2015-06-11  Achim Gratz  <Stromeko@NexGo.DE>
 
 	* inilex.ll: Increase read buffer size for the lexer input buffer
diff --git a/ini.h b/ini.h
index ec09def..1969da9 100644
--- a/ini.h
+++ b/ini.h
@@ -18,6 +18,16 @@
 
 class io_stream;
 #include <string>
+#include <vector>
+
+typedef std::vector <std::string> IniList;
+extern IniList found_ini_list, setup_ext_list;
+const std::string setup_exts[] = { "xz", "bz2", "ini" };
+extern bool is_64bit;
+extern std::string SetupArch;
+extern std::string SetupIniDir;
+extern std::string SetupBaseName;
+
 class IniState;
 class IniDBBuilder;
 class IniParseFeedback;
@@ -38,10 +48,9 @@ typedef enum
   EXCLUDE_NOT_FOUND
 } excludes;
 
-extern bool is_64bit;
-#define SETUP_INI_DIR	   (is_64bit ? "x86_64/" : "x86/")
-#define SETUP_INI_FILENAME "setup.ini"
-#define SETUP_BZ2_FILENAME "setup.bz2"
+#define SETUP_INI_DIR	   (SetupIniDir.c_str ())
+#define SETUP_INI_FILENAME ((SetupBaseName+".ini").c_str ())
+#define SETUP_BZ2_FILENAME ((SetupBaseName+".bz2").c_str ())
 
 /* The following three vars are used to facilitate error handling between the
    parser/lexer and its callers, namely ini.cc:do_remote_ini() and
diff --git a/main.cc b/main.cc
index e968d93..b0d78eb 100644
--- a/main.cc
+++ b/main.cc
@@ -86,6 +86,8 @@ extern char **_argv;
 #endif
 
 bool is_64bit;
+std::string SetupArch;
+std::string SetupIniDir;
 
 using namespace std;
 
@@ -97,6 +99,8 @@ static BoolOption PackageManagerOption (false, 'M', "package-manager", "Semi-att
 static BoolOption NoAdminOption (false, 'B', "no-admin", "Do not check for and enforce running as Administrator");
 static BoolOption WaitOption (false, 'W', "wait", "When elevating, wait for elevated child process");
 static BoolOption HelpOption (false, 'h', "help", "print help");
+static StringOption SetupBaseNameOpt ("setup", 'i', "ini-basename", "Use a different basename, e.g. \"foo\", instead of \"setup\"", false);
+std::string SetupBaseName;
 
 static void inline
 set_cout ()
@@ -262,6 +266,10 @@ WinMain (HINSTANCE h,
     if (unattended_mode || help_option)
       set_cout ();
 
+    SetupBaseName = SetupBaseNameOpt;
+    SetupArch = is_64bit ? "x86_64" : "x86";
+    SetupIniDir = SetupArch+"/";
+
     /* Get System info */
     OSVERSIONINFO version;
     version.dwOSVersionInfoSize = sizeof version;
-- 
2.4.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Correct-local-directory-search.patch --]
[-- Type: text/x-patch, Size: 8560 bytes --]

From 35addac4a65a1d70ab02b8d175f71efa83cf74ba Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Wed, 1 Jul 2015 21:36:12 +0200
Subject: [PATCH 2/6] Correct local directory search

	* fromcwd.cc: Remove unused includes.  Add global found_ini_list
	to record the search result.
	(SetupFindVisitor): Make setup.{ini,bz2,xz} known and provide bool
	private variables to record whether we found them.  Another bool
	inidir to indicate whether we are currently inside a directory
	where setup.ini files should exist.
	(SetupFindVisitor::visitFile): When inidir is true, check if a
	setup file with one of the known extensions was found and set the
	corresponding bool variables.
        (SetupFindVisitor::visitDirectory): Set inidir when appropriate.
	Recurse into directories only if they are potential mirror dirs,
	based on level. Truncate search and recurse into inidir.  Record
	any setup files in found_ini_list while preferring ".xz" over
	".bz2" over ".ini" extension.
	(SetupFindVisitor::operator bool): Return true when found_ini_list
	is non-empty.
	(do_from_local_dir): Restrict search to either a single mirror
	hierarchy or multiple mirror hierarchy; setup files directly in
	local directory or mixed hierarchies are no longer recognized.
	The setup files must be present in an architecture dependent
	directory of "x86/" or "x86_64/", either in the local directory
	for single mirror or one level down for multiple mirrors.

This patch already finds setup.xz files, but the code to deal with
them will be in a later patch.  The current code re-does the search
and ignores the results from here.
---
 ChangeLog  |  25 ++++++++++
 fromcwd.cc | 154 ++++++++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 132 insertions(+), 47 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cb49b7e..e21e2e6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,30 @@
 2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* fromcwd.cc: Remove unused includes.  Add global found_ini_list
+	to record the search result.
+	(SetupFindVisitor): Make setup.{ini,bz2,xz} known and provide bool
+	private variables to record whether we found them.  Another bool
+	inidir to indicate whether we are currently inside a drectory
+	where setup.ini files should exist.
+	(SetupFindVisitor::visitFile): When inidir is true, check if a
+	setup file with one of the known extensions was found and set the
+	corresponding bool variables.
+        (SetupFindVisitor::visitDirectory): Set inidir when appropriate.
+	Recurse into directories only if they are potential mirror dirs,
+	based on level. Truncate search and recurse into inidir.  Record
+	any setup files in found_ini_list while preferring ".xz" over
+	".bz2" over ".ini" extension.
+	(SetupFindVisitor::operator bool): Return true when found_ini_list
+	is non-empty.
+	(do_from_local_dir): Restrict search to either a single mirror
+	hierarchy or multiple mirror hierarchy; setup files directly in
+	local directory or mixed hierarchies are no longer recognized.
+	The setup files must be present in an architecture dependent
+	directory of "x86/" or "x86_64/", either in the local directory
+	for single mirror or one level down for multiple mirrors.
+
+2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* ini.h: Declare extern SetupBaseName, SetupArch and SetupIniDir.
 	Redefine macros to use them.
 
diff --git a/fromcwd.cc b/fromcwd.cc
index 87da0ad..edb67a6 100644
--- a/fromcwd.cc
+++ b/fromcwd.cc
@@ -25,77 +25,137 @@
    (otherwise, why would you have asked to install it?).  Note
    that we search recursively. */
 
-#if 0
-static const char *cvsid =
-  "\n%%% $Id$\n";
-#endif
-
-#include "win32.h"
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
 
-#include "resource.h"
-#include "state.h"
-#include "dialog.h"
-#include "msg.h"
+#include "String++.h"
 #include "find.h"
-#include "ScanFindVisitor.h"
-#include "filemanip.h"
 #include "ini.h"
 
+#include "ScanFindVisitor.h"
 #include "IniDBBuilderPackage.h"
 #include "IniParseFeedback.h"
 
+#define DEBUG_FROMCWD
+#ifdef DEBUG_FROMCWD
+#include "LogFile.h"
+#endif
+
 /* Trivial class for detecting the existence of setup.ini */
 
 class SetupFindVisitor : public FindVisitor
 {
 public:
-  SetupFindVisitor (): found(false){}
-  virtual void visitFile(const std::string& basePath,
-                         const WIN32_FIND_DATA *theFile)
-    {
-      if (!casecompare (SETUP_INI_FILENAME, theFile->cFileName) && 
-	  (theFile->nFileSizeLow || theFile->nFileSizeHigh))
-	{
-	  /* Check if base dir ends in SETUP_INI_DIR. */
-	  const char *dir = basePath.c_str() + basePath.size ()
-			    - strlen (SETUP_INI_DIR);
-	  if (dir < basePath.c_str ())
-	    return;
-	  if ((dir != basePath.c_str () && dir[-1] != '/' && dir[-1] != '\\')
-	      || casecompare (SETUP_INI_DIR, dir))
-	    return;
-	  found = true;
-	}
-    }
+  SetupFindVisitor () : inidir (false)
+  {
+    found_ini.resize (setup_ext_list.size ());
+    found_ini.assign (setup_ext_list.size (), false);
+  }
+  virtual void visitFile (const std::string& basePath,
+			  const WIN32_FIND_DATA *theFile)
+  {
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "examining file: "
+		    << basePath << "./" << theFile->cFileName << endLog;
+#endif
+    if (inidir &&
+	(theFile->nFileSizeLow || theFile->nFileSizeHigh))
+      {
+#ifdef DEBUG_FROMCWD
+	Log (LOG_PLAIN) << "checking extension: ";
+#endif
+	std::vector<bool>::iterator fi = found_ini.begin ();
+	for (std::vector<std::string>::const_iterator ext = setup_ext_list.begin ();
+	     ext != setup_ext_list.end ();
+	     ext++, fi++)
+	  {
+#ifdef DEBUG_FROMCWD
+	    Log (LOG_PLAIN) << *ext << " ";
+#endif
+	    if (!casecompare (SetupBaseName + "." + *ext,  theFile->cFileName))
+	      *fi = true;
+	  }
+#ifdef DEBUG_FROMCWD
+	Log (LOG_PLAIN) << endLog;
+#endif
+      }
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "state: "
+		    << " inidir="    << inidir
+		    << " found_xz="  << found_ini[0]
+		    << " found_bz2=" << found_ini[1]
+		    << " found_ini=" << found_ini[2]
+		    << endLog;
+#endif
+  }
+  virtual void visitDirectory (const std::string& basePath,
+			       WIN32_FIND_DATA const *aDir, int level)
+  {
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "examining directory: "
+		    << basePath << "./" << aDir->cFileName
+		    << endLog;
+#endif
+    if (level <= 0)
+      return;
+    inidir = !casecompare (SetupArch, aDir->cFileName);
+    if (level == 1 && !inidir)
+      return;
+#ifdef DEBUG_FROMCWD
+    Log (LOG_PLAIN) << "  recurse into: "
+		    << (inidir ? "inidir" : "mirror") << endLog;
+#endif
+    Find aFinder (basePath + aDir->cFileName);
+    aFinder.accept (*this, inidir ? 0 : --level);
+	std::vector<bool>::const_iterator fi = found_ini.begin ();
+	for (std::vector<std::string>::const_iterator ext = setup_ext_list.begin ();
+	     ext != setup_ext_list.end ();
+	     ext++, fi++)
+	  {
+	    if (*fi)
+	      found_ini_list.push_back (basePath + "/" + aDir->cFileName + "/"
+					+ SetupBaseName + "." + *ext);
+	  }
+	found_ini.assign (setup_ext_list.size (), false);
+  }
   virtual ~ SetupFindVisitor (){}
-  operator bool () const {return found;}
+  operator bool () const
+  {
+    return !found_ini_list.empty ();
+  }
 protected:
   SetupFindVisitor (SetupFindVisitor const &);
   SetupFindVisitor & operator= (SetupFindVisitor const &);
 private:
-  bool found;
+  bool inidir;
+  std::vector<bool> found_ini;
 };
-  
+
+IniList found_ini_list;
+
 bool
 do_from_local_dir (HINSTANCE h, HWND owner, std::string &local_dir)
 {
+#ifdef DEBUG_FROMCWD
+  Log (LOG_PLAIN) << "do_from_local_dir: "
+		  << local_dir << endLog;
+#endif
   // Assume we won't find the INI file.
-  SetupFindVisitor found_ini;
-  // Only search two levels deep.
-  Find (local_dir.c_str ()).accept(found_ini, 2);
-  if (found_ini)
-    {
-      // Found INI, load it.
+  SetupFindVisitor found;
+  // single mirror?
+  Find (local_dir.c_str ()).accept (found, 1);
+  if (found)
       return true;
-    }
-
+  // multi-mirror?
+  Find (local_dir.c_str ()).accept (found, 2);
+  if (found)
+      return true;
+  // nope, do full scan.
+#ifdef DEBUG_FROMCWD
+  Log (LOG_PLAIN) << "  starting full scan from "
+		  << local_dir << endLog;
+#endif
   IniParseFeedback myFeedback;
-  IniDBBuilderPackage myBuilder(myFeedback);
+  IniDBBuilderPackage myBuilder (myFeedback);
   ScanFindVisitor myVisitor (myBuilder);
-  Find(local_dir.c_str ()).accept(myVisitor);
+  Find (local_dir.c_str ()).accept (myVisitor);
   return false;
 }
-- 
2.4.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Refactor-setup-search-and-implement-XZ-compressed-se.patch --]
[-- Type: text/x-patch, Size: 24897 bytes --]

From c843bf7d1e10f4d2e3d63807410298aca8cf63c4 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Sat, 11 Jul 2015 21:58:30 +0200
Subject: [PATCH 3/6] Refactor setup search and implement XZ compressed setup
 files

	* ini.cc: Construct setup_ext_list from array until we can use
	C++11 aggregate initializers.  Clean up function parenthesis.
	(decompress_ini): Refactored for use from do_local_ini and
	do_remote_ini.  Change outdated comment about setup.ini
	uncompressed size.
	(check_ini_sig): Factor out signature check.
	(fetch_remote_ini): Refactored for use from do_remote_ini.
	(do_local_ini): Iterate over search results in found_ini_list.
	Use decompress_ini and check_ini_sig.
	(do_remote_ini): Iterate over known setup file extensions from
	setup_ext_list with early-out semantics, preferring ".xz" over
	".bz2" over ".ini" extension.  Use fetch_remote_ini and
	check_ini_sig.
	* ini.h: Remove unused macros.
	* IniParseFindVisitor.cc: Remove, the search is already done by
	SetupFindVisitor in do_from_local_dir.
	* IniParseFindVisitor.cc: Ditto.
	* Makefile.am (@SETUP@_SOURCES): Ditto.
---
 ChangeLog              |  21 +++
 IniParseFindVisitor.cc | 116 -----------------
 IniParseFindVisitor.h  |  50 -------
 Makefile.am            |   2 -
 ini.cc                 | 345 ++++++++++++++++++++++++++++---------------------
 ini.h                  |   4 -
 6 files changed, 217 insertions(+), 321 deletions(-)
 delete mode 100644 IniParseFindVisitor.cc
 delete mode 100644 IniParseFindVisitor.h

diff --git a/ChangeLog b/ChangeLog
index e21e2e6..5fc1c86 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,26 @@
 2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* ini.cc: Construct setup_ext_list from array until we can use
+	C++11 aggregate initializers.
+	(decompress_ini): Refactored for use from do_local_ini and
+	do_remote_ini.  Change outdated comment about setup.ini
+	uncompressed size.
+	(check_ini_sig): Factor out signature check.
+	(fetch_remote_ini): Refactored for use from do_remote_ini.
+	(do_local_ini): Iterate over search results in found_ini_list.
+	Use decompress_ini and check_ini_sig.
+	(do_remote_ini): Iterate over known setup file extensions from
+	setup_ext_list with early-out semantics, preferring ".xz" over
+	".bz2" over ".ini" extension.  Use fetch_remote_ini and
+	check_ini_sig.
+	* ini.h: Remove unused macros.
+	* IniParseFindVisitor.cc: Remove, the search is already done by
+	SetupFindVisitor in do_from_local_dir.
+	* IniParseFindVisitor.cc: Ditto.
+	* Makefile.am (@SETUP@_SOURCES): Ditto.
+
+2015-07-01  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* fromcwd.cc: Remove unused includes.  Add global found_ini_list
 	to record the search result.
 	(SetupFindVisitor): Make setup.{ini,bz2,xz} known and provide bool
diff --git a/IniParseFindVisitor.cc b/IniParseFindVisitor.cc
deleted file mode 100644
index 871d6de..0000000
--- a/IniParseFindVisitor.cc
+++ /dev/null
@@ -1,116 +0,0 @@
-/*
- * Copyright (c) 2002,2007 Robert Collins.
- *
- *     This program is free software; you can redistribute it and/or modify
- *     it under the terms of the GNU General Public License as published by
- *     the Free Software Foundation; either version 2 of the License, or
- *     (at your option) any later version.
- *
- *     A copy of the GNU General Public License can be found at
- *     http://www.gnu.org/
- *
- * Written by Robert Collins <robertc@hotmail.com>
- *
- */
-
-#if 0
-static const char *cvsid =
-  "\n%%% $Id$\n";
-#endif
-
-#include "IniParseFindVisitor.h"
-
-#include "csu_util/rfc1738.h"
-
-#include "IniParseFeedback.h"
-#include "IniDBBuilder.h"
-#include "io_stream.h"
-#include "ini.h"
-#include <stdexcept>
-
-using namespace std;
-
-extern int yyparse ();
-
-IniParseFindVisitor::IniParseFindVisitor(IniDBBuilder &aBuilder,
-                                         const std::string& localroot,
-                                         IniParseFeedback &feedback)
-  : _Builder (aBuilder), _feedback (feedback), baseLength (localroot.size()),
-    local_ini(0), setup_timestamp (0), setup_version()
-{}
-
-IniParseFindVisitor::~IniParseFindVisitor(){}
-
-/* look for potential packages we can add to the in-memory package
- * database
- */
-void
-IniParseFindVisitor::visitFile(const std::string& basePath,
-                               const WIN32_FIND_DATA *theFile)
-{
-  //TODO: Test for case sensitivity issues
-  if (casecompare(SETUP_INI_FILENAME, theFile->cFileName))
-    return;
-  
-  const char *dir = basePath.c_str () + basePath.size() - strlen (SETUP_INI_DIR);
-  if (dir < basePath.c_str ())
-    return;
-  if ((dir != basePath.c_str () && dir[-1] != '/' && dir[-1] != '\\') || casecompare (SETUP_INI_DIR, dir))
-    return;
-
-  current_ini_name = basePath + theFile->cFileName;
-  
-  io_stream *ini_file = io_stream::open("file://" + current_ini_name, "rb", 0);
-
-  if (!ini_file)
-    // We don't throw an exception, because while this is fatal to parsing, it
-    // isn't to the visitation.
-    {
-      // This should never happen
-      // If we want to handle it happening, use the log strategy call
-      throw new runtime_error ("IniParseFindVisitor: failed to open ini file, which should never happen");
-      return;
-    }
-  
-  _feedback.babble("Found ini file - " + current_ini_name);
-  _feedback.iniName (current_ini_name);
-  
-  /* Copy leading part of path to temporary buffer and unescape it */
-  
-  size_t pos = baseLength + 1;
-  size_t len = basePath.size () - (pos + strlen (SETUP_INI_DIR) + 1);
-  _Builder.parse_mirror = len <= 0 ? ""
-    			  : rfc1738_unescape (basePath.substr (pos, len));
-  ini_init (ini_file, &_Builder, _feedback);
-  
-  /*yydebug = 1; */
-
-  if (yyparse () || yyerror_count > 0)
-    _feedback.error (yyerror_messages);
-  else
-    local_ini++;
-
-  if (_Builder.timestamp > setup_timestamp)
-    {
-      setup_timestamp = _Builder.timestamp;
-      setup_version = _Builder.version;
-    }
-}
-
-int
-IniParseFindVisitor::iniCount() const
-{
-  return local_ini;
-}
-
-unsigned int 
-IniParseFindVisitor::timeStamp () const
-{
-  return setup_timestamp;
-}
-
-std::string
-IniParseFindVisitor::version() const
-{
-  return setup_version;
-}
diff --git a/IniParseFindVisitor.h b/IniParseFindVisitor.h
deleted file mode 100644
index ff48f70..0000000
--- a/IniParseFindVisitor.h
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * Copyright (c) 2002,2007 Robert Collins.
- *
- *     This program is free software; you can redistribute it and/or modify
- *     it under the terms of the GNU General Public License as published by
- *     the Free Software Foundation; either version 2 of the License, or
- *     (at your option) any later version.
- *
- *     A copy of the GNU General Public License can be found at
- *     http://www.gnu.org/
- *
- * Written by Robert Collins <robertc@hotmail.com>
- *
- */
-
-#ifndef SETUP_INIPARSEFINDVISITOR_H
-#define SETUP_INIPARSEFINDVISITOR_H
-
-#include "FindVisitor.h"
-
-
-/* parse passed in setup.ini files from disk. */
-class IniDBBuilder;
-class IniParseFeedback;
-/* IniParse files and create a package db when no cached .ini exists */
-class IniParseFindVisitor : public FindVisitor
-{
-public:
-  IniParseFindVisitor (IniDBBuilder &aBuilder,
-                       const std::string& localroot,
-                       IniParseFeedback &);
-  virtual void visitFile(const std::string& basePath, const WIN32_FIND_DATA *);
-  virtual ~ IniParseFindVisitor ();
-
-  unsigned int timeStamp() const;
-  std::string version() const;
-  int iniCount() const;
-protected:
-  IniParseFindVisitor (IniParseFindVisitor const &);
-  IniParseFindVisitor & operator= (IniParseFindVisitor const &);
-private:
-  IniDBBuilder &_Builder;
-  IniParseFeedback &_feedback;
-  unsigned int baseLength;
-  int local_ini;
-  unsigned int setup_timestamp;
-  std::string setup_version;
-};
-
-#endif /* SETUP_INIPARSEFINDVISITOR_H */
diff --git a/Makefile.am b/Makefile.am
index 88330d5..e9263cc 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -163,8 +163,6 @@ inilint_SOURCES = \
 	iniparse.yy \
 	IniParseFeedback.cc \
 	IniParseFeedback.h \
-	IniParseFindVisitor.cc \
-	IniParseFindVisitor.h \
 	install.cc \
 	io_stream.cc \
 	io_stream.h \
diff --git a/ini.cc b/ini.cc
index e90f98c..571ee2d 100644
--- a/ini.cc
+++ b/ini.cc
@@ -39,7 +39,6 @@
 #include "mount.h"
 #include "site.h"
 #include "find.h"
-#include "IniParseFindVisitor.h"
 #include "IniParseFeedback.h"
 
 #include "io_stream.h"
@@ -57,11 +56,12 @@ extern ThreeBarProgressPage Progress;
 
 unsigned int setup_timestamp = 0;
 std::string ini_setup_version;
-std::string current_ini_sig_name;
+IniList setup_ext_list ( setup_exts, setup_exts+3); // TODO: use C++11x initializer lists
 
 static BoolOption NoVerifyOption (false, 'X', "no-verify", "Don't verify setup.ini signatures");
 
 extern int yyparse ();
+
 /*extern int yydebug;*/
 
 class GuiParseFeedback : public IniParseFeedback
@@ -73,7 +73,7 @@ public:
       Progress.SetText3 ("");
       Progress.SetText4 ("Progress:");
     }
-  virtual void progress(unsigned long const pos, unsigned long const max)
+  virtual void progress (unsigned long const pos, unsigned long const max)
     {
       if (!max)
 	/* length not known or eof */
@@ -87,199 +87,247 @@ public:
 	  /* Log (LOG_BABBLE) << lastpct << "% (" << pos << " of " << max
 	    << " bytes of ini file read)" << endLog; */
 	}
-      Progress.SetBar1(pos, max);
+      Progress.SetBar1 (pos, max);
 
       static char buf[100];
-      sprintf(buf, "%d %%  (%ldk/%ldk)", lastpct, pos/1000, max/1000);
-      Progress.SetText3(buf);
+      sprintf (buf, "%d %%  (%ldk/%ldk)", lastpct, pos/1000, max/1000);
+      Progress.SetText3 (buf);
     }
   virtual void iniName (const std::string& name)
     {
       Progress.SetText1 ("Parsing...");
-      Progress.SetText2 (name.c_str());
+      Progress.SetText2 (name.c_str ());
       Progress.SetText3 ("");
     }
-  virtual void babble(const std::string& message)const
+  virtual void babble (const std::string& message)const
     {
       Log (LOG_BABBLE) << message << endLog;
     }
   virtual void warning (const std::string& message)const
     {
-      mbox (0, message.c_str(), "Warning", 0);
+      mbox (0, message.c_str (), "Warning", 0);
     }
-  virtual void error(const std::string& message)const
+  virtual void error (const std::string& message)const
     {
-      mbox (0, message.c_str(), "Parse Errors", 0);
+      mbox (0, message.c_str (), "Parse Errors", 0);
     }
   virtual ~ GuiParseFeedback ()
     {
-      Progress.SetText4("Package:");
+      Progress.SetText4 ("Package:");
     }
 private:
   unsigned int lastpct;
 };
 
-static int
-do_local_ini (HWND)
+static io_stream*
+decompress_ini (io_stream *ini_file)
 {
-  GuiParseFeedback myFeedback;
-  IniDBBuilderPackage findBuilder(myFeedback);
-  IniParseFindVisitor myVisitor (findBuilder, local_dir, myFeedback);
-  Find (local_dir).accept(myVisitor, 2);	// Only search two levels deep.
-  setup_timestamp = myVisitor.timeStamp();
-  ini_setup_version = myVisitor.version();
-  return myVisitor.iniCount();
+  // Replace the current compressed setup stream with its decompressed
+  // version.  Which decompressor to use is determined by file magic.
+  io_stream *compressed_stream = compress::decompress (ini_file);
+  if (!compressed_stream)
+    {
+      /* This isn't a known compression format or an uncompressed file
+	 stream.  Pass it on in case it was uncompressed, it will
+	 generate a parser error if it was some unknown format. */
+      delete compressed_stream;
+    }
+  else
+    {
+      /* Decompress the entire file in memory.  This has the advantage
+	 that input_stream->get_size () will work during parsing and
+	 we'll have an accurate status bar.  Also, we can't seek
+	 compressed streams, so when we write out a local cached copy
+	 of the .ini file below, we'd otherwise have to delete this
+	 stream and uncompress it again from the start, which is
+	 wasteful.  The current uncompressed size of the setup.ini
+	 file as of 2015 is about 5 MiB, so this is not a great deal
+	 of memory.  */
+      io_stream *uncompressed = new io_stream_memory ();
+      /* Note that the decompress io_stream now "owns" the underlying
+	 compressed io_stream instance, so it need not be deleted
+	 explicitly. */
+      if ((io_stream::copy (compressed_stream, uncompressed) != 0) ||
+	  (compressed_stream->error () != 0))
+	{
+	  /* There was a problem decompressing compressed_stream.  */
+	  Log (LOG_PLAIN) <<
+	    "Warning: Error code " << compressed_stream->error () <<
+	    " occurred while uncompressing " << current_ini_name <<
+	    " - possibly truncated or corrupt file. " << endLog;
+	  delete uncompressed;
+	  ini_file = NULL;
+	}
+      else
+	{
+	  ini_file = uncompressed;
+	  ini_file->seek (0, IO_SEEK_SET);
+	}
+    }
+  return ini_file;
 }
 
-static int
-do_remote_ini (HWND owner)
+static io_stream*
+check_ini_sig (io_stream* ini_file, io_stream* ini_sig_file,
+	       bool& sig_fail, const char* site, const char* sig_name, HWND owner)
 {
-  size_t ini_count = 0;
-  GuiParseFeedback myFeedback;
-  IniDBBuilderPackage aBuilder(myFeedback);
-  io_stream *ini_file, *ini_sig_file;
-
-  /* FIXME: Get rid of this io_stream pointer travesty.  The need to
-     explicitly delete these things is ridiculous.  Note that the
-     decompress io_stream "owns" the underlying compressed io_stream
-     instance, so it should not be deleted explicitly.  */
-
-  for (SiteList::const_iterator n = site_list.begin();
-       n != site_list.end(); ++n)
+  /* Unless the NoVerifyOption is set, check the signature for the
+     current setup and record the result.  On a failed signature check
+     the streams are invalidated so even if we tried to read in the
+     setup anyway there's be nothing to parse. */
+  if (!NoVerifyOption && ini_file)
     {
-      bool sig_fail = false;
-      /* First try to fetch the .bz2 compressed ini file.  */
-      current_ini_name = n->url + SETUP_INI_DIR + SETUP_BZ2_FILENAME;
-      current_ini_sig_name = n->url + SETUP_INI_DIR + SETUP_BZ2_FILENAME + ".sig";
-      ini_file = get_url_to_membuf (current_ini_name, owner);
-      if (!NoVerifyOption)
-	ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
-      if (!NoVerifyOption && ini_file && !ini_sig_file)
+      if (!ini_sig_file)
 	{
-	  note (owner, IDS_SETUPINI_MISSING, current_ini_sig_name.c_str(), n->url.c_str());
+	  note (owner, IDS_SETUPINI_MISSING, sig_name, site);
 	  delete ini_file;
 	  ini_file = NULL;
 	  sig_fail = true;
 	}
-      else if (!NoVerifyOption && ini_file && !verify_ini_file_sig (ini_file, ini_sig_file, owner))
+      else if (!verify_ini_file_sig (ini_file, ini_sig_file, owner))
 	{
-	  note (owner, IDS_SIG_INVALID, current_ini_sig_name.c_str(), n->url.c_str());
-	  delete ini_file;
-	  ini_file = NULL;
+	  note (owner, IDS_SIG_INVALID, sig_name, site);
 	  delete ini_sig_file;
 	  ini_sig_file = NULL;
+	  delete ini_file;
+	  ini_file = NULL;
 	  sig_fail = true;
 	}
+    }
+  return ini_file;
+}
+
+static int
+do_local_ini (HWND owner)
+{
+  size_t ini_count = 0;
+  GuiParseFeedback myFeedback;
+  IniDBBuilderPackage aBuilder (myFeedback);
+  io_stream *ini_file, *ini_sig_file;
+  // iterate over all setup files found in do_from_local_dir
+  for (IniList::const_iterator n = found_ini_list.begin ();
+       n != found_ini_list.end (); ++n)
+    {
+      bool sig_fail = false;
+      std::string current_ini_ext, current_ini_name, current_ini_sig_name;
+
+      current_ini_name = *n;
+      current_ini_sig_name = current_ini_name + ".sig";
+      current_ini_ext = current_ini_name.substr (current_ini_name.rfind (".") + 1);
+      ini_sig_file = io_stream::open ("file://" + current_ini_sig_name, "rb", 0);
+      ini_file = io_stream::open ("file://" + current_ini_name, "rb", 0);
+      ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
+				"localdir", current_ini_sig_name.c_str (), owner);
       if (ini_file)
+	ini_file = decompress_ini (ini_file);
+      if (!ini_file || sig_fail)
 	{
-	  /* Decompress the entire file in memory right now.  This has the
-	     advantage that input_stream->get_size() will work during parsing
-	     and we'll have an accurate status bar.  Also, we can't seek
-	     bz2 streams, so when it comes time to write out a local cached
-	     copy of the .ini file below, we'd otherwise have to delete this
-	     stream and uncompress it again from the start, which is wasteful.
-	     The current uncompresed size of the .ini file as of 2007 is less
-	     than 600 kB, so this is not a great deal of memory.  */
-	  io_stream *bz2_stream = compress::decompress (ini_file);
-	  if (!bz2_stream)
-	    {
-	      /* This isn't a valid bz2 file.  */
-	      delete ini_file;
-	      ini_file = NULL;
-	    }
+	  // no setup found or signature invalid
+	  note (owner, IDS_SETUPINI_MISSING, SetupBaseName.c_str (),
+		"localdir");
+	}
+      else
+	{
+	  // grok information from setup
+	  myFeedback.babble ("Found ini file - " + current_ini_name);
+	  myFeedback.iniName (current_ini_name);
+	  aBuilder.parse_mirror = "";
+	  ini_init (ini_file, &aBuilder, myFeedback);
+	  
+	  /*yydebug = 1; */
+
+	  if (yyparse () || yyerror_count > 0)
+	    myFeedback.error (yyerror_messages);
 	  else
-	    {
-	      io_stream *uncompressed = new io_stream_memory ();
+	    ++ini_count;
 
-	      if ((io_stream::copy (bz2_stream, uncompressed) != 0) ||
-                  (bz2_stream->error() != 0))
-		{
-		  /* There was a problem decompressing bz2.  */
-		  ini_file = NULL;
-		  Log (LOG_PLAIN) <<
-		    "Warning: Error code " << bz2_stream->error() << " occurred while uncompressing " <<
-		    current_ini_name << " - possibly truncated or corrupt bzip2"
-		    " file.  Retrying with uncompressed version." << endLog;
-		  delete bz2_stream;
-		  delete uncompressed;
-		}
-	      else
-		{
-		  delete bz2_stream;
-		  ini_file = uncompressed;
-		  ini_file->seek (0, IO_SEEK_SET);
-		}
+	  if (aBuilder.timestamp > setup_timestamp)
+	    {
+	      setup_timestamp = aBuilder.timestamp;
+	      ini_setup_version = aBuilder.version;
 	    }
+	  delete ini_file;
+	  ini_file = NULL;
 	}
+    }
+  return ini_count;
+}
 
-      if (!ini_file)
+static int
+do_remote_ini (HWND owner)
+{
+  size_t ini_count = 0;
+  GuiParseFeedback myFeedback;
+  IniDBBuilderPackage aBuilder (myFeedback);
+  io_stream *ini_file, *ini_sig_file;
+
+  /* FIXME: Get rid of this io_stream pointer travesty.  The need to
+     explicitly delete these things is ridiculous. */
+
+  // iterate over all sites
+  for (SiteList::const_iterator n = site_list.begin ();
+       n != site_list.end (); ++n)
+    {
+      bool sig_fail = false;
+      std::string current_ini_ext, current_ini_name, current_ini_sig_name;
+      // iterate over known extensions for setup
+      for (IniList::const_iterator ext = setup_ext_list.begin ();
+	   ext != setup_ext_list.end ();
+	   ext++)
 	{
-	  /* Try to look for a plain .ini file because one of the following
-	     happened above:
-	       - there was no .bz2 file found on the mirror.
-	       - the .bz2 file didn't look like a valid bzip2 file.
-	       - there was an error during bzip2 decompression.  */
-	  current_ini_name = n->url + SETUP_INI_DIR + SETUP_INI_FILENAME;
-	  current_ini_sig_name = n->url + SETUP_INI_DIR + SETUP_INI_FILENAME + ".sig";
+	  current_ini_ext = *ext;
+	  current_ini_name = n->url + SetupIniDir + SetupBaseName + "." + current_ini_ext;
+	  current_ini_sig_name = current_ini_name + ".sig";
+	  ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
 	  ini_file = get_url_to_membuf (current_ini_name, owner);
-	  if (!NoVerifyOption)
-	    ini_sig_file = get_url_to_membuf (current_ini_sig_name, owner);
-
-	  if (!NoVerifyOption && ini_file && !ini_sig_file)
-	    {
-	      note (owner, IDS_SETUPINI_MISSING, current_ini_sig_name.c_str(), n->url.c_str());
-	      delete ini_file;
-	      ini_file = NULL;
-	      sig_fail = true;
-	    }
-	  else if (!NoVerifyOption && ini_file && !verify_ini_file_sig (ini_file, ini_sig_file, owner))
-	    {
-	      note (owner, IDS_SIG_INVALID, current_ini_sig_name.c_str(), n->url.c_str());
-	      delete ini_file;
-	      ini_file = NULL;
-	      delete ini_sig_file;
-	      ini_sig_file = NULL;
-	      sig_fail = true;
-	    }
+	  ini_file = check_ini_sig (ini_file, ini_sig_file, sig_fail,
+				    n->url.c_str (), current_ini_sig_name.c_str (), owner);
+	  // stop searching as soon as we find a setup file
+	  if (ini_file)
+	    break;
 	}
-
-      if (!ini_file)
+      if (ini_file)
+	ini_file = decompress_ini (ini_file);
+      if (!ini_file || sig_fail)
 	{
-	  if (!sig_fail)
-	    note (owner, IDS_SETUPINI_MISSING, SETUP_INI_FILENAME, n->url.c_str());
-	  continue;
+	  // no setup found or signature invalid
+	  note (owner, IDS_SETUPINI_MISSING, SetupBaseName.c_str (), n->url.c_str ());
 	}
-
-      myFeedback.iniName (current_ini_name);
-      aBuilder.parse_mirror = n->url;
-      ini_init (ini_file, &aBuilder, myFeedback);
-
-      /*yydebug = 1; */
-
-      if (yyparse () || yyerror_count > 0)
-	myFeedback.error (yyerror_messages);
       else
 	{
-	  /* save known-good setup.ini locally */
-	  const std::string fp = "file://" + local_dir + "/" +
-				   rfc1738_escape_part (n->url) +
-				   "/" + SETUP_INI_DIR + SETUP_INI_FILENAME;
-	  io_stream::mkpath_p (PATH_TO_FILE, fp, 0);
-	  if (io_stream *out = io_stream::open (fp, "wb", 0))
+	  // grok information from setup
+	  myFeedback.iniName (current_ini_name);
+	  aBuilder.parse_mirror = n->url;
+	  ini_init (ini_file, &aBuilder, myFeedback);
+	  
+	  /*yydebug = 1; */
+
+	  if (yyparse () || yyerror_count > 0)
+	    myFeedback.error (yyerror_messages);
+	  else
 	    {
-	      ini_file->seek (0, IO_SEEK_SET);
-	      if (io_stream::copy (ini_file, out) != 0)
-		io_stream::remove (fp);
-	      delete out;
+	      /* save known-good setup.ini locally */
+	      const std::string fp = "file://" + local_dir + "/" +
+				      rfc1738_escape_part (n->url) +
+				      "/" + SetupIniDir + SetupBaseName + ".ini";
+	      io_stream::mkpath_p (PATH_TO_FILE, fp, 0);
+	      if (io_stream *out = io_stream::open (fp, "wb", 0))
+		{
+		  ini_file->seek (0, IO_SEEK_SET);
+		  if (io_stream::copy (ini_file, out) != 0)
+		    io_stream::remove (fp);
+		  delete out;
+		}
+	      ++ini_count;
 	    }
-	  ++ini_count;
-	}
-      if (aBuilder.timestamp > setup_timestamp)
-	{
-	  setup_timestamp = aBuilder.timestamp;
-	  ini_setup_version = aBuilder.version;
+	  if (aBuilder.timestamp > setup_timestamp)
+	    {
+	      setup_timestamp = aBuilder.timestamp;
+	      ini_setup_version = aBuilder.version;
+	    }
+	  delete ini_file;
+	  ini_file = NULL;
 	}
-      delete ini_file;
     }
   return ini_count;
 }
@@ -296,7 +344,7 @@ do_ini_thread (HINSTANCE h, HWND owner)
   if (ini_count == 0)
     return false;
 
-  if (get_root_dir ().c_str())
+  if (get_root_dir ().c_str ())
     {
       io_stream::mkpath_p (PATH_TO_DIR, "cygfile:///etc/setup", 0755);
 
@@ -332,7 +380,7 @@ do_ini_thread (HINSTANCE h, HWND owner)
 	}
     }
 
-  LogBabblePrintf (".ini setup_version is %s, our setup_version is %s", ini_setup_version.size() ?
+  LogBabblePrintf (".ini setup_version is %s, our setup_version is %s", ini_setup_version.size () ?
        ini_setup_version.c_str () : "(null)",
        setup_version);
   if (ini_setup_version.size ())
@@ -346,21 +394,21 @@ do_ini_thread (HINSTANCE h, HWND owner)
 }
 
 static DWORD WINAPI
-do_ini_thread_reflector(void* p)
+do_ini_thread_reflector (void* p)
 {
   HANDLE *context;
   context = (HANDLE*)p;
 
   try
   {
-    bool succeeded = do_ini_thread((HINSTANCE)context[0], (HWND)context[1]);
+    bool succeeded = do_ini_thread ((HINSTANCE)context[0], (HWND)context[1]);
 
     // Tell the progress page that we're done downloading
-    Progress.PostMessageNow(WM_APP_SETUP_INI_DOWNLOAD_COMPLETE, 0, succeeded);
+    Progress.PostMessageNow (WM_APP_SETUP_INI_DOWNLOAD_COMPLETE, 0, succeeded);
   }
-  TOPLEVEL_CATCH("ini");
+  TOPLEVEL_CATCH ("ini");
 
-  ExitThread(0);
+  ExitThread (0);
 }
 
 static HANDLE context[2];
@@ -374,4 +422,3 @@ do_ini (HINSTANCE h, HWND owner)
   DWORD threadID;
   CreateThread (NULL, 0, do_ini_thread_reflector, context, 0, &threadID);
 }
-
diff --git a/ini.h b/ini.h
index 1969da9..164e3d2 100644
--- a/ini.h
+++ b/ini.h
@@ -48,10 +48,6 @@ typedef enum
   EXCLUDE_NOT_FOUND
 } excludes;
 
-#define SETUP_INI_DIR	   (SetupIniDir.c_str ())
-#define SETUP_INI_FILENAME ((SetupBaseName+".ini").c_str ())
-#define SETUP_BZ2_FILENAME ((SetupBaseName+".bz2").c_str ())
-
 /* The following three vars are used to facilitate error handling between the
    parser/lexer and its callers, namely ini.cc:do_remote_ini() and
    IniParseFindVisitor::visitFile().  */
-- 
2.4.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-fromcwd.cc-Remove-debugging-output.patch --]
[-- Type: text/x-patch, Size: 3173 bytes --]

From 7788675cb2cfc547de7ead3ff5ae9839ae531506 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Sat, 11 Jul 2015 23:24:11 +0200
Subject: [PATCH 4/6] fromcwd.cc: Remove debugging output

---
 fromcwd.cc | 45 ---------------------------------------------
 1 file changed, 45 deletions(-)

diff --git a/fromcwd.cc b/fromcwd.cc
index edb67a6..d571b02 100644
--- a/fromcwd.cc
+++ b/fromcwd.cc
@@ -34,13 +34,6 @@
 #include "IniDBBuilderPackage.h"
 #include "IniParseFeedback.h"
 
-#define DEBUG_FROMCWD
-#ifdef DEBUG_FROMCWD
-#include "LogFile.h"
-#endif
-
-/* Trivial class for detecting the existence of setup.ini */
-
 class SetupFindVisitor : public FindVisitor
 {
 public:
@@ -52,57 +45,27 @@ public:
   virtual void visitFile (const std::string& basePath,
 			  const WIN32_FIND_DATA *theFile)
   {
-#ifdef DEBUG_FROMCWD
-    Log (LOG_PLAIN) << "examining file: "
-		    << basePath << "./" << theFile->cFileName << endLog;
-#endif
     if (inidir &&
 	(theFile->nFileSizeLow || theFile->nFileSizeHigh))
       {
-#ifdef DEBUG_FROMCWD
-	Log (LOG_PLAIN) << "checking extension: ";
-#endif
 	std::vector<bool>::iterator fi = found_ini.begin ();
 	for (std::vector<std::string>::const_iterator ext = setup_ext_list.begin ();
 	     ext != setup_ext_list.end ();
 	     ext++, fi++)
 	  {
-#ifdef DEBUG_FROMCWD
-	    Log (LOG_PLAIN) << *ext << " ";
-#endif
 	    if (!casecompare (SetupBaseName + "." + *ext,  theFile->cFileName))
 	      *fi = true;
 	  }
-#ifdef DEBUG_FROMCWD
-	Log (LOG_PLAIN) << endLog;
-#endif
       }
-#ifdef DEBUG_FROMCWD
-    Log (LOG_PLAIN) << "state: "
-		    << " inidir="    << inidir
-		    << " found_xz="  << found_ini[0]
-		    << " found_bz2=" << found_ini[1]
-		    << " found_ini=" << found_ini[2]
-		    << endLog;
-#endif
   }
   virtual void visitDirectory (const std::string& basePath,
 			       WIN32_FIND_DATA const *aDir, int level)
   {
-#ifdef DEBUG_FROMCWD
-    Log (LOG_PLAIN) << "examining directory: "
-		    << basePath << "./" << aDir->cFileName
-		    << endLog;
-#endif
     if (level <= 0)
       return;
     inidir = !casecompare (SetupArch, aDir->cFileName);
     if (level == 1 && !inidir)
       return;
-#ifdef DEBUG_FROMCWD
-    Log (LOG_PLAIN) << "  recurse into: "
-		    << (inidir ? "inidir" : "mirror") << endLog;
-#endif
     Find aFinder (basePath + aDir->cFileName);
     aFinder.accept (*this, inidir ? 0 : --level);
 	std::vector<bool>::const_iterator fi = found_ini.begin ();
@@ -134,10 +97,6 @@ IniList found_ini_list;
 bool
 do_from_local_dir (HINSTANCE h, HWND owner, std::string &local_dir)
 {
-#ifdef DEBUG_FROMCWD
-  Log (LOG_PLAIN) << "do_from_local_dir: "
-		  << local_dir << endLog;
-#endif
   // Assume we won't find the INI file.
   SetupFindVisitor found;
   // single mirror?
@@ -149,10 +108,6 @@ do_from_local_dir (HINSTANCE h, HWND owner, std::string &local_dir)
   if (found)
       return true;
   // nope, do full scan.
-#ifdef DEBUG_FROMCWD
-  Log (LOG_PLAIN) << "  starting full scan from "
-		  << local_dir << endLog;
-#endif
   IniParseFeedback myFeedback;
   IniDBBuilderPackage myBuilder (myFeedback);
   ScanFindVisitor myVisitor (myBuilder);
-- 
2.4.5


[-- Attachment #6: Type: text/plain, Size: 202 bytes --]



Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

Hi Achim,

looks good to me.  Please apply with two minor changes:

On Jul 11 23:31, Achim Gratz wrote:
> [...]
> diff --git a/ini.cc b/ini.cc
> index e90f98c..571ee2d 100644
> --- a/ini.cc
> +++ b/ini.cc
> @@ -39,7 +39,6 @@
>  #include "mount.h"
>  #include "site.h"
>  #include "find.h"
> -#include "IniParseFindVisitor.h"
>  #include "IniParseFeedback.h"
>  
>  #include "io_stream.h"
> @@ -57,11 +56,12 @@ extern ThreeBarProgressPage Progress;
>  
>  unsigned int setup_timestamp = 0;
>  std::string ini_setup_version;
> -std::string current_ini_sig_name;
> +IniList setup_ext_list ( setup_exts, setup_exts+3); // TODO: use C++11x initializer lists

Can the constant 3 go away (sizeof setup_exts / sizeof *setup_exts)?

> [...]
> +	  myFeedback.iniName (current_ini_name);
> +	  aBuilder.parse_mirror = "";
> +	  ini_init (ini_file, &aBuilder, myFeedback);
> +	  
   ^^^^^^^
   Extra whitespaces, `git am' complains about, in the line preceeding
   the "yydebug = 1" comments in both, do_local_ini and do_remote_ini.

> +	  /*yydebug = 1; */
> +
> [...]
> -
> -      /*yydebug = 1; */
> -

Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> looks good to me.  Please apply with two minor changes:

Pushed.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: setup

[Warren Young]

On Aug 11, 2015, at 1:47 AM, Corinna Vinschen wrote:
> 
> On Aug 10 09:02, Warren Young wrote:
> 
>> If you mean that it always copies itself to some kind of temp space
>> and execs it from there, that feels kind of wasteful
> 
> ...the restart part is done already anyway as soon as
> setup elevates itself.

Sold. :)

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

On Aug 10 09:02, Warren Young wrote:
> > On Aug  7 15:05, Warren Young wrote:
> >> 
> >> You’d have to couple this either with a MoveFileEx(…,
> >> MOVEFILE_DELAY_UNTIL_REBOOT) call or a background task that replaces
> >> /bin/setup.exe with %LOCALAPPDATA%/Temp/setup-v$next.exe.
> > 
> > Why?
> 
> I was assuming a world where setup.exe lives in /bin and you can call
> it from Bash.  (In such a world, I hope it gets renamed to something
> less generic, like cygpkg.)  I was further assuming that
> setup-*.tar.xz would be packaged the same as any other Cygwin package,
> unpacking directly into /bin.

That was the idea.

> Thus, the MoveFileEx() call would be the one setup.exe normally
> offers, where it sees you’re trying to replace an in-use executable.
> 
> I was offering the background task alternative as a way around that.
> However, the rest of your reply gives me a different idea.
> 
> > The idea is that setup is called, performs a CopyFile of itself and
> > then exec's its copy.
> 
> If you mean that it always copies itself to some kind of temp space
> and execs it from there, that feels kind of wasteful, since you’re
> trading my design that copies only when required for one that copies
> every time.

Setup is not called multiple times per second.  Copy and restart take
not much time, and the restart part is done already anyway as soon as
setup elevates itself.  Why not simply elevate (or just start if already
elevated) the copy called .setup-${arch}.exe?  Note the leading dot.
This copy never has the problem to be overwritten by unpacking, and the
rest of setup does not need any changes.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Warren Young]

On Aug 10, 2015, at 12:03 PM, Achim Gratz wrote:
> 
>> There have been a bunch of attempts in the past at replacing
>> setup.exe. At least 3, that I can think of.  All have fizzled.
> 
> These were?

The Debian and Red Hat packaging systems have both been ported to Cygwin.  Those could be used along with a bare-bones setup.exe to bootstrap Cygwin, after which setup.exe would no longer be needed.

  https://github.com/transcode-open/apt-cyg
  https://cygwin.com/ml/cygwin-announce/2003-05/msg00001.html

Then there was a pretty GUI installer someone made many years ago; I believe we were still in the 1.5.x line at the time.  I don’t remember enough about it to find it via Google again, but I do remember a few posts to the mailing list with positive responses.  Then the developer disappeared and no one took up the code base.

That’s part of what I mean about the difficulty of replacing key infrastructure.  Without either a change at the core or an overwhelming attack from outside, there just isn’t enough reason for someone to try to adopt something nonstandard.  Without that user base, there isn’t enough drive to continue development, so the project fizzles.

Consider the rise of Ubuntu-based Linuxes, replacing the various the RPM-based ones.  That didn’t happen purely because Ubuntu was “better.”  A necessary part of this was Shuttleworth pouring millions of dollars from a really lucky IPO into the project.

As proof, consider all of the Ubuntu clones that have gone nowhere, despite being “better” in some way.

The closest thing I can think of to success in the area you propose to tackle is mintty, which stepped into a gap between Windows Console and Cygwin/X + rxvt.  It didn’t try to replace either one, exactly, so it didn’t have to succeed by first making the other disappear.  “Better and separate” beats “better.”

Re: setup

[Achim Gratz]

Warren Young writes:
> Replacing core infrastructure like this almost never works out
> smoothly, and usually fails outright.  It’s far better to remediate
> the code base in-place, if at all possible.

That's my sentiment as well, but that doesn't preclude the exploration
of alternatives.

> There have been a bunch of attempts in the past at replacing
> setup.exe. At least 3, that I can think of.  All have fizzled.

These were?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Warren Young]

On Aug 10, 2015, at 11:00 AM, Achim Gratz wrote:
> 
> Warren Young writes:
>> Isn’t the whole point of this discussion that setup.exe already knows
>> all the tricks it needs to in order to do what we want here, except
>> for the “replace setup.exe in place” issue I’ve brought up separately?
> 
> Well, setup.exe today has a lot of probably bitrotted features that
> Cygwin never used (even if some of it looks quite interesting and
> useful)

I don’t think there is any call for setup.exe to be highly backwards-compatible.  It isn’t asked to deal with old package repos, very old versions of setup.ini, etc.  Therefore, I would say that whoever is in charge of this code base should simply get rid of the bitrotted features.  

If any of these obsolete features are needed again in the future, they can be pulled out of the SCM history.

> there are other things it doesn't do even though it should
> probably be doing it.

Okay, but...

>> Why do you need a self-contained POSIX environment to replace
>> setup.exe?
> 
> At the moment I'm just thinking whether that might be a more sustainable
> way forward.

Replacing core infrastructure like this almost never works out smoothly, and usually fails outright.  It’s far better to remediate the code base in-place, if at all possible.

There have been a bunch of attempts in the past at replacing setup.exe. At least 3, that I can think of.  All have fizzled.

Re: setup

[Achim Gratz]

Warren Young writes:
> Isn’t the whole point of this discussion that setup.exe already knows
> all the tricks it needs to in order to do what we want here, except
> for the “replace setup.exe in place” issue I’ve brought up separately?

Well, setup.exe today has a lot of probably bitrotted features that
Cygwin never used (even if some of it looks quite interesting and
useful) and there are other things it doesn't do even though it should
probably be doing it.

> Why do you need a self-contained POSIX environment to replace
> setup.exe?

At the moment I'm just thinking whether that might be a more sustainable
way forward.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: setup

[Warren Young]

On Aug 10, 2015, at 10:40 AM, Achim Gratz wrote:
> 
> Warren Young writes:
> 
>> In that sense, setup.exe is already bootstrapped: it has everything
>> within itself to be able to replace itself.
> 
> I do like the concept of a self-contained executable, but teaching
> setup.exe new tricks is likely a larger effort than adding a script here
> and there to an install system.

Isn’t the whole point of this discussion that setup.exe already knows all the tricks it needs to in order to do what we want here, except for the “replace setup.exe in place” issue I’ve brought up separately?

Why do you need a self-contained POSIX environment to replace setup.exe?

Re: setup

[Achim Gratz]

Warren Young writes:
> But that’s a side issue, because I don’t see why you think you need
> Busybox in the first place, unless it is to be able to run postinst
> scripts.  Can’t you just say that that setup-*.tar.xz package never
> has a postinst script, so that all you need to do is un-tar it?

The idea was that you have a fairly complete POSIX environment with as
small a footprint as possible, then use that to bootstrap a more
conventionally styled Cygwin install.

> In that sense, setup.exe is already bootstrapped: it has everything
> within itself to be able to replace itself.

I do like the concept of a self-contained executable, but teaching
setup.exe new tricks is likely a larger effort than adding a script here
and there to an install system.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Link?

I'd think Jon needs to give his OK for me to post it or he should post
it himself.

> I'm using setup with the -m option exclusively so I'm mostly
> interested that this still works.

That's pretty extensively tested by now since that is what I use for the
installations at work.

> However, you might want to make a quick list what scenarios you want
> to have tested as well.  I try to do that over the next couple of days
> and hopfully other maintainers try, too.

Installs from the internet (with and without proxy) and ad-hoc installs
of single packages and package groups from a local directory that
doesn't have a setup.ini.

> IIUC, you mean that the current method of downloading setup only from
> sware should be maintained?  What kind of "key" are you talking about?

Currently the integrity of setup.exe and therefore the whole
installation hinges on it being downloaded from sware via https, so
that's a strong reason to keep it that way.

The "key" I'm talking about was that in addition to "setup-version:" we
could have an additional "setup-trial-version" or something like that
would inform setup that a successor is waiting in the wings.

> That's a general problem.  I'm not sure how much exposure the Cygwin
> test DLLs really have, either.  I'm reasonable sure that some of you
> maintainers are using them, but otherwise it's a bit of a black hole.

You should at least be able to find out how often they were downloaded.
For setup trial versions this isn't enough, so they'd have to contact
sourceware and HEAD/GET something that indicated success or failure (and
some script to extract that out of the logs on sourceware).  It also
must not be sneaky about this, ideally the user consents before the
trial version gets downloaded at all.

> Not sure I follow.  How would such an install system look like?
> I have a vague notion that busybox could run the scripts, but there
> are installation path issues and requirements of some postinstall
> scripts which might not be suffiecently handled by busybox.

I have done a few manual installations from a running Cygwin onto a
"cold" installation on the same machine, mainly to switch out the Cygwin
DLL.  You need to mount the "other" Cygwin somewhere (I use /mnt/cygwin
or something like that) and then redirect the install root there (tar -C
/mnt/cygwin).  The postinstall scripts on the other hand must be started
from within the new Cygwin installation anyway.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: setup

[Warren Young]

> On Aug  7 15:05, Warren Young wrote:
>> 
>> You’d have to couple this either with a MoveFileEx(…,
>> MOVEFILE_DELAY_UNTIL_REBOOT) call or a background task that replaces
>> /bin/setup.exe with %LOCALAPPDATA%/Temp/setup-v$next.exe.
> 
> Why?

I was assuming a world where setup.exe lives in /bin and you can call it from Bash.  (In such a world, I hope it gets renamed to something less generic, like cygpkg.)  I was further assuming that setup-*.tar.xz would be packaged the same as any other Cygwin package, unpacking directly into /bin.

Thus, the MoveFileEx() call would be the one setup.exe normally offers, where it sees you’re trying to replace an in-use executable.

I was offering the background task alternative as a way around that.  However, the rest of your reply gives me a different idea.

> The idea is that setup is called, performs a CopyFile of itself
> and then exec's its copy.

If you mean that it always copies itself to some kind of temp space and execs it from there, that feels kind of wasteful, since you’re trading my design that copies only when required for one that copies every time.

But what if we invert it?  Why couldn't setup-*.tar.xz package unpack into /tmp?  The running setup.exe would exec it there with a flag that causes the second setup.exe instance to copy itself back into /bin.  Then there is only a copy made when setup.exe is actually being replaced, and /bin/setup.exe is kept up-to-date.

Re: setup

[Warren Young]

On Aug 7, 2015, at 11:22 PM, Achim Gratz <Stromeko@nexgo.de> wrote:
> 
> a minimal Cygwin install system w/ busybox

Busybox isn’t going to be usable in its normal sense without cygwin1.dll to map symlinks to argv[0] so that Busybox can tell how it was called, and thus which function to provide.

But that’s a side issue, because I don’t see why you think you need Busybox in the first place, unless it is to be able to run postinst scripts.  Can’t you just say that that setup-*.tar.xz package never has a postinst script, so that all you need to do is un-tar it?

In that sense, setup.exe is already bootstrapped: it has everything within itself to be able to replace itself.

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

On Aug  8 07:22, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> I would consider this a release candidate.  Some more testing with
> >> interactive and ad-hoc installs would be useful, though.
> >
> > How do you want to handle this?  We don't have real provisions for
> > setup.exe release candidates.
> 
> True.  At the moment it'd probably mean that some other folks on this
> list try it and give their GTG.  They'd either have to build it
> themselves or maybe use the AppVeyor build from Jon Turney.

Link?  I'm using setup with the -m option exclusively so I'm mostly
interested that this still works.  However, you might want to make a
quick list what scenarios you want to have tested as well.  I try to
do that over the next couple of days and hopfully other maintainers
try, too.

> > Maybe we should change how we provide setup updates in future?
> >
> > I have a vague idea that setup should ideally be part of the Cygwin
> > distro package set.  So setup updates itself, and it would be possible
> > to handle public "test" releases.
> 
> We could add a new key to setup.ini that indicates the existence of a
> test version and have setup ask if the user would maybe want to use that
> instead.  Ideally setup would then download and exec it if the user
> choses to run it.

IIUC, you mean that the current method of downloading setup only from
sware should be maintained?  What kind of "key" are you talking about?

> I don't have code ready to do that, though…  Another
> problem that needs to be solved is to know how much exposure the new
> setup.exe really had to decide if it's good for release.

That's a general problem.  I'm not sure how much exposure the Cygwin
test DLLs really have, either.  I'm reasonable sure that some of you
maintainers are using them, but otherwise it's a bit of a black hole.

> > The issue of overwriting setup while setup is running could be worked
> > around by setup first copying itself to a intermediate filename (e.g.
> > .setup.exe) and then exce'ing that copy.
> >
> > Other ideas how we could handle this?
> 
> As said before, the idea that setup.exe needs to be entirely
> self-contained is both an advantage and limiting what it can do.  I
> haven't had time yet to check, but a minimal Cygwin install system w/
> busybox might not be too large compared to the connectivity demands of
> todays' Windows itself.  Here setup.exe would just need to unpack the
> current image of the install system and provide the GUI to pick the
> packages and control the actual installation.

Not sure I follow.  How would such an install system look like?
I have a vague notion that busybox could run the scripts, but there
are installation path issues and requirements of some postinstall
scripts which might not be suffiecently handled by busybox.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Aug  7 15:05, Warren Young wrote:
> On Aug 7, 2015, at 1:47 PM, Corinna Vinschen <corinna-cygwin@cygwin.com> wrote:
> > 
> > On Aug  6 17:57, Achim Gratz wrote:
> >> 
> >> I would consider this a release candidate.  Some more testing with
> >> interactive and ad-hoc installs would be useful, though.
> > 
> > I have a vague idea that setup should ideally be part of the Cygwin
> > distro package set.  So setup updates itself, and it would be possible
> > to handle public "test" releases.
> > 
> > The issue of overwriting setup while setup is running could be worked
> > around by setup first copying itself to a intermediate filename (e.g.
> > .setup.exe) and then exce'ing that copy.
> 
> You’d have to couple this either with a MoveFileEx(…,
> MOVEFILE_DELAY_UNTIL_REBOOT) call or a background task that replaces
> /bin/setup.exe with %LOCALAPPDATA%/Temp/setup-v$next.exe.

Why?  The idea is that setup is called, performs a CopyFile of itself
and then exec's its copy.  If the copyfile fails with a sharing
violation or something like that, the copy is in use, which means, setup
is already running.  However, if the copy already exists and can be
overwritten. it's just some older copy.  I don't think there's a
necessity to remove the copy at all.

> This would be a nice point to give setup.exe a CLI language for
> installing packages in yum/apt-get fashion, too.  (Yes, yes, I know,
> SHTDI.)

I like the idea in genral, but I'd also like to write SHTDI in all
uppercase.  Well, it already is, but you get my meaning...


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> I would consider this a release candidate.  Some more testing with
>> interactive and ad-hoc installs would be useful, though.
>
> How do you want to handle this?  We don't have real provisions for
> setup.exe release candidates.

True.  At the moment it'd probably mean that some other folks on this
list try it and give their GTG.  They'd either have to build it
themselves or maybe use the AppVeyor build from Jon Turney.

> Maybe we should change how we provide setup updates in future?
>
> I have a vague idea that setup should ideally be part of the Cygwin
> distro package set.  So setup updates itself, and it would be possible
> to handle public "test" releases.

We could add a new key to setup.ini that indicates the existence of a
test version and have setup ask if the user would maybe want to use that
instead.  Ideally setup would then download and exec it if the user
choses to run it.  I don't have code ready to do that, though…  Another
problem that needs to be solved is to know how much exposure the new
setup.exe really had to decide if it's good for release.

> The issue of overwriting setup while setup is running could be worked
> around by setup first copying itself to a intermediate filename (e.g.
> .setup.exe) and then exce'ing that copy.
>
> Other ideas how we could handle this?

As said before, the idea that setup.exe needs to be entirely
self-contained is both an advantage and limiting what it can do.  I
haven't had time yet to check, but a minimal Cygwin install system w/
busybox might not be too large compared to the connectivity demands of
todays' Windows itself.  Here setup.exe would just need to unpack the
current image of the install system and provide the GUI to pick the
packages and control the actual installation.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Warren Young]

On Aug 7, 2015, at 1:47 PM, Corinna Vinschen <corinna-cygwin@cygwin.com> wrote:
> 
> On Aug  6 17:57, Achim Gratz wrote:
>> 
>> I would consider this a release candidate.  Some more testing with
>> interactive and ad-hoc installs would be useful, though.
> 
> I have a vague idea that setup should ideally be part of the Cygwin
> distro package set.  So setup updates itself, and it would be possible
> to handle public "test" releases.
> 
> The issue of overwriting setup while setup is running could be worked
> around by setup first copying itself to a intermediate filename (e.g.
> .setup.exe) and then exce'ing that copy.

You’d have to couple this either with a MoveFileEx(…, MOVEFILE_DELAY_UNTIL_REBOOT) call or a background task that replaces /bin/setup.exe with %LOCALAPPDATA%/Temp/setup-v$next.exe.

This would be a nice point to give setup.exe a CLI language for installing packages in yum/apt-get fashion, too.  (Yes, yes, I know, SHTDI.)

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Aug  6 17:57, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> OK for push?
> >
> > Yes, thank you.
> 
> Done.
> 
> I would consider this a release candidate.  Some more testing with
> interactive and ad-hoc installs would be useful, though.

How do you want to handle this?  We don't have real provisions for
setup.exe release candidates.

Maybe we should change how we provide setup updates in future?

I have a vague idea that setup should ideally be part of the Cygwin
distro package set.  So setup updates itself, and it would be possible
to handle public "test" releases.

The issue of overwriting setup while setup is running could be worked
around by setup first copying itself to a intermediate filename (e.g.
.setup.exe) and then exce'ing that copy.

Other ideas how we could handle this?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> OK for push?
>
> Yes, thank you.

Done.

I would consider this a release candidate.  Some more testing with
interactive and ad-hoc installs would be useful, though.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

On Aug  5 17:41, Achim Gratz wrote:
> Corinna Vinschen writes:
> > Same here.  Am I missing something?
> 
> Nothing, but I missed to copy the negation from the corresponding code
> for MD5 even though the description of the patch explicitly makes note
> of the correct logic.  It's fixed already in my repo since I finally
> updated all of Cygwin at work today.
> 
> --8<---------------cut here---------------start------------->8---
> 	Modified   IniDBBuilderPackage.cc
> diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
> index b41955a..ff92ec2 100644
> --- a/IniDBBuilderPackage.cc
> +++ b/IniDBBuilderPackage.cc
> @@ -268,7 +268,7 @@ IniDBBuilderPackage::buildInstallSize (const std::string &size)
>  void
>  IniDBBuilderPackage::buildInstallSHA512 (unsigned char const *sha512)
>  {
> -  if (sha512 && cbpv.source()->sha512_isSet) {
> +  if (sha512 && !cbpv.source()->sha512_isSet) {
>      memcpy (cbpv.source()->sha512sum, sha512, sizeof cbpv.source()->sha512sum);
>      cbpv.source()->sha512_isSet = true;
>    }
> @@ -277,7 +277,7 @@ IniDBBuilderPackage::buildInstallSHA512 (unsigned char const *sha512)
>  void
>  IniDBBuilderPackage::buildSourceSHA512 (unsigned char const *sha512)
>  {
> -  if (sha512 && cbpv.source()->sha512_isSet) {
> +  if (sha512 && !cbpv.source()->sha512_isSet) {
>      memcpy (cspv.source()->sha512sum, sha512, sizeof cspv.source()->sha512sum);
>      cbpv.source()->sha512_isSet = true;
>    }
> --8<---------------cut here---------------end--------------->8---
> 
> OK for push?

Yes, thank you.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Same here.  Am I missing something?

Nothing, but I missed to copy the negation from the corresponding code
for MD5 even though the description of the patch explicitly makes note
of the correct logic.  It's fixed already in my repo since I finally
updated all of Cygwin at work today.

--8<---------------cut here---------------start------------->8---
	Modified   IniDBBuilderPackage.cc
diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index b41955a..ff92ec2 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -268,7 +268,7 @@ IniDBBuilderPackage::buildInstallSize (const std::string &size)
 void
 IniDBBuilderPackage::buildInstallSHA512 (unsigned char const *sha512)
 {
-  if (sha512 && cbpv.source()->sha512_isSet) {
+  if (sha512 && !cbpv.source()->sha512_isSet) {
     memcpy (cbpv.source()->sha512sum, sha512, sizeof cbpv.source()->sha512sum);
     cbpv.source()->sha512_isSet = true;
   }
@@ -277,7 +277,7 @@ IniDBBuilderPackage::buildInstallSHA512 (unsigned char const *sha512)
 void
 IniDBBuilderPackage::buildSourceSHA512 (unsigned char const *sha512)
 {
-  if (sha512 && cbpv.source()->sha512_isSet) {
+  if (sha512 && !cbpv.source()->sha512_isSet) {
     memcpy (cspv.source()->sha512sum, sha512, sizeof cspv.source()->sha512sum);
     cbpv.source()->sha512_isSet = true;
   }
--8<---------------cut here---------------end--------------->8---

OK for push?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On Aug  4 20:34, Achim Gratz wrote:
> Achim Gratz writes:
> > Pushed.
> 
> I've found another (and hopefully the last, except for the mostly
> dormant gzip code) 16kiB buffer to be increased to 64kiB.  Also, when
> installing from a local directory without a setup.ini file there
> actually is a need to skip the checksum validation.  For this to work we
> also need to record whether we actually set a checksum and testing the
> first byte for zero is not correct for doing this.  As detailed in the
> design for Bitcoin, there is an exponetnially decreasing probability for
> the leading bits to all be zero.  Current Cygwin has more than 80
> packages with the first eight bits or more all zero (I've seen 15
> leading zero bits in my local mirror).
> 

> >From 9de3a769ae38b1e54ec99e5f105f832a74becfab Mon Sep 17 00:00:00 2001
> From: Achim Gratz <...>
> Date: Tue, 4 Aug 2015 19:39:23 +0200
> Subject: [PATCH 1/2] Increase read buffer size for MD5 checksumming to 64kiB
> 
> 	* install.cc (md5_one): Change buffer size from 16kiB to 64kiB for
> 	faster reading.

Acked.

> >From d2f2501580391154fbd5331a932271ce17880680 Mon Sep 17 00:00:00 2001
> From: Achim Gratz <...>
> Date: Tue, 4 Aug 2015 19:48:56 +0200
> Subject: [PATCH 2/2] Properly record SHA512 checksum presence and skip
>  validation for ad-hoc installs
> 
> diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
> index 7ee2af4..b41955a 100644
> --- a/IniDBBuilderPackage.cc
> +++ b/IniDBBuilderPackage.cc
> @@ -268,15 +268,19 @@ IniDBBuilderPackage::buildInstallSize (const std::string &size)
>  void
>  IniDBBuilderPackage::buildInstallSHA512 (unsigned char const *sha512)
>  {
> -  if (sha512)
> +  if (sha512 && cbpv.source()->sha512_isSet) {
>      memcpy (cbpv.source()->sha512sum, sha512, sizeof cbpv.source()->sha512sum);
> +    cbpv.source()->sha512_isSet = true;
> +  }

This looks wrong.

     if (...&& cbpv.source()->sha512_isSet)
       ...
       cbpv.source()->sha512_isSet = true;

If the flag is set you set it again?

>  }
>  
>  void
>  IniDBBuilderPackage::buildSourceSHA512 (unsigned char const *sha512)
>  {
> -  if (sha512)
> +  if (sha512 && cbpv.source()->sha512_isSet) {
>      memcpy (cspv.source()->sha512sum, sha512, sizeof cspv.source()->sha512sum);
> +    cbpv.source()->sha512_isSet = true;
> +  }
>  }

Same here.  Am I missing something?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

Achim Gratz writes:
> Pushed.

I've found another (and hopefully the last, except for the mostly
dormant gzip code) 16kiB buffer to be increased to 64kiB.  Also, when
installing from a local directory without a setup.ini file there
actually is a need to skip the checksum validation.  For this to work we
also need to record whether we actually set a checksum and testing the
first byte for zero is not correct for doing this.  As detailed in the
design for Bitcoin, there is an exponetnially decreasing probability for
the leading bits to all be zero.  Current Cygwin has more than 80
packages with the first eight bits or more all zero (I've seen 15
leading zero bits in my local mirror).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Increase-read-buffer-size-for-MD5-checksumming-to-64.patch --]
[-- Type: text/x-patch, Size: 1290 bytes --]

From 9de3a769ae38b1e54ec99e5f105f832a74becfab Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Tue, 4 Aug 2015 19:39:23 +0200
Subject: [PATCH 1/2] Increase read buffer size for MD5 checksumming to 64kiB

	* install.cc (md5_one): Change buffer size from 16kiB to 64kiB for
	faster reading.
---
 ChangeLog  | 7 ++++++-
 install.cc | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 355fd23..334bd9a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,9 @@
-2015-08-03  Achim Gratz  <ASSI <Stromeko@NexGo.DE>>
+2015-08-04  Achim Gratz  <Stromeko@NexGo.DE>
+
+	* install.cc (md5_one): Change buffer size from 16kiB to 64kiB for
+	faster reading.
+
+2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
 
 	* inilex.ll: Introduce HEX and B64 definitions, use them in the
 	rules section.  Parse both SHA512 and SHA512-Base64URL checksums
diff --git a/install.cc b/install.cc
index 1e69564..8c1589b 100644
--- a/install.cc
+++ b/install.cc
@@ -1010,7 +1010,7 @@ md5_one (const packagesource& pkgsource)
   Progress.SetText4 ("Progress:");
   Progress.SetBar1 (0);
 
-  unsigned char buffer[16384];
+  unsigned char buffer[64 * 1024];
   ssize_t count;
   while ((count = thefile->read (buffer, sizeof (buffer))) > 0)
     {
-- 
2.4.6


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Properly-record-SHA512-checksum-presence-and-skip-va.patch --]
[-- Type: text/x-patch, Size: 4447 bytes --]

From d2f2501580391154fbd5331a932271ce17880680 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Tue, 4 Aug 2015 19:48:56 +0200
Subject: [PATCH 2/2] Properly record SHA512 checksum presence and skip
 validation for ad-hoc installs

	* package_source.h (packagesource): Add boolean member variable
	sha512_isSet to record whether an SHA512 checksum has been set.
	(packagesource): Initialize sha512_isSet to false.

	* IniDBBuilderPackage.cc (buildInstallSHA512, buildSourceSHA512):
	Only set the SHA512 checksum when it was previously unset like it
	is done for MD5 checksums.  That will generally be the checksum
	recorded on the package line in setup.ini, any further checksums
	in separate lines will thus be ignored.

	* install.cc (chksum_one): Conditionalize the comparison of the
	SHA512 checksum on whether or not it was previously set.  Check
	SHA512 checksum first since it is the default now.  This is
	necessary for ad-hoc installs from local disk without a setup.ini
	file.  Output a warning when the checksum was not be verified
	because neither a MD5 nor a SHA512 checksum was set.
---
 ChangeLog              | 19 +++++++++++++++++++
 IniDBBuilderPackage.cc |  8 ++++++--
 install.cc             |  8 ++++++--
 package_source.h       |  2 ++
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 334bd9a..03f91b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,24 @@
 2015-08-04  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* package_source.h (packagesource): Add boolean member variable
+	sha512_isSet to record whether an SHA512 checksum has been set.
+	(packagesource): Initialize sha512_isSet to false.
+
+	* IniDBBuilderPackage.cc (buildInstallSHA512, buildSourceSHA512):
+	Only set the SHA512 checksum when it was previously unset like it
+	is done for MD5 checksums.  That will generally be the checksum
+	recorded on the package line in setup.ini, any further checksums
+	in separate lines will thus be ignored.
+
+	* install.cc (chksum_one): Conditionalize the comparison of the
+	SHA512 checksum on whether or not it was previously set.  Check
+	SHA512 checksum first since it is the default now.  This is
+	necessary for ad-hoc installs from local disk without a setup.ini
+	file.  Output a warning when the checksum was not be verified
+	because neither a MD5 nor a SHA512 checksum was set.
+
+2015-08-04  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* install.cc (md5_one): Change buffer size from 16kiB to 64kiB for
 	faster reading.
 
diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index 7ee2af4..b41955a 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -268,15 +268,19 @@ IniDBBuilderPackage::buildInstallSize (const std::string &size)
 void
 IniDBBuilderPackage::buildInstallSHA512 (unsigned char const *sha512)
 {
-  if (sha512)
+  if (sha512 && cbpv.source()->sha512_isSet) {
     memcpy (cbpv.source()->sha512sum, sha512, sizeof cbpv.source()->sha512sum);
+    cbpv.source()->sha512_isSet = true;
+  }
 }
 
 void
 IniDBBuilderPackage::buildSourceSHA512 (unsigned char const *sha512)
 {
-  if (sha512)
+  if (sha512 && cbpv.source()->sha512_isSet) {
     memcpy (cspv.source()->sha512sum, sha512, sizeof cspv.source()->sha512sum);
+    cbpv.source()->sha512_isSet = true;
+  }
 }
 
 void
diff --git a/install.cc b/install.cc
index 8c1589b..a5c4b10 100644
--- a/install.cc
+++ b/install.cc
@@ -1044,8 +1044,12 @@ chksum_one (const packagesource& pkgsource)
 {
   if (!pkgsource.Cached ())
     return;
-  if (pkgsource.md5.isSet())
+  if (pkgsource.sha512_isSet)
+    sha512_one (pkgsource);
+  else if (pkgsource.md5.isSet())
     md5_one (pkgsource);
   else
-    sha512_one (pkgsource);
+    Log (LOG_BABBLE) << "No checksum recorded for " << pkgsource.Base ()
+		     << ", cannot determine integrity of package!"
+		     << endLog;
 }
diff --git a/package_source.h b/package_source.h
index 79d357b..997ccf8 100644
--- a/package_source.h
+++ b/package_source.h
@@ -62,6 +62,7 @@ public:
    _installedSize (0)
   {
     memset (sha512sum, 0, sizeof sha512sum);
+    sha512_isSet = false;
   };
   /* how big is the source file */
   size_t size;
@@ -107,6 +108,7 @@ public:
   virtual void set_canonical (char const *);
   virtual void set_cached (const std::string& );
   unsigned char sha512sum[SHA512_DIGEST_LENGTH];
+  bool sha512_isSet;
   MD5Sum md5;
   typedef std::vector <site> sitestype;
   sitestype sites;
-- 
2.4.6


[-- Attachment #4: Type: text/plain, Size: 188 bytes --]



Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> On Aug  3 22:02, Achim Gratz wrote:
>> 
>> Here are the four patches to make setup work again.
>
> Trusting that you tested them, I only have a minor nit in terms of
> the ChangeLog:

Pushed.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> Replace numerical constants with character literals.

Sure, but only those that should represent characters.  The shift
lengths for instance need to be kept as integers.

> Alternatively, what about using UCHAR_MAX?

There's an implicit assumption that the character is 8bits wide, so
using that constant seems to conceal that.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On Aug  3 22:31, Achim Gratz wrote:
> Corinna Vinschen writes:
> >> 	(hexnibble, nibbled1, b64url, b64d1, b64d2, b64d3): Make all
> >> 	calculation constants dealing with characters character literals.
> >
> > characters character? :)
> 
> How'd you say that?  Sorry, it's been a long day.

Replace numerical constants with character literals.

Alternatively, what about using UCHAR_MAX?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
>> 	(hexnibble, nibbled1, b64url, b64d1, b64d2, b64d3): Make all
>> 	calculation constants dealing with characters character literals.
>
> characters character? :)

How'd you say that?  Sorry, it's been a long day.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Aug  3 22:02, Achim Gratz wrote:
> 
> Here are the four patches to make setup work again.

Trusting that you tested them, I only have a minor nit in terms of
the ChangeLog:

> 	* ini.h (nibbled1): Fix a thinko in this macro that made the result always zero.
       ^^^^^^^^^^^^^^^^
       line too long.

> 	(hexnibble, nibbled1, b64url, b64d1, b64d2, b64d3): Make all
> 	calculation constants dealing with characters character literals.

characters character? :)


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: setup

[Achim Gratz]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]


Here are the four patches to make setup work again.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-MD5sum.str-formatting-bug.patch --]
[-- Type: text/x-patch, Size: 1314 bytes --]

From c7ad9a642a51753344a841c1718efd963af82505 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Mon, 3 Aug 2015 20:15:51 +0200
Subject: [PATCH 1/4] Fix MD5sum.str () formatting bug

	* csu_util/MD5Sum.cc (str): The stream modifiers are not sticky
	and must be applied for each conversion.
---
 ChangeLog          | 5 +++++
 csu_util/MD5Sum.cc | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7d14458..f8cd2b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
+
+	* csu_util/MD5Sum.cc (str): The stream modifiers are not sticky
+	and must be applied for each conversion.
+
 2015-08-01 David Hoke  <d_hoke@hotmail.com>
 
 	* download.cc (IncludeSource): New command line switch
diff --git a/csu_util/MD5Sum.cc b/csu_util/MD5Sum.cc
index 1d9c362..596089d 100644
--- a/csu_util/MD5Sum.cc
+++ b/csu_util/MD5Sum.cc
@@ -85,9 +85,9 @@ MD5Sum::str() const
 {
   std::ostringstream hexdigest;
 
-  hexdigest << std::hex << std::setfill('0') << std::setw(2);
   for (int i=0; i<16; ++i )
-    hexdigest << static_cast<unsigned int>(digest[i]);
+    hexdigest << std::hex << std::setfill('0') << std::setw(2)
+	      << static_cast<unsigned int>(digest[i]);
   return hexdigest.str();
 }
 
-- 
2.4.6


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Fix-bug-in-nibbled1-and-use-character-types-througho.patch --]
[-- Type: text/x-patch, Size: 2472 bytes --]

From 7faf6f57472ee0247c757f12aff56faf2c2c4919 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Mon, 3 Aug 2015 21:04:10 +0200
Subject: [PATCH 2/4] Fix bug in nibbled1 and use character types throughout

	* ini.h (nibbled1): Fix a thinko in this macro that made the result always zero.
	(hexnibble, nibbled1, b64url, b64d1, b64d2, b64d3): Make all
	calculation constants dealing with characters character literals.
	Remove trailing semicolons.
---
 ChangeLog |  7 +++++++
 ini.h     | 20 ++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f8cd2b4..ef3a733 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* ini.h (nibbled1): Fix a thinko in this macro that made the result always zero.
+	(hexnibble, nibbled1, b64url, b64d1, b64d2, b64d3): Make all
+	calculation constants dealing with characters character literals.
+	Remove trailing semicolons.
+
+2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* csu_util/MD5Sum.cc (str): The stream modifiers are not sticky
 	and must be applied for each conversion.
 
diff --git a/ini.h b/ini.h
index 164e3d2..7f6db22 100644
--- a/ini.h
+++ b/ini.h
@@ -59,16 +59,16 @@ extern int yyerror_count;             /* number of parse errors */
 
 /* The following definitions are used in the parser implementation */
 
-#define hexnibble(val) (255 & (val > '9') ? val - 'a' + 10 : val - '0');
-#define nibbled1(v1,v2) (255 & ((v1 << 4) & v2));
+#define hexnibble(val)  ('\xff' & (val > '9') ? val - 'a' + 10 : val - '0')
+#define nibbled1(v1,v2) ('\xff' & ((v1 << 4) | v2))
 #define b64url(val)						\
-  (63 & ((  val == '_') ? 63					\
-	 : (val == '-') ? 62					\
-	 : (val >= 'a') ? val - 'a' + 26			\
-	 : (val >= 'A') ? val - 'A' +  0			\
-	 :                val - '0' + 52))
-#define b64d1(v1,v2,v3,v4) (255 & ((v1 << 2) | (v2 >> 4)));
-#define b64d2(v1,v2,v3,v4) (255 & ((v2 << 4) | (v3 >> 2)));
-#define b64d3(v1,v2,v3,v4) (255 & ((v3 << 6) |  v4));
+  ('\x3f' & ((  val == '_') ? '\x3f'				\
+	     : (val == '-') ? '\x3e'				\
+	     : (val >= 'a') ? val - 'a' + '\x1a'		\
+	     : (val >= 'A') ? val - 'A' + '\x00'		\
+	     :                val - '0' + '\x34'))
+#define b64d1(v1,v2,v3,v4) ('\xff' & ((v1 << 2) | (v2 >> 4)))
+#define b64d2(v1,v2,v3,v4) ('\xff' & ((v2 << 4) | (v3 >> 2)))
+#define b64d3(v1,v2,v3,v4) ('\xff' & ((v3 << 6) |  v4))
 
 #endif /* SETUP_INI_H */
-- 
2.4.6


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-If-MD5-checksum-is-missing-then-SHA512-must-always-b.patch --]
[-- Type: text/x-patch, Size: 1621 bytes --]

From ffbee617a0943ef08d250b3d65a1a46a9b7fd9e1 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Mon, 3 Aug 2015 21:19:53 +0200
Subject: [PATCH 3/4] If MD5 checksum is missing then SHA512 must always be
 checked

	* install.cc (chksum_one): If MD5 checksum is not available, then
	SHA512 must always be checked even if just against the zeroed
	digest as initialized.  Otherwise a package with no checksum would
	be treated as valid.
---
 ChangeLog  | 7 +++++++
 install.cc | 6 +++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ef3a733..4d19736 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
 
+	* install.cc (chksum_one): If MD5 checksum is not available, then
+	SHA512 must always be checked even if just against the zeroed
+	digest as initialized.  Otherwise a package with no checksum would
+	be treated as valid.
+
+2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
+
 	* ini.h (nibbled1): Fix a thinko in this macro that made the result always zero.
 	(hexnibble, nibbled1, b64url, b64d1, b64d2, b64d3): Make all
 	calculation constants dealing with characters character literals.
diff --git a/install.cc b/install.cc
index 4274715..1e69564 100644
--- a/install.cc
+++ b/install.cc
@@ -1044,8 +1044,8 @@ chksum_one (const packagesource& pkgsource)
 {
   if (!pkgsource.Cached ())
     return;
-  if (pkgsource.sha512sum[0])
-    sha512_one (pkgsource);
-  else if (pkgsource.md5.isSet())
+  if (pkgsource.md5.isSet())
     md5_one (pkgsource);
+  else
+    sha512_one (pkgsource);
 }
-- 
2.4.6


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Simplify-parser-both-SHA512-checksum-types-can-parse.patch --]
[-- Type: text/x-patch, Size: 3872 bytes --]

From 4d0695ecf313e9eede3f6693ea9426e0ce674517 Mon Sep 17 00:00:00 2001
From: Achim Gratz <Stromeko@Stromeko.DE>
Date: Mon, 3 Aug 2015 21:23:37 +0200
Subject: [PATCH 4/4] Simplify parser, both SHA512 checksum types can parse to
 the same token

	* inilex.ll: Introduce HEX and B64 definitions, use them in the
	rules section.  Parse both SHA512 and SHA512-Base64URL checksums
	to the SHA512 token.
	* iniparse.yy (packagedata): Remove all occurences of the
	SHA512B64URL token.
---
 ChangeLog   |  8 ++++++++
 inilex.ll   | 10 ++++++----
 iniparse.yy |  7 ++-----
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4d19736..cf5cbda 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2015-08-03  Achim Gratz  <ASSI <Stromeko@NexGo.DE>>
+
+	* inilex.ll: Introduce HEX and B64 definitions, use them in the
+	rules section.  Parse both SHA512 and SHA512-Base64URL checksums
+	to the SHA512 token.
+	* iniparse.yy (packagedata): Remove all occurences of the
+	SHA512B64URL token.
+
 2015-08-03  Achim Gratz  <Stromeko@NexGo.DE>
 
 	* install.cc (chksum_one): If MD5 checksum is not available, then
diff --git a/inilex.ll b/inilex.ll
index 86100ae..67764f0 100644
--- a/inilex.ll
+++ b/inilex.ll
@@ -46,10 +46,12 @@ static void ignore_line (void);
 %x eolstate
 
 STR	[!a-zA-Z0-9_./:\+~-]+
+HEX	[0-9a-f]
+B64	[a-zA-Z0-9_-]
 
 %%
 
-[0-9a-f]{32} {
+{HEX}{32} {
     yylval = (char *) new unsigned char[16];
     memset (yylval, 0, 16);
     int i, j;
@@ -63,7 +65,7 @@ STR	[!a-zA-Z0-9_./:\+~-]+
     return MD5;
 }
 
-[0-9a-f]{128} {
+{HEX}{128} {
     yylval = (char *) new unsigned char[SHA512_DIGEST_LENGTH];
     memset (yylval, 0, SHA512_DIGEST_LENGTH);
     int i, j;
@@ -77,7 +79,7 @@ STR	[!a-zA-Z0-9_./:\+~-]+
     return SHA512;
 }
 
-[a-zA-Z0-9_-]{86} {
+{B64}{86} {
     /* base64url as defined in RFC4648 */
     yylval = (char *) new unsigned char[SHA512_DIGEST_LENGTH];
     memset (yylval, 0, SHA512_DIGEST_LENGTH);
@@ -98,7 +100,7 @@ STR	[!a-zA-Z0-9_./:\+~-]+
     v3 = 0;
     v4 = 0;
     ((unsigned char *) yylval) [j+0] = b64d1(v1, v2, v3, v4);
-    return SHA512B64URL;
+    return SHA512;
 }
 
 \"[^"]*\"		{ yylval = new char [strlen (yytext+1) + 1];
diff --git a/iniparse.yy b/iniparse.yy
index 442135d..5fd6c3f 100644
--- a/iniparse.yy
+++ b/iniparse.yy
@@ -42,8 +42,8 @@ void add_correct_version();
 %token CATEGORY DEPENDS REQUIRES
 %token APATH PPATH INCLUDE_SETUP EXCLUDE_PACKAGE DOWNLOAD_URL
 %token T_PREV T_CURR T_TEST
-%token SHA512 SHA512B64URL MD5 INSTALLEDSIZE MAINTAINER PRIORITY
-%token MD5LINE SHA512LINE
+%token INSTALLEDSIZE MAINTAINER PRIORITY
+%token MD5 MD5LINE SHA512 SHA512LINE
 %token DESCTAG DESCRIPTION FILESIZE ARCHITECTURE SOURCEPACKAGE
 %token RECOMMENDS PREDEPENDS
 %token SUGGESTS CONFLICTS REPLACES PROVIDES PACKAGENAME STRTOEOL PARAGRAPH
@@ -109,7 +109,6 @@ singleitem /* non-empty */
  | STANDARDSVERSION STRING NL	{ /* TODO */ }
  | MD5LINE MD5 NL	{ iniBuilder->buildInstallMD5 ((unsigned char *)$2); }
  | SHA512LINE SHA512 NL		{ iniBuilder->buildInstallSHA512 ((unsigned char *)$2); }
- | SHA512LINE SHA512B64URL NL	{ iniBuilder->buildInstallSHA512 ((unsigned char *)$2); }
  | SOURCEPACKAGE source NL
  | CATEGORY categories NL
  | INSTALL STRING { iniBuilder->buildPackageInstall ($2); } installmeta NL
@@ -149,13 +148,11 @@ installmeta: /* empty */
 installchksum: /* empty */
  | MD5 			{ iniBuilder->buildInstallMD5 ((unsigned char *)$1);}
  | SHA512		{ iniBuilder->buildInstallSHA512 ((unsigned char *)$1);}
- | SHA512B64URL		{ iniBuilder->buildInstallSHA512 ((unsigned char *)$1);}
  ;
 
 sourcechksum: /* empty */
  | MD5 			{ iniBuilder->buildSourceMD5 ((unsigned char *)$1); }
  | SHA512 		{ iniBuilder->buildSourceSHA512 ((unsigned char *)$1); }
- | SHA512B64URL		{ iniBuilder->buildSourceSHA512 ((unsigned char *)$1); }
  ;
 
 source /* non-empty */
-- 
2.4.6


[-- Attachment #6: Type: text/plain, Size: 197 bytes --]



Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for KORG EX-800 and Poly-800MkII V0.9:
http://Synth.Stromeko.net/Downloads.html#KorgSDada

Re: setup

[Achim Gratz]

Achim Gratz writes:
> I've just found out that MD5 checksums get parsed OK, but wind up as
> solid zeros in the package database in setup.  I have no idea how this
> is even possible, since the exact same code works just fine for SHA512
> checksums.  I didn't check yet if this actually works in the release
> version of setup.  An extra pair of eyeballs would be appreciated.  I
> don't see how my changes could be responsible and I was sure I'd checked
> it before… :-(

Found it, I feel stupid now…

> Also, I want to clean up the parser code a bit and fix a very old
> formatting bug found while debugging the problem with the MD5 checksums.

In preparation.

> Another thing: it seems that the SHA512 digest is checked for existence
> by checking that the first byte is non-zero.  Is that actually a
> guarantee that the SHA2 standard makes or is it just very unlikely to
> encounter 0x00 as the first byte?

If the SHA512 checksum is all zeros in the package DB it doesn't get
checked, which is actually the reason I didn't catch the error earlier.
Have to fix that also.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Achim Gratz]

I've just found out that MD5 checksums get parsed OK, but wind up as
solid zeros in the package database in setup.  I have no idea how this
is even possible, since the exact same code works just fine for SHA512
checksums.  I didn't check yet if this actually works in the release
version of setup.  An extra pair of eyeballs would be appreciated.  I
don't see how my changes could be responsible and I was sure I'd checked
it before… :-(

Also, I want to clean up the parser code a bit and fix a very old
formatting bug found while debugging the problem with the MD5 checksums.

Another thing: it seems that the SHA512 digest is checked for existence
by checking that the first byte is non-zero.  Is that actually a
guarantee that the SHA2 standard makes or is it just very unlikely to
encounter 0x00 as the first byte?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup

[Achim Gratz]

Corinna Vinschen writes:
> On Jul 28 21:19, Achim Gratz wrote:
>> http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/37bbf074f6333624be361f2acd07dfefeacd5a83
>
> This one is missing a ChangeLog hint that it tries to cleanup the code
> slightly (extra else branch for upgrade == true), otherwise ok for commit.

Done with that change.

>> http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/0c6a81eec41765ef7864682b7a038c7237c21c16
>
> Ok for commit.

Done with a ChangeLog entry.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

Re: setup

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

On Jul 28 21:19, Achim Gratz wrote:
> 
> With the latest round of changes I'm now down to two local change-sets
> for setup:
> 
> 1. Implement an option that installs exactly the requested packages, no
> matter what packages are already installed.  This leaves an installation
> in the same state as a new install from scratch would have, modulo any
> customizations in /etc.  All package versions will be reset to current,
> missing packages get installed and additional packages removed.  I use
> this to ensure that automated installs end up with the same packages no
> matter the history of the existing installation.  Specifically I
> implemented it so that I can switch installations from one option to
> another (currently there are four install options: command line, GUI,
> developer and developer w/ debuginfo) without manual intervention.
> 
> http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/37bbf074f6333624be361f2acd07dfefeacd5a83

This one is missing a ChangeLog hint that it tries to cleanup the code
slightly (extra else branch for upgrade == true), otherwise ok for commit.

> 2. Implement an option that pulls in source packages automatically.
> This is taken from a patch offered by David Hoke on this list about a
> year ago (ChangeLog is missing at the moment).
> 
> http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/0c6a81eec41765ef7864682b7a038c7237c21c16

Ok for commit.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

setup

[Achim Gratz]

With the latest round of changes I'm now down to two local change-sets
for setup:

1. Implement an option that installs exactly the requested packages, no
matter what packages are already installed.  This leaves an installation
in the same state as a new install from scratch would have, modulo any
customizations in /etc.  All package versions will be reset to current,
missing packages get installed and additional packages removed.  I use
this to ensure that automated installs end up with the same packages no
matter the history of the existing installation.  Specifically I
implemented it so that I can switch installations from one option to
another (currently there are four install options: command line, GUI,
developer and developer w/ debuginfo) without manual intervention.

http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/37bbf074f6333624be361f2acd07dfefeacd5a83

2. Implement an option that pulls in source packages automatically.
This is taken from a patch offered by David Hoke on this list about a
year ago (ChangeLog is missing at the moment).

http://repo.or.cz/w/cygwin-setup/local.git/commitdiff/0c6a81eec41765ef7864682b7a038c7237c21c16


Both changes have been tested for a while, albeit in a somewhat
restricted environment.  Let me know what you think, I'd love to get
these upstream.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup

[Brian Keener]

Robert Collins wrote:
> Anyway, I've an idea for a UI change to support it more effectively: add
> a bin column, and the default is bin on, src off, but the user can
> choose any combination (with bin and src off = keep/skip

Sounds intriguing but also I'm afraid somewhat confusing as I now have the 
version selection column (keep/skip, Reinstall, Retrieve, 1.4.3, 1.4.4, 
uninstall ....) And then I have the source column which dictates if I want the 
source for the selected version if I am changing version or I can select source 
only for the current version.  Now you add a bin column and then I could have 
in the version column 1.4.4 (which is not the current installed version) but 
have bin and source off which is essentially do nothing (sounds confusing).

How about an alternate approach in terms of get rid of the Source column and 
only use the version columns as follows:

keep/skip, uninstall, 1.4.4 bin&src, 1.4.4 bin, 1.4.4 src, reinstall bin&src, 
reinstall bin, reinstall src, retrieve bin&src, retrieve bin, retrieve src ...

Just an alternate thought.

Re: setup

[Michael A Chase]

----- Original Message ----- 
From: "Robert Collins" <robert.collins@itdomain.com.au>
To: <cygwin-apps@cygwin.com>
Sent: Monday, March 04, 2002 19:28
Subject: setup


I'll be looking into the reports on the setup-20020225 beta
today/tonight. I've caught one regression myself - there's no src only
capability any more :[. How'd that happen? :}.

Anyway, I've an idea for a UI change to support it more effectively: add
a bin column, and the default is bin on, src off, but the user can
choose any combination (with bin and src off = keep/skip)

======
That sounds more intuitive than the src checkbox.

Will it be in the normal CVS update or will you produce a new snapshot?
-- 
Mac :})
** I normally forward private questions to the appropriate mail list. **
Ask Smarter: http://www.tuxedo.org/~esr/faqs/smart-questions.html
Give a hobbit a fish and he eats fish for a day.
Give a hobbit a ring and he eats fish for an age.

setup

[Robert Collins]

I'll be looking into the reports on the setup-20020225 beta
today/tonight. I've caught one regression myself - there's no src only
capability any more :[. How'd that happen? :}.

Anyway, I've an idea for a UI change to support it more effectively: add
a bin column, and the default is bin on, src off, but the user can
choose any combination (with bin and src off = keep/skip)

Thoughts?
Rob

Re: Setup

[Brian Keener]

Robert Collins wrote:
> > As to the NULL file when reinstalling - that patch was applied to HEAD
> and not
> ...
> > tried to just install zlib it still failed and would not install zlib.
> It just > doesn't seem to like the last file.
> 
> I'll have a look-see.
>

I tried the setup-20020225.exe and also updated my sources from cvs HEAD and 
this problem appears to be fixed.  I installed one and only one file  as a 
reinstall and it appeared to work just like it should.

It also appears (and I haven't looked real close yet) that you have improved 
what packages show and do not show depending on what installation/download 
method I chose.

Good job.

Bk

Re: Setup

[Christopher Faylor]

On Sun, Feb 24, 2002 at 10:04:32PM +1100, Robert Collins wrote:
>> I don't think that setup.exe should automatically switch to Uninstall
>> in any circumstances unless the package maintainer has specifically
>> indicated that is the correct behavior.  Somehow.
>
>I agree. My point has been (from the beginning) about *where* that
>intent gets indicated.

No, you're not agreeing if you think that a setup.ini file can be
crafted in such a way that an "Uninstall" will show up without the
package maintainer explicitly somehow saying "I want this package to
be uninstalled".  And, not making a test version of a package available
is not "explicit".

cgf

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Christopher Faylor" <cgf@redhat.com>

> >Otherwise we need a *new* mechanism to tell setup.exe when a package
is
> >replaced from current to test - that is that no test version exists,
and
> >that when moving to test, the current version should be removed.
>
> Ok.  We need a new mechanism.  We also need a mechanism that says
> "remove this package" and I don't think that the mechanism is to just
> move the package to "prev".  Maybe the mechanism is as simple as just
> having a setup.hint like:
>
>   setup.hint:
>   curr uninstall "This package is now obsolete"
>
> (wouldn't it be cool to have a "bubble" appear with the above words
when
> you moved the mouse cursor over the package name?)
>
> Actually, in this case, where the package maintainer means to
obviously
> uninstall something, I think it is acceptable for setup.exe to do so.

Versioned conflicts will do this, which is a wishlist feature. Someone
offered to do them, but I don't think a patch has arrived yet.

> I don't think that setup.exe should automatically switch to Uninstall
> in any circumstances unless the package maintainer has specifically
> indicated that is the correct behavior.  Somehow.

I agree. My point has been (from the beginning) about *where* that
intent gets indicated.

Rob

Re: Setup

[Robert Collins]

----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>

> I catch the drift on the patches and the same with the other two we
discussed.  > At one point in time when I was still submitting changes
to setup, when I still > understood the code before the major rewrite
:-)

It should be a lot easier to understand now, and it's starting to really
come together now.

> Another nice touch and clarification in chooser might be to expand the
text on
> the version number in the display for those packages where I might
have 5
> versions available for install.  As I am clicking through the versions
on a
> given package when I get to a version that is the Prev, Curr or Test
if the
> text shows this - ie 1.4.5 (Curr) or 1.4.3 (Prev) or 1.4.4.  In this
case 1.4.4 > is just an available intermediate.

Nice idea, patches accepted :} (Hint: start with the PackagePickLine
class). I've added to the TODO list.

Rob

Re: Setup

[Robert Collins]

----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>

> > he solution is wrong, but I can't articulate (yet) how the model is
> > wrong, and so I've followed the (apparent) consensus.
>
> I don't know why that doesn't seem right to you.  It appears to me
that I will
> never have an installation that is comprised solely of all Test
packages.

[Devil's advocate mode on] Why not? Debian has one, and it works great.

> I
> will always be testing a few packages but the bulk of the system will
be the
> current Stable versions.  That would naturally be the nature of the
beast as
> working with all test versions would be to cumbersome to find where
something
> was failing.  That said if I have to work with Current Stable versions
while I
> am testing experimental packages why would I want the default on the
installed
> packages to be Uninstall - I would want to keep those packages so I
would still > have a working system. I also do not particularly want to
click on 20 packages
> to say keep these instead of uninstalling.  Makes perfect sense to me.

I have never said that I want every package to uninstall. I have
explained that that behaviour is a SIDE EFFECT of the behaviour I want,
which is for system smarts about the intention of maintainers to be
removed from setup, and made explicit. This gives greater flexability,
and the potential for quicker changes - because setup.exe won't be part
of the change process.

However, that was not what I was referring to in saying that the
solution is wrong. I mean that the whole x= prev/curr/test y=version
model is wrong, and because THAT is wrong, the GUI and engine behaviour
is confusing (because it has multiple, reasonable interpretations).

> As to the NULL file when reinstalling - that patch was applied to HEAD
and not
...
> tried to just install zlib it still failed and would not install zlib.
It just > doesn't seem to like the last file.

I'll have a look-see.

Rob

Re: Setup

[Christopher Faylor]

On Fri, Feb 22, 2002 at 08:48:04PM -0500, Brian Keener wrote:
>Robert Collins wrote:
>> What TAG did you build from? setup200202 works fine for me, and doesn't
>> uninstall installed packages when test is selected. Like Chris I feel
>
>Built from HEAD - which is why I guess the fix for the Test/Uninstall wasn't 
>there, but once I applied the patch for the Test/Uninstall fix to my copy then 
>Test/Keep as opposed to Test/Uninstall works great.
>
>> he solution is wrong, but I can't articulate (yet) how the model is
>> wrong, and so I've followed the (apparent) consensus.
>
>I don't know why that doesn't seem right to you.

I think Robert was saying that the whole prev/curr/test model feels wrong.

But, perhaps uncharacteristically, neither of us has an opinion on how to
change it.  :-)

cgf

Re: Setup

[Brian Keener]

Robert Collins wrote:
> What TAG did you build from? setup200202 works fine for me, and doesn't
> uninstall installed packages when test is selected. Like Chris I feel

Built from HEAD - which is why I guess the fix for the Test/Uninstall wasn't 
there, but once I applied the patch for the Test/Uninstall fix to my copy then 
Test/Keep as opposed to Test/Uninstall works great.

> he solution is wrong, but I can't articulate (yet) how the model is
> wrong, and so I've followed the (apparent) consensus.

I don't know why that doesn't seem right to you.  It appears to me that I will 
never have an installation that is comprised solely of all Test packages.  I 
will always be testing a few packages but the bulk of the system will be the 
current Stable versions.  That would naturally be the nature of the beast as 
working with all test versions would be to cumbersome to find where something 
was failing.  That said if I have to work with Current Stable versions while I 
am testing experimental packages why would I want the default on the installed 
packages to be Uninstall - I would want to keep those packages so I would still 
have a working system. I also do not particularly want to click on 20 packages 
to say keep these instead of uninstalling.  Makes perfect sense to me.

As to the NULL file when reinstalling - that patch was applied to HEAD and not 
just setup200202 wasn't it or was the full fix only committed to setup200202? I 
do seem to have that fix (since I get the message about the Null file - no 
silent failure) or at least part of it but it does still seem to have a problem 
with the last file that you might have selected.  At first I thought it only 
had the problem if for some reason setup determined it needed the method 
replace_one but even after it removed zlib when I said reinstall and then I 
tried to just install zlib it still failed and would not install zlib.  It just 
doesn't seem to like the last file.

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>
To: <cygwin-apps@sources.redhat.com>
Sent: Friday, February 22, 2002 12:53 PM
Subject: Re: Setup


> Robert Collins wrote:
> > And a complete fix is in CVS now.
> >
>
> I just tried setup from cvs and rebuilt and I still get the null file
on the
> last file I try to install from my local directory.  Actually it was a
> reinstall of zlib and it failed with the null error and then when I
went back
> into setup it showed it as a new install and when I selected to
install it - I
> still got the Null error.  I then went and did an install from the
internet and
> zlib showed as needing to be installed and then installed just fine.
>
> It might be something with my version since I did a full cvs update
but I
> didnot get the patch for test to default to keep if already installed.
That
> appears to have been applied to the branch and so I applied it to mine
> manually.  So maybe something else got skipped as well.

What TAG did you build from? setup200202 works fine for me, and doesn't
uninstall installed packages when test is selected. Like Chris I feel
the solution is wrong, but I can't articulate (yet) how the model is
wrong, and so I've followed the (apparent) consensus.

If you're looking at HEAD, bugfixes are going in there as well, but the
test mode behaviour is cosmetic IMO, as opposed to the silent failure
from local directories!

Rob

Re: Setup

[Brian Keener]

Robert Collins wrote:
> And a complete fix is in CVS now.
>

I just tried setup from cvs and rebuilt and I still get the null file on the 
last file I try to install from my local directory.  Actually it was a 
reinstall of zlib and it failed with the null error and then when I went back 
into setup it showed it as a new install and when I selected to install it - I 
still got the Null error.  I then went and did an install from the internet and 
zlib showed as needing to be installed and then installed just fine.

It might be something with my version since I did a full cvs update but I 
didnot get the patch for test to default to keep if already installed.  That 
appears to have been applied to the branch and so I applied it to mine 
manually.  So maybe something else got skipped as well.

Bk

RE: Setup

[Robert Collins]

> -----Original Message-----
> From: Gary R. Van Sickle [mailto:g.r.vansickle@worldnet.att.net]

> Feature freeze feature freeze feature freeze ;-)

HEAD is open for business, as per ~ 3 emails back.
Rob

Re: Setup

[Christopher Faylor]

On Wed, Feb 20, 2002 at 11:17:05PM -0600, Gary R. Van Sickle wrote:
>> -----Original Message-----
>> From: cygwin-apps-owner@cygwin.com
>> [mailto:cygwin-apps-owner@cygwin.com]On Behalf Of Christopher Faylor
>
>[snip]
>
>> (wouldn't it be cool to have a "bubble" appear with the above words when
>> you moved the mouse cursor over the package name?)
>>
>
>NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Um.  I wasn't suggesting this for this release.  I understand what feature
freeze means.

Speaking of chilling...

cgf

RE: Setup

[Gary R. Van Sickle]

> -----Original Message-----
> From: cygwin-apps-owner@cygwin.com
> [mailto:cygwin-apps-owner@cygwin.com]On Behalf Of Christopher Faylor

[snip]

> (wouldn't it be cool to have a "bubble" appear with the above words when
> you moved the mouse cursor over the package name?)
>

NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Feature freeze feature freeze feature freeze ;-)

No, for Setup 3.0 or whatever the release-after-next will be numbered, I think
we almost need to have something like that.  In particular I think we need to do
the "popup-text-tooltip-when-the-description-text-goes-off-the-screen" trick,
since pretty much every description overflows the allotted real-estate.

--
Gary R. Van Sickle
Brewer.  Patriot.

Re: Setup

[Brian Keener]

Robert Collins wrote:
> > That's where our view differs because I see the radio buttons as a way
> of
> > selecting the maximum version I want considered and if that version
> does not
> > exist then the default is the next version down (Test -> Current ->
> Prev
> > depending on Radio button selected) and if the version selected is
> what is
> > installed the default is keep and if it is not installed at all then
> the
> > default is skip.
> 
> Well, I'll consider a patch to setup to change it's behaviour. At the
> moment no limitation of maximum version is present, regardless of the
> prev/curr/test state.

I catch the drift on the patches and the same with the other two we discussed.  
At one point in time when I was still submitting changes to setup, when I still 
understood the code before the major rewrite :-) - don't get me wrong this is 
good stuff - I thought we had it where Test, Curr, and Prev only set the 
maximum version we wanted to look at.  But on the down side we never had it 
fully functional where it would handle multiple versions simply because they 
existed on disk and this is a very good new feature.  I would not want to loose 
itt.

But what I do propose is that we think of Prev as the last stable version, we 
think of Current as the current stable and we think of test as a development 
version - which could be changing rapidly.  There may be any number of versions 
resident on my disk that are in between the control points and they should all 
be available for me to choose from in the chooser.  But the maintainer dictates 
via setup.ini which version is the Prev stable, Curr stable and experimental 
(Test) or any combination thereof. 

With this in mind the Prev, Curr, and Test buttons should simply set the 
maximum version (with the primary choice being the Current Stable for the 
inexperience that simply want to use the product) that I want installed of any 
given package and then I still have the ability to select any other major or 
intermediate version (these amy or may not be a Prev, Curr, or Test) for a 
package as well.  Essentially if I have the Current radio selected and a Prev 
is installed and there is a new Curr it will select that current version for 
install and if on another package I have a Test version installed and there is 
a Curr version available then it will select the current version for install 
(thus deinstalling the Test version) and if by some quirk I have a Test 
installed, the Current Radio button selected but there is no Curr version it 
will select the Prev and if there is no Prev version it will simply select keep 
(although I'm not sure I like this - but I like an uninstall option even less).

I would think we also need some mechanism (because of the ability to mix and 
match versions) to give the user the ability to select the default actions for 
all packages listed - for example: 1) I want to *keep/skip* (depending on if 
installed or not) all and then simply upgrade one or two - no matter what is 
available 2) I want to *reinstall* whatever I currently have installed - no 
matter what is available 3) I want to install *sources* for everything I have 
installed - nothing more - nothing less and the really nasty option I want to 
*uninstall* everything with a big fat warning.  I am essentially describing a 
global for the chooser that lets me pick from the standard chooser options but 
for all packages.  I can then pick and choose below that for the individual 
package.  I am sure categories will to some degree get in the way of this but I 
am sure that some workable solution could be found.

Another nice touch and clarification in chooser might be to expand the text on 
the version number in the display for those packages where I might have 5 
versions available for install.  As I am clicking through the versions on a 
given package when I get to a version that is the Prev, Curr or Test if the 
text shows this - ie 1.4.5 (Curr) or 1.4.3 (Prev) or 1.4.4.  In this case 1.4.4 
is just an available intermediate.

This is my $.02 worth and the world of setup and chooser as I see and would 
code if I could - don't get me wrong I'm still trying but the going is slow - 
got my hands in too many pots (and some have hot water) and setup is a tough 
teacher for learning C++.

Bk

Re: Setup

[Christopher Faylor]

On Mon, Feb 18, 2002 at 11:46:01PM -0500, Christopher Faylor wrote:
>>Setup doesn't *default* to uninstall.  Two things have to happen: The
>>user has to select Test (which means 'give me a testing distribution').
>>Their has to be no valid testing version for that package.
>
>If I click on Test, then a whole bunch of Installs show up on the
                                         uninstalls
>screen.  If you don't like the word "default" then I'll use
>"automatically switch".

cgf

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>
To: <cygwin-apps@cygwin.com>
Sent: Tuesday, February 19, 2002 3:44 PM
Subject: Re: Setup


> Robert Collins wrote:
> > > But if I select test (which means 'give me a testing package not a
> > testing
> > > distribution')
> >
> > No it doesn't. Clicking on a package version is how one selects per
> > package versions.
> >
> That's where our view differs because I see the radio buttons as a way
of
> selecting the maximum version I want considered and if that version
does not
> exist then the default is the next version down (Test -> Current ->
Prev
> depending on Radio button selected) and if the version selected is
what is
> installed the default is keep and if it is not installed at all then
the
> default is skip.

Well, I'll consider a patch to setup to change it's behaviour. At the
moment no limitation of maximum version is present, regardless of the
prev/curr/test state.

Rob

Re: Setup

[Christopher Faylor]

On Tue, Feb 19, 2002 at 03:19:43PM +1100, Robert Collins wrote:
>
>===
>----- Original Message -----
>From: "Christopher Faylor" <cgf@redhat.com>
>To: <cygwin-apps@cygwin.com>
>Sent: Tuesday, February 19, 2002 3:05 PM
>Subject: Re: Setup
>
>
>> On Tue, Feb 19, 2002 at 12:27:26PM +1100, Robert Collins wrote:
>>If the question is "Should 'upset' add a dummy Test entry for every
>>case where there is no such thing?" then the answer, IMO, is no.  I
>>think the same applies for the case where an initial release of a
>>product is marked test.  Setting up a dummy "Current" which is the same
>>as "Test" would defeat the purpose of "Test".
>
>For the first case, I think the answer is yes, for the second, it
>*should* be no (because, as you say, it would defeat the purpose of
>test).
>
>Otherwise we need a *new* mechanism to tell setup.exe when a package is
>replaced from current to test - that is that no test version exists, and
>that when moving to test, the current version should be removed.

Ok.  We need a new mechanism.  We also need a mechanism that says
"remove this package" and I don't think that the mechanism is to just
move the package to "prev".  Maybe the mechanism is as simple as just
having a setup.hint like:

  setup.hint:
  curr uninstall "This package is now obsolete"

(wouldn't it be cool to have a "bubble" appear with the above words when
you moved the mouse cursor over the package name?)

Actually, in this case, where the package maintainer means to obviously
uninstall something, I think it is acceptable for setup.exe to do so.

>>I think that the bottom line is that setup.exe should NEVER default to
>>Uninstall.  Uninstall should only be on when the user specifically
>>selects it.  Anything else is, IMO, surprising and dangerous.
>
>I agree that the user should be warned before automated uninstalls
>happen.  Thats not ever been the case though in the gui.

I've always considered it a bug when the word "Uninstall" shows up
in the GUI and I didn't specifically click on it.

>Setup doesn't *default* to uninstall.  Two things have to happen: The
>user has to select Test (which means 'give me a testing distribution').
>Their has to be no valid testing version for that package.

If I click on Test, then a whole bunch of Installs show up on the
screen.  If you don't like the word "default" then I'll use
"automatically switch".

I don't think that setup.exe should automatically switch to Uninstall
in any circumstances unless the package maintainer has specifically
indicated that is the correct behavior.  Somehow.

When I click on test I assume it means "Give me all of the test versions".
I think it is acceptable that if there are no test versions, then every
other package shows up as "Skip".  I don't think that we should assume
that the correct behavior is Uninstall.

Actually, this is one of the things that I hate about the current way
setup works.  I don't like the Prev/Curr/Test stuff that much.  It feels
like we're overloading functionality incorrectly but I don't have a real
idea about how to fix it.

cgf

Re: Setup

[Brian Keener]

Robert Collins wrote:
> > But if I select test (which means 'give me a testing package not a
> testing
> > distribution')
> 
> No it doesn't. Clicking on a package version is how one selects per
> package versions.
>
That's where our view differs because I see the radio buttons as a way of 
selecting the maximum version I want considered and if that version does not 
exist then the default is the next version down (Test -> Current -> Prev 
depending on Radio button selected) and if the version selected is what is 
installed the default is keep and if it is not installed at all then the 
default is skip.

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>

> But if I select test (which means 'give me a testing package not a
testing
> distribution')

No it doesn't. Clicking on a package version is how one selects per
package versions.

> and the only test version available is a test version of vim
> that I want to try and everything else defaults to uninstall and I say
next
> what do I have when I get done - nada - nothing and lets do it again
because I
> wasn't paying attention and had no idea that setup would uninstall my
entire
> system because I wanted to try one test version.
>
> Hummmmm - sounds pretty nasty to me.

Exactly why I've been raising this!

Rob

Re: Setup

[Brian Keener]

Robert Collins wrote:
> Setup doesn't *default* to uninstall. Two things have to happen:
> The user has to select Test (which means 'give me a testing
> distribution').
> Their has to be no valid testing version for that package.

But if I select test (which means 'give me a testing package not a testing 
distribution') and the only test version available is a test version of vim 
that I want to try and everything else defaults to uninstall and I say next 
what do I have when I get done - nada - nothing and lets do it again because I 
wasn't paying attention and had no idea that setup would uninstall my entire 
system because I wanted to try one test version.

Hummmmm - sounds pretty nasty to me.

bk

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>
To: "Robert Collins" <robert.collins@itdomain.com.au>;
<cygwin-apps@sources.redhat.com>
Sent: Tuesday, February 19, 2002 3:18 PM
Subject: Re: Setup


> Robert Collins wrote:
> > I have emailed trying to start a discussion on this at least twice.
My
> > contention is that this is a factor for setup.ini to take care of,
not
> > setup.exe. It's easier to add entries than have complex logic for
> > removing them when they aren't appropriate.
> >
> So what you're saying is just looking at Current vs Previous - if
there was no
> setup.ini entry for current and there was only an entry for previous
then with
> the default display set to current the selected default action for the
> theoretical package would be uninstall (just as I saw on the Test) -
correct?

No. There is a pretty big difference (IMO) between prev, current and
test.
Prev - distribution with old versions kept around for convenience sake.
Current - distribution with the latest stable versions.
Test - distribution with versions where things break, major changes
occur etc.

Any version of a package can be selected without the use of the
prev/curr/test buttons - they are designed to make wholesale changes to
the environment.

Prev-Current should be minor - to allow backing out of a minor revision
change that introduced a new bug.

Current-Test *may* be very major.

we have currently in the 'default' picking logic:
packageversion * trustp (trusts const t) const
{
  return t == TRUST_PREV ? (prev ? prev : (curr ? curr : installed))
       : t == TRUST_CURR ? (curr ? curr : installed)
       : exp;
}

IOW, for prev, choose an explicit prev, then a current, and then an
installed.
for current, choose a current, and then an installed.
for test (exp), choose the explicit experimental version.

> And in addition if as a maintainer I didn't want this to happen then I
should
> make sure I have a prev,curr and test in setup.ini even if they all
are the
> same file name.

That is one way, it is very manual however. What I'm suggesting is that
*in the absence* of prev:,curr:,test: tags in setup.hint, that upset
always generate a
[test] entry - just like it automatically generates a [prev] entry when
there are two versions in the directory.

> This bears the question (if my previous assumption is correct) what if
my
> setup.ini has a prev, curr, and test file defined but I am doing an
"Install
> from Local Directory" and the test version does not exist on my
system.  I
> would assume that since I defined a test and even though is does not
exist that > my default action would be keep - true?

I'm not sure :}. I'm not sure that that would be appropriate behaivour
in the general case. We really need versioned conflicts: and depends:
lines to make this all work correctly.

Rob

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Christopher Faylor" <cgf@redhat.com>
To: <cygwin-apps@cygwin.com>
Sent: Tuesday, February 19, 2002 3:05 PM
Subject: Re: Setup


> On Tue, Feb 19, 2002 at 12:27:26PM +1100, Robert Collins wrote:

> If the question is "Should 'upset' add a dummy Test entry for every
case
> where there is no such thing?" then the answer, IMO, is no.  I think
the
> same applies for the case where an initial release of a product is
> marked test.  Setting up a dummy "Current" which is the same as "Test"
> would defeat the purpose of "Test".

For the first case, I think the answer is yes, for the second, it
*should* be no (because, as you say, it would defeat the purpose of
test).

Otherwise we need a *new* mechanism to tell setup.exe when a package is
replaced from current to test - that is that no test version exists, and
that when moving to test, the current version should be removed.

> I think that the bottom line is that setup.exe should NEVER default to
> Uninstall.  Uninstall should only be on when the user specifically
selects
> it.  Anything else is, IMO, surprising and dangerous.

I agree that the user should be warned before automated uninstalls
happen. Thats not ever been the case though in the gui.

Setup doesn't *default* to uninstall. Two things have to happen:
The user has to select Test (which means 'give me a testing
distribution').
Their has to be no valid testing version for that package.

Rob

Re: Setup

[Brian Keener]

Robert Collins wrote:
> I have emailed trying to start a discussion on this at least twice. My
> contention is that this is a factor for setup.ini to take care of, not
> setup.exe. It's easier to add entries than have complex logic for
> removing them when they aren't appropriate.
>
So what you're saying is just looking at Current vs Previous - if there was no 
setup.ini entry for current and there was only an entry for previous then with 
the default display set to current the selected default action for the 
theoretical package would be uninstall (just as I saw on the Test) - correct?

And in addition if as a maintainer I didn't want this to happen then I should 
make sure I have a prev,curr and test in setup.ini even if they all are the 
same file name.

This bears the question (if my previous assumption is correct) what if my 
setup.ini has a prev, curr, and test file defined but I am doing an "Install 
from Local Directory" and the test version does not exist on my system.  I 
would assume that since I defined a test and even though is does not exist that 
my default action would be keep - true?

bk

Re: Setup

[Christopher Faylor]

On Tue, Feb 19, 2002 at 12:27:26PM +1100, Robert Collins wrote:
>>1) If I have a group of packages installed and I select the Test option
>>to see what packages are available for in Test should all the packages
>>I have installed that do not have a Test Version show Uninstall or
>>should they should keep??  It is my contention that thy should show
>>keep and not uninstall - unless I explicitly request uninstall.
>
>I have emailed trying to start a discussion on this at least twice.  My
>contention is that this is a factor for setup.ini to take care of, not
>setup.exe.  It's easier to add entries than have complex logic for
>removing them when they aren't appropriate.

If the question is "Should 'upset' add a dummy Test entry for every case
where there is no such thing?" then the answer, IMO, is no.  I think the
same applies for the case where an initial release of a product is
marked test.  Setting up a dummy "Current" which is the same as "Test"
would defeat the purpose of "Test".

I think that the bottom line is that setup.exe should NEVER default to
Uninstall.  Uninstall should only be on when the user specifically selects
it.  Anything else is, IMO, surprising and dangerous.

cgf

Re: Setup

[Robert Collins]

===
----- Original Message ----- 
From: "Robert Collins" <robert.collins@itdomain.com.au>

> I've found the cause, and stopped the silent failure (fix is in
> setup200202). I'm working on correcting the behaviour now.

And a complete fix is in CVS now.

Rob

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>
To: <cygwin-apps@sources.redhat.com>
Sent: Tuesday, February 19, 2002 8:06 AM
Subject: Re: Setup


> Robert Collins wrote:
> > Ok, final feedback and bug-killing time folk.
>
> I just built from CVS and the big thing I noticed in the past and
still seems
> to be there has to do with doing an "Install from Local Directory"
after doing

I've found the cause, and stopped the silent failure (fix is in
setup200202). I'm working on correcting the behaviour now.

Rob

Re: Setup

[Robert Collins]

===
----- Original Message -----
From: "Brian Keener" <bkeener@thesoftwaresource.com>


> Robert Collins wrote:
> > Ok, final feedback and bug-killing time folk.
>
> The other couple of things I've noticed are probably open for
preference so I
> will throw them out for general consensus:
>
> 1) If I have a group of packages installed and I select the
> Test option to see what packages are available for in Test should all
the
> packages I have installed that do not have a Test Version show
Uninstall or
> should they should keep??  It is my contention that thy should show
keep and
> not uninstall - unless I explicitly request uninstall.

I have emailed trying to start a discussion on this at least twice. My
contention is that this is a factor for setup.ini to take care of, not
setup.exe. It's easier to add entries than have complex logic for
removing them when they aren't appropriate.

> 2) If I am doing and "Install from Local Directory" and the Source
file does
> not exist in the Local path should the option for Source only or the
Sources
> checkbox even be available?? It is my contention again that if the
Sources
> cannot be found then the option should not exist.

Patches gratefully accepted.

Rob

Re: Setup

[Brian Keener]

Robert Collins wrote:
> Ok, final feedback and bug-killing time folk.

I just built from CVS and the big thing I noticed in the past and still seems 
to be there has to do with doing an "Install from Local Directory" after doing 
the Download.  It appears the download is working fine but when I select 
Install from Local Directory if I select any package(s) for reinstall then 
setup seems to uninstall just fine but the Reinstall doesn't seem to do 
anything - my log doesn't seem to show any creations but it shows alot of 
unlinks for the removals.  I discovered this while researching another problem 
dealing with install from local directory where setup doesn't seem to install 
the last package selected for upgrade (as opposed to reinstall mentioned 
above).  It seems to download it fine or if I select install from the internet 
that also seems to work - I've been trying to debug it but my skills are 
beginner at best.  I can neither confirm or deny the upgrade problem at the 
moment but I just had the reinstall happen so I can confirm that one.

The other couple of things I've noticed are probably open for preference so I 
will throw them out for general consensus:

1) If I have a group of packages installed and I select the 
Test option to see what packages are available for in Test should all the 
packages I have installed that do not have a Test Version show Uninstall or 
should they should keep??  It is my contention that thy should show keep and 
not uninstall - unless I explicitly request uninstall.  

2) If I am doing and "Install from Local Directory" and the Source file does 
not exist in the Local path should the option for Source only or the Sources 
checkbox even be available?? It is my contention again that if the Sources 
cannot be found then the option should not exist.

Those are the few things I found - if you need a copy of my setup.log.full - 
just let me know.  I'll keep trying on the Reinstall to get a handle on what 
gives but if anyone has any clues you might want to fix it as opposed to 
waiting on me.

bk

Setup

[Robert Collins]

Ok, final feedback and bug-killing time folk.

A big thanks to Michael, Jason, Pavel, Gary, and everyone else who has
helped make this latest incarnation of setup.exe the best yet.

Chuck, your fix is in there :}.
Chris, thank you for humouring me with regards to that... 300 line
changelogs tend to make me grumpy :}.

I've branched 'setup200202' with the string patch + Michaels' patchs +
Jason's off-by-one patch.

The version number on the new setup will be 2.194.2.x where x will be
version specific bug fixes.

No, and I really mean it this time :] wholesale changes to the branch
please. CVS HEAD is no open for development again.

Cheers,
Rob



P.S. Michael : A Changelog is a beautiful thing :}.