1393242 - Misc mozversioncontrol improvements

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

Patches forthcoming.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Helper function to obtain a repository from a build config;

Some callers already have a build config object. Let's not
make them call a function that imports buildconfig.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Return a Repository using configure preferences;

If configure has defined VCS binaries, we should use those.

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Vendor python-hglib 2.4;

python-hglib is a Python client for Mercurial's command server. It
facilitates querying Mercurial efficiently (using a single process)
and without having to parse output in the common case.

Let's vendor it so we can make use of it for more advanced Mercurial
scenarios.

Content vendored from changeset 820d7c1e470a without modifications
(other than deleting unwanted files).

As part of vendoring, we add the package to the virtualenv and make
it available to mach.

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Hook up hglib to HgRepository;

Because hglib spawns a persistent process, we introduce a context
manager for Repository. It no-ops by default. On HgRepository it
controls the lifetime of the persistent hg process.

A helper method for running an hg command via hglib has been added.
We can't transition existing methods to hglib because hglib
requires a context manager, which no consumer is using yet.

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

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Use hglib for get_files_in_working_directory();

And convert consumers to context managers because hglib requires that.

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

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 8900524 [details]
Bug 1393242 - Helper function to obtain a repository from a build config;

https://reviewboard.mozilla.org/r/171906/#review177584

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 8900525 [details]
Bug 1393242 - Return a Repository using configure preferences;

https://reviewboard.mozilla.org/r/171908/#review177586

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 8900526 [details]
Bug 1393242 - Vendor python-hglib 2.4;

https://reviewboard.mozilla.org/r/171910/#review177588

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

https://reviewboard.mozilla.org/r/171912/#review177592

::: python/mozversioncontrol/mozversioncontrol/__init__.py:129
(Diff revision 1)
> +        import hglib.client
> +
>          super(HgRepository, self).__init__(path, tool=hg)
>          self._env[b'HGPLAIN'] = b'1'
>  
> +        # Setting this is a global change. But since the tool was validated

What do you mean it's a "global change"? Would another process outside of m-c that uses hglib talk to the same server and be impacted by this setting?

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

https://reviewboard.mozilla.org/r/171914/#review177598

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

https://reviewboard.mozilla.org/r/171912/#review177592

> What do you mean it's a "global change"? Would another process outside of m-c that uses hglib talk to the same server and be impacted by this setting?

Modified a global variable for the Python process. I'll update the comment.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

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

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0279960d71ab
Helper function to obtain a repository from a build config; r=mshal
https://hg.mozilla.org/integration/autoland/rev/217260fb6976
Return a Repository using configure preferences; r=mshal
https://hg.mozilla.org/integration/autoland/rev/159028ef12d2
Vendor python-hglib 2.4; r=mshal
https://hg.mozilla.org/integration/autoland/rev/ffe8bef74be0
Hook up hglib to HgRepository; r=mshal
https://hg.mozilla.org/integration/autoland/rev/18d89d8ab860
Use hglib for get_files_in_working_directory(); r=mshal

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/299de9e2cdab
Backed out changesets 18d89d8ab860, ffe8bef74be0, 159028ef12d2, 217260fb6976, 0279960d71ab

Comment 16

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Make hglib tests more resilient;

These tests weren't running in automation because hglib wasn't
available. An upcoming commit will vendor hglib. This exposed that the
tests can fail if ui.username isn't set.

In this commit, we introduce a helper function to obtain an hglib
client with ui.username set. We also convert tests to use the context
manager form of the client so resources are cleaned up immediately
without relying on refcounting or garbage collection.

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

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8900524 [details]
Bug 1393242 - Helper function to obtain a repository from a build config;

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

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8900525 [details]
Bug 1393242 - Return a Repository using configure preferences;

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8900526 [details]
Bug 1393242 - Vendor python-hglib 2.4;

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

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

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

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

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

Comment 22

[Michael Shal [:mshal]]

Comment on attachment 8902788 [details]
Bug 1393242 - Make hglib tests more resilient;

https://reviewboard.mozilla.org/r/174452/#review179982

Looks reasonable to me, but try still seems unhappy on Windows. (And Linux has some check_spidermonkey_style errors, which I can't tell if they're related)

Comment 23

[Gregory Szorc [:gps]]

Attachment: Bug 1393242 - Normalize paths in MercurialRevisionFinder;

