Re: setup.exe rebase patch

[Jason Tishler <jason at tishler dot net>]

Rob,

On Sun, Feb 24, 2002 at 10:45:32PM +1100, Robert Collins wrote:
> ----- Original Message -----
> From: "Jason Tishler" <jason@tishler.net>
> 
> Some ideas follow:
> 
> > Attached is a patch that adds the rebase functionality to setup.exe.
> > I would like to get some feedback before I start to resolve the
> > following issues:
> >
> >     o How to handle a missing config file (i.e., /etc/setup/rebase.conf)?
> 
> Create one with sensible defaults. If no sensible defaults exist, add a
> new tab.

It is easy to arrive at one with sensible defaults.  What I was "really"
trying to ask above was the following:

    o What should create this file?  A postinstall script, a new rebase
      package, etc?
    o What should setup.exe do if the file is missing?  Proceed without
      rebasing (i.e., ignore missing rebase.conf), create a new (default)
      one, complain and exit, etc?

I guess that I was trying to ask a more philosophical question of how
coupled rebase was going to be with setup.exe.  It sounds like you are
comfortable with a tight coupling.  Is this correct?

> >     o Need to handle in-use files.
> 
> Copy the file, rebase it, schedule for reboot replacement (see
> install.cc for the mechanics).

I already had a plan for the above -- I just didn't want to implement it
until I got some feedback as to whether or not my rebase approach was
acceptable.  What I posted was more a proof of concept or prototype than
a finished patch.  I wanted to make sure that I wasn't off in "left
field" before I started to dot I's and cross T's.

> >     o How to handle corruptible (by rebasing) files?
> 
> Hmm. Can we detect the corruption?

Yes, objdump can, so...

> Does OpenLibrary fail or something nicely obvious like that?

I haven't tried LoadLibrary(), etc. but I presume that one of the
APIs can.  However, isn't this too little, too late?  What can we do
when the corruption occurs?  Rollback to the copy in the archive?  But,
then this image will *not* be rebased. :,(

I would like to propose the following alternative -- at least until
someone figures out how to prevent the rebase corruption problem.  To
refresh your memory, it seems that some *stripped* images  are not
rebasable, but "all" unstripped one are.

I recommend adding the requirement to setup.html that requires package
maintainer to test whether or not their stripped relocatable images
(e.g., DLLs) are rebasable.  If not, then they should release their
packages with those files unstripped.

For example, the Python DLLs are rebasable after stripping, so I
don't need to do anything special for the Python package.  However,
the PostgreSQL DLLs are not, so I need to release PostgreSQL with
*unstripped* DLLs.

Does the above sound like a reasonable interim solution?

> >     o How should dependent DLLs be handled?
> 
> Recursively, as long as they are in the cygwin tree.

Is this really necessary?  If we are going to only rebase Cygwin DLLs,
then the dependencies will ultimately be rebased when their package is
(re)installed.  If so, does setup.exe install dependencies first or
just alphabetically?  If alphabetically, then the dependent DLLs may
not be on disk yet and hence, cannot be rebased at the moment.

Regardless of the resolution to the above, can we declare this a second
order item to be dealt with during subsequent iterations?

> > and some other more niggling items.
> 
> Can you list them?

Here is my (dirty) laundry list so far:

    Deal with in-use files.
    Get config file prefix from registry.
    Warn on unrebases if base and/or size does not match used list entry.
    Refactor free_list and used_list to use common base class.
    Refactor used_list::find() and used_list::remove().
    Add dirty flag and write only if dirty.

I don't feel that any of them are shop stoppers.

> > I re-installed 30 out of the 33 packages that contain DLLs and my
> system
> > is functioning normally.  Python can even fork too!
> 
> Cool. This will probably help Ralf's KDE project too.
> 
> > I also have a stand-alone rebase.exe that I would like to contribute
> to
> > Cygwin so that users can rebase without having to run setup.exe.
> 
> Neato. This sounds like a cygutils thingo to me, or a new package.
> Chuck? Ohhhh Chuck?

See my (forthcoming) response to Chuck's email.

> One important thing is to have setup's rebasing and the rebase
> commandline tool *both* store their work in rebase.conf. That way they
> won't require full rebasing when someone fiddles with something.

This is already done!

In fact, since I posted my patch, I have been "eating my own dog
food."  I religiously have been using either a setup.exe with the
rebase functionality or my stand-alone rebase (e.g., for snapshots,
CVS builds, etc.) for all (rebase tolerant) installs on my main system.
I felt strongly that I should live with this "solution" before sic-ing
it on the general public.  So far, so good.

> I've read through your patch. Can I ask you to redo it against HEAD?
> (After updating cygpath etc to the String class.

Yes, I guess that this is good news?

> Some feedback/thoughts on the current patch (not including what will
> change during updating).
> 
> 1) log.cc - don't clean up there. If you need to cleanup, have a static
> object whose destructor should get called.

This is already done too (see rebaser.cc:338).  Note that my stand-alone
rebase.exe is already using this successfully.

> It's on my mental todo list
> to find out why the destructors aren't called (even if exit() is used
> :--[).

The above is why I called the clean-up routine explicitly.

> Like the initialisation of globals issue a while back, lets not
> work around a compiler fault, but rather address the real issue.

However, I thought that it was an artifact of calling ExitProcess()
instead of exit().

> In your
> case, a trivial wrapper (ie rebaser_deleter) that wraps the instance
                              ^^^^^^^^^^^^^^^
> pointer should do it.

Hmm...  Where did you get the above name?  rebaser.cc:338 perchance? :,)

