|mach vendor rust| fails when inside a git worktree

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

52 Branch
mozilla53

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed)

Details

Attachments

(1 attachment)

If you're in a git worktree (which is a working directory tied to a repository) there is no .git directory. |mach vendor rust| relies on the existence of .git to detect that it's under git version control, and so this fails. Example output produced:

kats@kgupta-air mozilla-wr$ mach vendor rust
Error running mach:

    ['vendor', 'rust']

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:

Exception: Unknown VCS, or not a source checkout: /Users/kats/zspace/mozilla-wr

  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/mach_commands.py", line 1603, in vendor_rust
    vendor_command.vendor(**kwargs)
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/vendor_rust.py", line 61, in vendor
    self.check_modified_files()
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/vendor_rust.py", line 46, in check_modified_files
    modified = [f for f in self.repository.get_modified_files() if os.path.basename(f) not in ('Cargo.toml', 'Cargo.lock')]
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/util.py", line 943, in __get__
    setattr(instance, name, self.func(instance))
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/base.py", line 289, in repository
    return get_repository_object(self.topsrcdir)
  File "/Users/kats/zspace/mozilla-wr/python/mozversioncontrol/mozversioncontrol/__init__.py", line 105, in get_repository_object
    raise Exception('Unknown VCS, or not a source checkout: %s' % path)
Comment on attachment 8805236 [details]
Bug 1313446 - Detect a git repository when inside a git worktree.

https://reviewboard.mozilla.org/r/89010/#review88168

::: python/mozversioncontrol/mozversioncontrol/__init__.py:104
(Diff revision 1)
>      '''
>      if os.path.isdir(os.path.join(path, '.hg')):
>          return HgRepository(path)
>      elif os.path.isdir(os.path.join(path, '.git')):
>          return GitRepository(path)
> +    elif os.system('git rev-parse') == 0:

`os.system` should not be used because it executes in a shell. Executing sub-processes in a shell is considered a bad practice unless you need the shell.

::: python/mozversioncontrol/mozversioncontrol/__init__.py:104
(Diff revision 1)
> +    elif os.system('git rev-parse') == 0:
> +        return GitRepository(path)

We should refactor this along with the above 2 lines to be in a shared block that gets executed when the `.git` directory exists.

That being said, as long as we're not dealing with the internals of the .git directory, it is probably sufficient to just change the check to "is a .git path present".
Attachment #8805236 - Flags: review-
Assignee: bugmail → nobody
Yeah, just replace isdir with exists.
Comment on attachment 8805236 [details]
Bug 1313446 - Detect a git repository when inside a git worktree.

https://reviewboard.mozilla.org/r/89010/#review98004

::: python/mozversioncontrol/mozversioncontrol/__init__.py:102
(Diff revision 2)
>      '''Get a repository object for the repository at `path`.
>      If `path` is not a known VCS repository, raise an exception.
>      '''
>      if os.path.isdir(os.path.join(path, '.hg')):
>          return HgRepository(path)
> -    elif os.path.isdir(os.path.join(path, '.git')):
> +    elif os.path.exists(os.path.join(path, '.git')):

Mmmmmm I'm pretty sure I reviewed something similar a few weeks ago. This tells me we really need some abstraction around repositories instead of having to rediscover the same bugs again and again.
Attachment #8805236 - Flags: review?(mh+mozilla) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/febbbdc0273e
Detect a git repository when inside a git worktree. r=glandium
Assignee: nobody → bugmail
https://hg.mozilla.org/mozilla-central/rev/febbbdc0273e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Mike Hommey [:glandium] from comment #5)
> Mmmmmm I'm pretty sure I reviewed something similar a few weeks ago. This
> tells me we really need some abstraction around repositories instead of
> having to rediscover the same bugs again and again.

When I wrote the `mach vendor rust` command I rewrote this mozversioncontrol code to try to be a central module for working with VCS. (There wasn't anything using it at the time.) If there are other places touching VCS we should switch them to using mozversioncontrol now.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.