1429094 - mach bootstrap doesn't detect the correct mercurial version in some cases

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Julien Wajsberg [:julienw]]

The full output of my "hg --version" is

(third party extension reviewboard requires version 4.1 or newer of Mercurial; disabling)
Mercurial Distributed SCM (version 4.0)
(see https://mercurial-scm.org for more information)

Copyright (C) 2005-2016 Matt Mackall and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Notice the first line.
As a result `mach bootstrap` finds "4.1" as version, instead of "4.0".

version-control-tools doesn't have the issue because it uses directly the python library "mercurial" to get the version.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: 0001-Bug-1429094-Change-the-regular-expression-that-finds.patch

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8941092 [details] [diff] [review]
0001-Bug-1429094-Change-the-regular-expression-that-finds.patch

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

Thanks for catching this.

::: python/mozboot/mozboot/base.py
@@ +464,5 @@
>          if not hg:
>              print(NO_MERCURIAL)
>              return False, False, None
>  
> +        our = self._parse_version(hg, '\\(version', self._hgplain_env())

Do you think it's:

a) worth adding even more context, e.g. 'Mercurial Distributed SCM \\(version'; or
b) adding a comment as to why we're matching a lone open paren here?

Comment 3

[Julien Wajsberg [:julienw]]

(In reply to Nathan Froyd [:froydnj] from comment #2)

> a) worth adding even more context, e.g. 'Mercurial Distributed SCM
> \\(version'; or

Ideally we should test with older versions too; are we sure the text hasn't changed ?

> b) adding a comment as to why we're matching a lone open paren here?

I can do that!


I just found out that an alternative solution is setting `HGRCPATH` to an empty string. In my setup at least, `HGRCPATH= hg --version` doesn't show the offending line. It could be more error-proof, what do you think?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> 
> > a) worth adding even more context, e.g. 'Mercurial Distributed SCM
> > \\(version'; or
> 
> Ideally we should test with older versions too; are we sure the text hasn't
> changed ?

I don't know!  We don't have anything resembling a test suite for `mach bootstrap`, other than users. =/

> > b) adding a comment as to why we're matching a lone open paren here?
> 
> I can do that!

\o/

> I just found out that an alternative solution is setting `HGRCPATH` to an
> empty string. In my setup at least, `HGRCPATH= hg --version` doesn't show
> the offending line. It could be more error-proof, what do you think?

I think if we could use that in _parse_version, that would be the best solution.  We have talked about using it other places in `mach bootstrap`, maybe this is a good first step to doing that!

Comment 5

[Julien Wajsberg [:julienw]]

So even with an empty value, the repository's .hg/hgrc is used. He's man page says we can use an empty file or /dev/null. I tried using the latter and it works for me on Linux, but I have no idea whether this works on MacOS or Windows. Maybe adding an empty hgrc file to the repository and using it could be a better idea. What's your suggestion ? Thanks !

Comment 6

[Mike Hommey [:glandium]]

Note, neither the original code nor the new code deal with mercurial not being in english. Specifically, on my machine, if I run hg --version, I get: Mercurial - 分散構成管理ツール(バージョン 4.4.1).

Comment 7

[Julien Wajsberg [:julienw]]

The environment variable HGPLAIN should make it work, at least this is the intent.

Comment 8

[Gregory Szorc [:gps]]

Yes, HGPLAIN should remove localization.

I'd also favor adding HGRCPATH='' just for the `hg --version` invocation so that stderr output about the extension load failure isn't present. Alternatively, we could not redirect stderr into stdout so stderr output doesn't come into play. Just as long as stderr is being suppressed, we should be fine.

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: Bug 1429094 - Define the environment variable HGRCPATH to ignore any .hgrc that may exist

By ignoring existing .hgrc we make sure we don't try to load extensions
and have a consistent output. Especially we won't have error messages
that could confuse us into extracting a wrong version number.

Review commit: https://reviewboard.mozilla.org/r/211798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211798/

Comment 10

[Julien Wajsberg [:julienw]]

I added HGRCPATH=/dev/null because I think it's even more error-proof. But I want to test it on Windows (tomorrow). If this is broken in Windows I'll use to the empty string and request a review.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8941528 [details]
Bug 1429094 - Define the environment variable HGRCPATH to ignore any .hgrc that may exist

https://reviewboard.mozilla.org/r/211798/#review217596

This needs a fix to not use `/dev/null`. But otherwise this looks good.

::: python/mozboot/mozboot/base.py:462
(Diff revision 1)
> +        HGRCPATH controls the loading of hgrc files, setting it to /dev/null
> +        forces that no hgrc is used at all.
>          """
>          env = os.environ.copy()
>          env[b'HGPLAIN'] = b'1'
> +        env[b'HGRCPATH'] = b'/dev/null'

An empty value is the same as `/dev/null`. Since `/dev/null` won't work on Windows, you should use the empty value.

Comment 12

[Julien Wajsberg [:julienw]]

> An empty value is the same as `/dev/null`. Since `/dev/null` won't work on Windows, you should use the empty value.

OK thanks. I used this value because in the hg man page, if you search for HGRCPATH, you get 2 different explanations in 2 different parts of the man page:

first:
If empty, only the .hg/hgrc from the current repository is read.

but later:
In rare cases, the value can be set to an empty file or the null device (often /dev/null) to bypass loading of any user or system config files.

So confusedly I thought that using /dev/null would also ignore .hg/hgrc.


But of course you know the source code :) so I'll change this as you say and request a review.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8941528 [details]
Bug 1429094 - Define the environment variable HGRCPATH to ignore any .hgrc that may exist

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211798/diff/1-2/

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8941528 [details]
Bug 1429094 - Define the environment variable HGRCPATH to ignore any .hgrc that may exist

https://reviewboard.mozilla.org/r/211798/#review218992

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/195e88aab631
Define the environment variable HGRCPATH to ignore any .hgrc that may exist r=gps

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/195e88aab631

Comment 17

[Luke Wagner [:luke]]

I just tried installing from scratch and, running 'mach bootstrap' the first time, I had the following error which was resolved by reverting to the cset before this bug (f5c4825b6ae8).  The error was:


Your version of Mercurial (4.4.2) is sufficiently modern.
Your version of Python (2.7.12) is new enough.
Your version of Rust (1.23.0) is new enough.
Rust supports x86_64-unknown-linux-gnu targets.
Error running mach:

    ['bootstrap']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

AttributeError: 'DebianBootstrapper' object has no attribute '_hgplain_env'

  File "/moz/mi/python/mozboot/mozboot/mach_commands.py", line 32, in bootstrap
    bootstrapper.bootstrap()
  File "/moz/mi/python/mozboot/mozboot/bootstrap.py", line 324, in bootstrap
    env=self.instance._hgplain_env(),

Comment 18

[Gregory Szorc [:gps]]

Doh. Patch to fix is up in bug 1431226. Thanks for reporting.