Noissue - Add '.revision'-file to devbuilds

Noissue - Add '.revision'-file to devbuilds


This references https://gitlab.com/eyeo/adblockplus/adblockpluschrome/issues/88

, the dependency update to this change will happen in https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/126

[tlucas, 2019-10-10 15:42:50]

Patch Set 1:

Add `.revision`-file to devbuilds.

[tlucas, 2019-10-11 11:48:55]

> Rietveld is down again. So I review this patch here:

> 1. Right now, you always ignore any errors by git. However, in case Git isn't
even installed, the resulting OSError isn't caught. How about instead of
catching any exceptions, checking whether we are building from a git repository
(by checking whether there is a ".git" folder) before calling git?
> 2. Please respect the baseDir argument passed in to createBuild() rather than
assuming that the repository is in the current working directory.

Patch Set 2: addressed Sebastians comments.

[Sebastian Noack, 2019-10-11 21:18:43]

https://codereview.adblockplus.org/30062555/diff/30063555/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/30062555/diff/30063555/packagerChrome.py#n...
packagerChrome.py:403: add_revision_file(baseDir, files)
Splitting this out into a function seems unnecessary. Moreover, it seems more
logical to keep the checks together in order to state more obviously that we do
this for all devbuilds built from Git:

  building_from_git = os.path.exists(os.path.join(baseDir, '.git'))
  if not releaseBuild and not devenv and building_from_git:
      cmd = ['git', 'rev-parse', 'HEAD']
      files['.revision'] = subprocess.check_output(cmd, cwd=baseDir)

Note that I also removed the strip() as plain text files are supposed to end
with a new line:
https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-n...

[tlucas, 2019-10-14 12:14:54]

Patch Set 3

Fully adapated Sebastian's suggestion

https://codereview.adblockplus.org/30062555/diff/30063555/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/30062555/diff/30063555/packagerChrome.py#n...
packagerChrome.py:403: add_revision_file(baseDir, files)
On 2019/10/11 21:18:43, Sebastian Noack wrote:
> Splitting this out into a function seems unnecessary. Moreover, it seems more
> logical to keep the checks together in order to state more obviously that we
do
> this for all devbuilds built from Git:
> 
>   building_from_git = os.path.exists(os.path.join(baseDir, '.git'))
>   if not releaseBuild and not devenv and building_from_git:
>       cmd = ['git', 'rev-parse', 'HEAD']
>       files['.revision'] = subprocess.check_output(cmd, cwd=baseDir)
> 
> Note that I also removed the strip() as plain text files are supposed to end
> with a new line:
>
https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-n...

Done.

[Sebastian Noack, 2019-10-15 00:17:43]

LGTM