961258 - Fail builds if the installed hg version is <= 2.6

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[(no longer active)]

See bug 957562 comment 16 for the rationale.

Comment 1

[Gregory Szorc [:gps]]

This can't be a hard requirement since release automation is still running 2.5 on clients (sadly).

Proposal: we add a configure check that can be disabled with --disable-minimum-hg-version-check (or similar). We add said mozconfig override to the in-tree mozconfigs.

Comment 2

[Gregory Szorc [:gps]]

I filed bug 961279 to upgrade automation clients to 2.7+. I'm not holding my breath.

Comment 3

[(no longer active)]

(In reply to comment #1)
> This can't be a hard requirement since release automation is still running 2.5
> on clients (sadly).

Hmm, they don't run mach though right?  Can we add this check to mach?

Comment 4

[Justin Wood (:Callek)]

I'm also not a fan of this at all due to users able to use git or a tarball in order to build. So needing a specific hg version feels wrong

Comment 5

[(no longer active)]

(In reply to comment #4)
> I'm also not a fan of this at all due to users able to use git or a tarball in
> order to build. So needing a specific hg version feels wrong

Using git is irrelevant here (speaking as a git user myself!).  You have to use hg to do your pushes anyways, and our goal here is to protect old broken hg's from existing on the developers' machines so that they don't use those to push.

Comment 6

[Justin Wood (:Callek)]

Being able to push is not a requirement to build. Also you can fork on virgin your own patches and never push yourself as well. Developers who never want to push not never need to push shouldn't be penalized for that. We've come a long way toward makin entrance to out project easy!  (My first build attempt took me 2 weeks fwiw) I'm advocating to keep sanity for those users who don't yet have push ability. 

A warning for old versions is be ok with, though I expect if it doesn't break the build no one will care given how many warnings a build gives out these days

Comment 7

[Justin Wood (:Callek)]

Err fork on github.  Love auto correct

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should just be testing hg's version if and only if it's installed.  No need to require it to be installed.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Check for Mercurial 2.6+ during configure

Mercurial versions less than 2.6 have a bug where clients can
introduce bad changeset data if they run certain commands. This
bad data can be propagated through pushing.

To minimize the risk of bad pushes in the future, we now check for a
non-buggy Mercurial version in configure. The check only applies if the
source directory is checked out from Mercurial. Furthermore, the check
can be disabled via a configure argument.

Because Mercurial clients in automation are currently running 2.5.4, we
universally disable the check in automation.

https://tbpl.mozilla.org/?tree=Try&rev=b05c0bfce60c

Comment 10

[Gregory Szorc [:gps]]

Hmmm. The Hf build (rooting hazards analysis) isn't picking up the common mozconfig file.

From https://tbpl.mozilla.org/php/getParsedLog.php?id=33199902&tree=Try&full=1, we see:

  FOUND_MOZCONFIG := /builds/slave/l64-br-haz_try_dep-00000000000/build/mozconfig.analysis

But I can't find mozconfig.analysis in the tree nor can I find a reference to this file earlier in the job log. Where is this file coming from? Why isn't it in the tree? I dare say this mozconfig not sourcing build/mozconfig.common is a bug. But I suppose it could be intentional.

Terrence?

Comment 11

[Gregory Szorc [:gps]]

Found it: https://hg.mozilla.org/build/mozharness/file/d416937ec90a/scripts/spidermonkey/build.browser

I argue this should be in the tree. I'll file a bug.

Comment 12

[Justin Wood (:Callek)]

L10n repacks need to also be considered. And again I 100% don't agree with forcing this in order to build

Comment 13

[Gregory Szorc [:gps]]

It's a soft force. I'm not thrilled with it either. But the alternative is people continue to unknowingly push bad data.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8361981 [details] [diff] [review]
Check for Mercurial 2.6+ during configure

Review of attachment 8361981 [details] [diff] [review]:
-----------------------------------------------------------------

I'd rather avoid this and enforce checks at the server level. If I understand correctly the issue this is supposed to avoid, that just means checking changeset metadata (which contains the list of changed files) matches the actual changes to the corresponding manifest. This can be done on the server end without involving the client.

Comment 15

[Gregory Szorc [:gps]]

Let me crawl some repo data and see if I can identify bad changesets. If so, we should be able to a) identify all pushers of bad changesets b) deploy a hook to prevent bad changesets from being pushed

Comment 16

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #15)
> Let me crawl some repo data and see if I can identify bad changesets. If so,
> we should be able to a) identify all pushers of bad changesets b) deploy a
> hook to prevent bad changesets from being pushed

I have a hook already. I forgot to file it.

Comment 17

[Gregory Szorc [:gps]]

Here's what I wrote:

@command('scan-bad-changesets', [], _('foo bar'))
def scan_bad_changesets(ui, repo):
    for rev in repo:
        ui.progress('changeset', rev, total=len(repo))
        ctx = repo[rev]
        files = set(ctx.files())
        expected = set()

        m = ctx.manifest()

        for pctx in ctx.parents():
            pm = pctx.manifest()

            for k, v in pm.items():
                if k not in m:
                    expected.add(k)
                    continue
                if m[k] != v:
                    expected.add(k)
                    continue

            for k in m:
                if k not in pm:
                    expected.add(k)

        if files != expected:
            ui.write('%d:%s\n' % (ctx.rev(), ctx.hex()))

Comment 18

[Mike Hommey [:glandium]]

That probably misidentifies merges and is going to be very slow, especially when sheriffs push merges with hundreds of changesets. I know, my initial implementation kind of looked like that and took 2 minutes for ~700 changesets.

Comment 19

[Mike Hommey [:glandium]]

Filed bug 962300