Opened 12 months ago

Closed 6 months ago

Last modified 6 months ago

#8886 closed bug (fixed)

sync-all: END actions result in confusing error message

Reported by: fw Owned by: hvr
Priority: normal Milestone: 7.10.1
Component: Trac & Git Version:
Keywords: sync-all Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

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.

Attachments (2)

Change History (14)

comment:1 Changed 12 months ago by fw

  • Version 7.6.3 deleted

comment:2 Changed 12 months ago by fw

  • Status changed from new to patch

comment:3 Changed 11 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Merged, thanks!

comment:4 follow-up: Changed 8 months ago by thomie

  • Component changed from Build System to Trac & Git
  • Keywords sync-all added
  • Owner set to hvr
  • Resolution fixed deleted
  • Status changed from closed to new
  • Type of failure changed from Building GHC failed to None/Unknown

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.

comment:5 Changed 8 months ago by thomie

  • Status changed from new to patch

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
./sync-all -r http://git.haskell.org remote set-url origin

The attached patch proposes a solution.

comment:6 in reply to: ↑ 4 Changed 8 months ago by fw

Replying to 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.

comment:7 follow-up: Changed 8 months ago by 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? 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.

comment:8 in reply to: ↑ 7 Changed 8 months ago by fw

Replying to 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.

comment:9 Changed 6 months ago by Austin Seipp <austin@…>

In 7012ed8515100b4947383e93b82dbff7a0aa835c/ghc:

Check if file is present instead of directory

Fixes #8886.

Problem: any `sync-all` command that is run before the first succesfull
`sync-all get` would trigger a false warning about an old `time` package
being present.

Cause: after cloning the ghc repository, the (empty) `libraries/time`
directory is already present.

Solution: check if the directory actually contains any of the `time`
files. I picked the `LICENSE` file, since `boot` does so as well.

Signed-off-by: Austin Seipp <[email protected]>

comment:10 Changed 6 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Merged.

comment:11 Changed 6 months ago by thoughtpolice

  • Milestone set to 7.10.1

comment:12 Changed 6 months ago by Austin Seipp <austin@…>

In 0f31c2e5c1cf240a78221bb09578f6eb7084ada5/ghc:

Cleanup and better documentation of sync-all script

Summary:
Rumor has it that sync-all is slowly on the way out. Now that all
subrepositories have been turned into git submodules, sync-all might
not be needed anymore. Nevertheless, here are some changes I had made
while trying to understand why it existed in the first place:

* update comments + help text
* rename some variables for maintainability
    * s/branch_name/remote_name/
    origin is the name of a remote, not a branch

    * s/repo_base/remote_root/
    the word *remote* is key here

    * s/defaultrepo/default_root/
    this was a darcsism, and it doesn't refer to a repository but to the
    root directory of all repositories
* small tweaks
* .git can be a file nowadays
* don't skip END actions on exceptions #8886
  reverts d523f9b3d4ce3463e8816cad2139ea397e00f8d1

Test Plan:
Why revert d523f9b3d4ce3463e8816cad2139ea397e00f8d1?

I put an old haddock repository from
http://darcs.haskell.org/haddock2.git back in my tree. Now, when running
`sync-all get`, the following happens:

1. I get a cryptic error saying:

    fatal: reference is not a tree:
    5412c262f403e52be45d607b34eb3a5806ea2a76
    Unable to checkout '5412c262f403e52be45d607b34eb3a5806ea2a76' in
    submodule path 'utils/haddock'
    git failed: 256 at ./sync-all line 112.

2. sync-all checks if maybe an old haddock repository is present

3. I get a clear warning saying:

    ============================
    ATTENTION!

    You have an old haddock repository in your GHC tree!

    Please remove it (e.g. "rm -r utils/haddock"), and then run
    "./sync-all get" to get the new repository.
    ============================

Without commit d523f9b3d4ce3463e8816cad2139ea397e00f8d1 reverted, steps
2 and 3 were skipped. The problem that commit tried to solve,
is now solved with 7012ed8515100b4947383e93b82dbff7a0aa835c.

Reviewers: nomeata, austin, hvr

Reviewed By: austin, hvr

Subscribers: simonmar, ezyang, carter

Differential Revision: https://phabricator.haskell.org/D178

GHC Trac Issues: #8886, #9212
Note: See TracTickets for help on using tickets.