> I also note the need to save the .conf file, I'd
> suggest that that occurs at the end of the install process, not when the
> user exits setup. (Setup might crash, they might do another run, run a
> command line rebase...)

Agreed, I had this concern too.  I will add this to my todo list.

> 2) I really don't like the rebaser::set_cygwin_root_dir construct: why
> not use get_root_dir () when you need the cygwin root? (Any why do you
> need it? Surely you only need cygpath (filename) ?

This is for the stand-alone case which I wasn't sure was going to be
able to easily share code with setup.exe.  This concern appears to
be mitigated.  I can certainly have rebaser always retrieve the Cygwin
root from the registry if this is deemed the better approach.

> 3) config file should use io_streams, not stdio please.

Sigh...  Oh, where oh where has my little standard C++ library gone?
Oh, where oh where has it gone? :,)

I only used stdio because I developed the stand-alone rebase first to
speed development.  I had always intended to replace the *printf() calls
during integration.

I have adding this to my todo list.

> 4) New errors should go into the resource.rc stringtable. I'm guilty of
> this myself, but no need to let it get worse :}.

I presume that you mean the *printf() messages?  I have added this to my
todo list.  However, will this be OK for the stand-alone version?  That
is the stand-alone version a console app while setup.exe is a GUI one.

> 5) If you've a complex .conf file, you might want to write a .y and .l
> combo for it instead of manually parsing.

Sorry, I don't have any experience with YACC and lex.  But, I will learn
how to use them if it deemed necessary.

Can this be handled as a second order item too?

BTW, I have been using the silly MS .ini file format.  I am quite open to
suggestions regarding better formats.

> 6) config_file_reader has a static buffer (errk).

Agreed that this is ugly -- in fact, this is the one area that I'm least
happy with.  Note I didn't want to get bogged down with parsing issues
during the proof of concept phase but I have added this to my list too.
Since this is encapsulated I can easily change this implementation
without any rippling effects.

What I needed is push back (onto the input stream) functionality.  Any
suggestions on a cleaner implementation is appreciated.

> 7) I don't really grok your class hierarchy. Can you please explain it?

There is not much to it so I will attempt to explain it textually.  I can
send you a UML (i.e., Visio) diagram, if necessary.

The rebaser class uses the singleton design pattern.  This seemed
appropriate and minimized the impact of adding the rebase functionality
to setup.exe.  rebaser uses (as in has-a) free_list and used_list.
rebase also uses config_file_reader and config_file_writer.
config_file_reader and config_file_writer inherit from config_file.

That's it -- there is not much to grok.  Although, I readily admit that
trying to understand new code without any comments and no documentation
is never fun (and easy).

> (I don't want to criticize or suggest alterations until I do grok it).
> It *feels* procedural to me.

Sniff, sniff -- you used the P word! :,(

I tried to find the appropriate abstractions in the problem domain.
I tried to encapsulate implementations behind reasonable interfaces to
minimize the impact of implementations changes.  Etc, etc.  However,
I'm open to design changes too, if deemed necessary.

> 8) You seem to duplicate some io_stream functionality - i.e.
> config_file_writer::rename. I don't see why a if (io_stream::rename
> (temp_filename,real_filename) isn't enough.

Again due to stand-alone development -- to be dealt with during
integration.

> 9) Should we rename my 'list.h' to 'vector.h'

vector does seem to better capture the semantics.  But, what is the
relevance to my list code?  Are you suggesting that I use your stuff.
I tried but I needed STL map-like functionality.

> and generalise your list handling code into a template?

It's already on my todo list.  However, I wish that I never had to write
this kind of code.  Besides being a waste of time, there is no way that
I could ever come close to what the STL developers achieved.

> 10) Please capitalise the first letter of words in class names. Setup
> has been quite haphazard, I'm trying to be more consistent :}. Also, I
> prefer things like FreeList to free_list - I find them easier.

The above made me smile!  I completely agree with you.  However, I was
just trying to follow the GNU coding style.  Specifically, the following
item is relevant here:

    http://www.gnu.org/prep/standards_26.html#SEC26

> 11) All .h files should compile correctly if they are the only
> translation unit. I'm not sure if that is or is not the case for your
> files just now. Again, this is something I don't expect you to know, as
> I'm still introducing it into setup.

OK, I didn't know this.  How does one test this?  I just tried and g++
doesn't seem to like to compile *.h files alone.

> Hope thats enough feedback for ya!

Yes!  Thank you very much for your time.

Jason