sync-all: END actions result in confusing error message

The instructions at Newcomers were slightly outdated, and result in a rather confusing error message:

$ ./sync-all --testsuite get
Unrecognised flag: --testsuite at ./sync-all line 872.
== Checking for old haddock repo
== Checking for old binary repo
== Checking for old mtl repo
== Checking for old Cabal repo
== Checking for old time from tarball
============================
ATTENTION!

You have an old time package in your GHC tree!

Please remove it (e.g. "rm -r libraries/time"), and then run
"./sync-all get" to get the new repository.
============================
== Checking for obsolete Git repo URL
$

The patch suppresses the misleading error message. I've already removed the --testsuite flag from the wiki page.

Trac metadata

Trac field	Value
Version	7.6.3
Type	Bug
TypeOfFailure	BuildingGhcFailed
Priority	normal
Resolution	Unresolved
Component	Build System
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed weight to 5

added Tbug Trac import ghc build failure labels

Attached file 0001-sync-all-Skip-END-actions-on-exceptions.patch (download).

Trac metadata

Trac field	Value
Version	7.6.3 →

closed

Merged, thanks!

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

reopened

assigned to @hvr

This commit should be reverted IMHO.

The idea of using the END block, if I understand correctly, is that it should run at the end, no matter what else happens. For example the change back to the initial_working_directory should not be skipped in case of errors.

Trac metadata

Trac field	Value
TypeOfFailure	BuildingGhcFailed → OtherFailure
Resolution	ResolvedFixed → Unresolved
Component	Build System → Trac & Git

added infrastructure label and removed ghc build failure label

Attached file 0001-Check-if-file-is-present-instead-of-directory.patch (download).

The false warning is also triggered when running a different sync-all command before the first succesful sync-all get, for example if one runs (for whatever reason):

git clone git://github.com/ghc/ghc

cd ghc

1. /sync-all -r http://git.haskell.org remote set-url origin

The attached patch proposes a solution.

Replying to [ticket:8886#comment:84136 thomie]:

The idea of using the END block, if I understand correctly, is that it should run at the end, no matter what else happens. For example the change back to the initial_working_directory should not be skipped in case of errors.

If the script dies with an error, the process is probably in a slightly broke state, and printing all those warnings only obscures the error. Changing the directory does not in itself have any effect because the process is about to terminate anyway, and its not the point of the END action.

Ok, you're right that changing the directory is not needed.

But one thing I don't understand: why are those checks in the END block, if not for running even on an error. Wouldn't they be better placed straight in the main function then? Would make things simpler. I guess the author of those old repository checks in the END block thought about potential errors that could occur, and deemed it necessary to show those warnings. Yes it obscures the initial error, but maybe that error is caused by having an old repository in the first place.

The bug that still stands is getting false warnings about old time library being present when running another command before sync-all get. My patch fixes that.

Replying to [ticket:8886#comment:84151 thomie]:

Ok, you're right that changing the directory is not needed.

But one thing I don't understand: why are those checks in the END block, if not for running even on an error. Wouldn't they be better placed straight in the main function then?

Probably, I don't understand they way the script is written. I have seen some Perl in my day, and this usage of BEGIN (for argument parsing, of all) and END blocks seemed highly suspect to me. I didn't want to audit the code for any forms of non-local control flow, so I put in just the exception check as a minimal, less risky change.

mentioned in commit 7012ed85

closed

Merged.

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

changed milestone to %7.10.1

mentioned in commit 0f31c2e5

added ghc build failure label

added Pnormal label