This ensures we use forward slashes, even if Mercurial emits
backslashes (which it can do on Windows).

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

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8900524 [details]
Bug 1393242 - Helper function to obtain a repository from a build config;

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

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8900525 [details]
Bug 1393242 - Return a Repository using configure preferences;

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

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8902788 [details]
Bug 1393242 - Make hglib tests more resilient;

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

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8900526 [details]
Bug 1393242 - Vendor python-hglib 2.4;

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

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171912/diff/3-4/

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171914/diff/3-4/

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

https://reviewboard.mozilla.org/r/171914/#review180098

This series is still failing in CI. I'm unsure why. Having trouble reproducing locally :/

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171912/diff/4-5/

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171914/diff/4-5/

Comment 33

[Michael Shal [:mshal]]

Comment on attachment 8902788 [details]
Bug 1393242 - Make hglib tests more resilient;

https://reviewboard.mozilla.org/r/174452/#review180102

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

https://reviewboard.mozilla.org/r/171912/#review180104

::: python/mozversioncontrol/mozversioncontrol/__init__.py:150
(Diff revisions 4 - 5)
> +            # The cwd if the spawned process should be the repo root to ensure
> +            # relative paths are normalized to it.
> +            old_cwd = os.getcwd()
> +            try:
> +                os.chdir(self.path)
> -            self._client.open()
> +                self._client.open()
> +            finally:
> +                os.chdir(old_cwd)

I think this will fix the SpiderMonkey test failures.

We may still have a failure due to a bug in old Python not liking unicode types in an environment variable dictionary.

Comment 35

[Michael Shal [:mshal]]

Comment on attachment 8903241 [details]
Bug 1393242 - Normalize paths in MercurialRevisionFinder;

https://reviewboard.mozilla.org/r/175034/#review180106

LGTM, as long as CI is happy :)

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8902788 [details]
Bug 1393242 - Make hglib tests more resilient;

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

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8903241 [details]
Bug 1393242 - Normalize paths in MercurialRevisionFinder;

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

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8900526 [details]
Bug 1393242 - Vendor python-hglib 2.4;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171910/diff/3-4/

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171912/diff/5-6/

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171914/diff/5-6/

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8902788 [details]
Bug 1393242 - Make hglib tests more resilient;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174452/diff/3-4/

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8903241 [details]
Bug 1393242 - Normalize paths in MercurialRevisionFinder;

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

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8900526 [details]
Bug 1393242 - Vendor python-hglib 2.4;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171910/diff/4-5/

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171912/diff/6-7/

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171914/diff/6-7/

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8900524 [details]
Bug 1393242 - Helper function to obtain a repository from a build config;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171906/diff/3-4/

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8900525 [details]
Bug 1393242 - Return a Repository using configure preferences;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171908/diff/3-4/

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8902788 [details]
Bug 1393242 - Make hglib tests more resilient;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174452/diff/4-5/

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8903241 [details]
Bug 1393242 - Normalize paths in MercurialRevisionFinder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175034/diff/3-4/

Comment 50

[Gregory Szorc [:gps]]

Comment on attachment 8900526 [details]
Bug 1393242 - Vendor python-hglib 2.4;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171910/diff/5-6/

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8900527 [details]
Bug 1393242 - Hook up hglib to HgRepository;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171912/diff/7-8/

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8900528 [details]
Bug 1393242 - Use hglib for get_files_in_working_directory();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171914/diff/7-8/

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/608f8c2125a7
Helper function to obtain a repository from a build config; r=mshal
https://hg.mozilla.org/integration/autoland/rev/318dfe168265
Return a Repository using configure preferences; r=mshal
https://hg.mozilla.org/integration/autoland/rev/fdd63a193b9d
Make hglib tests more resilient; r=mshal
https://hg.mozilla.org/integration/autoland/rev/175c99472cfd
Normalize paths in MercurialRevisionFinder; r=mshal
https://hg.mozilla.org/integration/autoland/rev/4a06dcbf5793
Vendor python-hglib 2.4; r=mshal
https://hg.mozilla.org/integration/autoland/rev/b07f70e6b32b
Hook up hglib to HgRepository; r=mshal
https://hg.mozilla.org/integration/autoland/rev/a2d3d62c0df7
Use hglib for get_files_in_working_directory(); r=mshal

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/608f8c2125a7
https://hg.mozilla.org/mozilla-central/rev/318dfe168265
https://hg.mozilla.org/mozilla-central/rev/fdd63a193b9d
https://hg.mozilla.org/mozilla-central/rev/175c99472cfd
https://hg.mozilla.org/mozilla-central/rev/4a06dcbf5793
https://hg.mozilla.org/mozilla-central/rev/b07f70e6b32b
https://hg.mozilla.org/mozilla-central/rev/a2d3d62c0df7