Opened 5 years ago

Closed 5 years ago

#1398 closed defect (fixed)

Improve backwards-compatibility of ensure_dependencies.py

Reported by: matze Assignee: AAlvz
Priority: P4 Milestone:
Module: Automation Keywords: infrastructure hg git
Cc: trev, fhd, AAlvz, poz2k4444 Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5254149566365696/
https://github.com/mjhennig/adblockplus-infrastructure/pull/6

Description

Reading the current version of git(1) repositories in ensure_dependencies.py currently fails in less recent versions of the tool - which do not understand the -C option.

Change History (9)

comment:1 Changed 5 years ago by matze

  • Owner set to AAlvz

One can reproduce this in e.g. @AAlvz' system:

$ git --version
git version 1.7.9.5

Since this is a standard package from an active Ubuntu LTS, which is also an OS used in our production environment, I consider this a supported version of Ubuntu and this issue a valid bug:

$ uname -a
Linux aalvz 3.13.0-35-generic #62~precise1-Ubuntu SMP Mon Aug 18 14:52:04 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

We've patched the ensure_dependencies.py script, applying the following modifications:

< command = ["git", "-C", repo, "rev-parse", "--revs-only", rev]
> command = ["git", "--git-dir", repo + "/.git", "rev-parse", "--revs-only", rev]

This seems to be a valid workaround that is compatible with more recent versions of git(1) (e.g. 2.1.0, deb/jessie) as well.

We'll publish a more sophisticated version (using native path merging, for example) for review later.

comment:2 Changed 5 years ago by matze

  • Component changed from Unknown to Build-and-Release-Tools

comment:4 Changed 5 years ago by trev

  • Priority changed from Unknown to P4
  • Ready set

According to this answer, the --git-dir flag isn't sufficient - one has to use --work-dir as well. However, if it is really that problematic then there is a much simpler solution - just pass in cwd=repo parameter to subprocess.check_call().

Last edited 5 years ago by trev (previous) (diff)

comment:5 Changed 5 years ago by matze

  • Review URL(s) modified (diff)

comment:6 Changed 5 years ago by matze

@trev: You're right, the call in the Git.update() method requires a --work-tree option as well (this issue was masked before, as the process bailed out way earlier). Since your suggested cwd= solution seems like the most simple and portable one, we'll simply use it for each invocation of git(1) - rather than only the one requiring both --git-dir and --work-tree.

comment:7 Changed 5 years ago by matze

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed 5 years ago by matze

After the LGTM in review, the only thing missing so far is the push into the buildtools repo - which I cannot perform due to missing permission for both, buildtools and jshydra. @trev/@fhd Do you want to commit there or grant me the permissions?

The changes have been merged and pushed into the infrastructure repo already, however.

comment:9 Changed 5 years ago by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.