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

RESOLVED FIXED in Firefox 59

Status

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: julienw, Assigned: julienw)

Tracking

Trunk
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #8941092 - Flags: review?(core-build-config-reviews)
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?
Attachment #8941092 - Flags: review?(core-build-config-reviews) → review+
(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?
Assignee: nobody → felash
Flags: needinfo?(nfroyd)
(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!
Flags: needinfo?(nfroyd)
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 !
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).
The environment variable HGPLAIN should make it work, at least this is the intent.
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.
Attachment #8941092 - Attachment is obsolete: true
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 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.
Attachment #8941528 - Flags: review-
> 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.
Attachment #8941528 - Flags: review?(gps) → review?(core-build-config-reviews)
Attachment #8941528 - Flags: review?(core-build-config-reviews) → review?(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
Attachment #8941528 - Flags: review?(gps) → review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/195e88aab631
Define the environment variable HGRCPATH to ignore any .hgrc that may exist r=gps
https://hg.mozilla.org/mozilla-central/rev/195e88aab631
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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(),
Blocks: 1431226
Doh. Patch to fix is up in bug 1431226. Thanks for reporting.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.