This is the mail archive of the
cygwin-apps
mailing list for the Cygwin project.
Re: [PATCH] Incidental setup.exe patches #3: Simplify packagedb task handling
On 14/04/2010 09:21, Corinna Vinschen wrote:
>> [ ... ] I couldn't work
>> out what the original problem referred to might have been
> Are you sure? Please check again. The original situation which this
> was supposed to fix is this:
>
> - You have an existing installation in C:\cygwin
> - You want to create a new installation in D:\cygwin-new
> - You choose D:\cygwin-new as root dir in the root dir dialog
> - The package list was nevertheless based on the c:\cygwin directory
> installation, because the package_db stuff was already initialized
> with the Cygwin root dir set to C:\cygwin
Thanks, that's just what I was looking for.
> Can you make sure that this doesn't happen after your change?
Argh, zOMG. See, I did this:
> $ grep -w task *.[ch]*
> archive.cc: * One such task is identifying archives
> compress.cc: * One such task is identifying compresss
> package_db.cc: packagedb::task =
> package_db.h: static PackageDBActions task;
> package_meta.cc: return db.task == PackageDB_Install ? "Reinstall" : "Retri
> eve";
> root.cc: db.task = chosen_db_task;
> threebar.cc: switch (task)
> threebar.cc: Window::PostMessage (task);
> threebar.h: int task;
> threebar.h: task = t;
.... and reasoned that there really is only one place the behaviour of the
application can possibly depend on the value set in task. Then I looked at this:
> - /* Deferred initialization of packagedb *after* the root dir has been
> - chosen. */
> - packagedb db;
> - db.task = chosen_db_task;
and thought "Oh, and we don't need to actually construct a packagedb and throw
it away again just to access a static member".
*Sigh*, I should have read what the constructor does. Wow, what a
misbegotten application of OOP that is. A pseudo-singleton that stores all
the data in class static members the first time you construct one and
retrieves it for any subsequently constructed ones. That's probably the first
non-idempotent default constructor I've ever seen.
So:
1. There's no problem setting the class-static task variable early: it's just
constructing the first packagedb that mustn't be done until after the root is set.
1a. So we can still get rid of the caching-and-deferred-initialisation. The
original bug was solved by /not/ constructing a packagedb at the original site
where the task was set, rather than specifically by constructing one later.
1b. It never needed to construct a packagedb at that point anyway, because you
don't need a this-pointer to set a static member; it was that misunderstanding
that caused the bug in the first place.
2. There's no need to construct a packagedb immediately after the root dir is
set; whenever we first construct one subsequently, it'll work.
3. For that reason the patch ought to fix the original bug in any case.
I have to run out to the shops for half an hour now, and when I get back
I'll fire up a VM, and make absolutely certain.
cheers,
DaveK