Setup showstopper

[Max Bowsher]

Dave Korn wrote:
>     Hey all,
>
>  As long as we're talking about setup, let me raise the one I've just run
> into:
>
>  When there's an error validating the checksum of a package (to be 
> precise,
> when validateCachedPackage() returns an error indication to
> check_for_cached() in download.cc), setup raises an exception and aborts,
> because there's no handler to catch it.  For example:
>
> Fatal Error: Uncaught Exception
> Thread: DialogProc
> Type: 9Exception
> Message: Package validation failure for
> file:///...path..to.../release/X11/fontconfig/libfontconfig-devel/libfontcon
> fig-devel-2.2.2-1.tar.bz2
> AppErrNo: 1
>
>  This isn't very helpful, as it will only fail again in the same way next
> time it gets to try and validate that package.
>
>  In other words, a validation error becomes permanent and fatal and 
> there's
> no way to ever recover/repair/upgrade/revert your cygwin installation
> without manually going into the local package dir, finding the relevant
> file, and getting rid of it.  Which is not newbie-friendly.  (It can of
> course be worked around with --no-md5, but no newbie is going to type 
> "setup
> --help" and guess where the hell the output has gone!).

Known issue, though currently, I'm not sure what the best UI for this 
situation is.

> 1)  Wouldn't the best thing be to just delete (or rename-aside, or even
> ignore) the apparently-corrupt package?

Yes. I think probably "rename-aside, plus notify the user" is probably the 
best way to go.

> 2)  check_for_cached is supposed to return 0 for failure.  Why is it
> throwing an exception?

There is a difference between "not cached" and "corrupt".

> 3)  The functions download_one and do_download_thread, which call
> check_for_cached, do in fact wrap the call in try...except handlers, but 
> as
> soon as they catch the exception they pass it straight to fatal (...); why
> should it be regarded as a fatal error?

All exceptions are fatal until someone has designed a better way to handle 
them.

> 4)  The only function to call check_for_cached as an extern is
> packageversion::scan in package_version.h; this function fails to wrap it 
> in
> any kind of exception handler at all, which is presumably because someone
> looked at the check_for_cached function, saw the comment about "Return 0 
> for
> failure", and didn't look through the source and see that it might also
> raise an exception.

The exceptions were added after that package_version.h code was written. 
Setup's code is in flux between design patterns.

>  So, what do you reckon?  Should I make check_for_cached just quietly
> return zero if the package validates wrong?  That would (I think) have the
> effect of making setup redownload the package and overwrite the old 
> corrupt
> one, wouldn't it?

I think at very least we should tell the user.

>  Or should I leave it to the callers to decide what to do, but perhaps 
> find
> a better error return mechanism (zero for 'not found', -ve for 'error', 
> +ve
> for 'success' perhaps?) that's more logical and maintainable?

I don't think that is logical or maintainable.

Either an enum of possibilities, or a simple bool, with exceptions for 
exceptional cases.

BTW, I am working on this too. There are additional complications... like 
why the local package scan is being run in "Download" and "Local" modes, but 
not in "Install" mode.

Max.