build.py - Issue 5710085762318336: 1757 - Integrate ensure_dependencies.py with the adblockpluschrome repository

Side by Side Diff

Use n/p to move between diff chunks; N/P to move between comments.

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Side by Side Diff: build.py

Issue 5710085762318336: 1757 - Integrate ensure_dependencies.py with the adblockpluschrome repository (Closed)

Patch Set: Created Jan. 13, 2015, 5:14 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
1 #!/usr/bin/env python	1 #!/usr/bin/env python

2 # coding: utf-8	2 # coding: utf-8

3	3

4 import os, sys, subprocess	4 import os, sys, subprocess

5	5

	6 DEPENDENCY_SCRIPT = os.path.join(os.path.dirname(__file__), "ensure_dependencies .py")

	7

	8 if os.path.isfile(DEPENDENCY_SCRIPT):
	Sebastian Noack 2015/01/13 17:21:53 Why do you check whether the script exists? It's t Why do you check whether the script exists? It's there. You are adding it with the same patch. paco 2015/01/14 11:40:49 As requested by Wladimir, we should call the scrip Show quoted text On 2015/01/13 17:21:53, Sebastian Noack wrote: > Why do you check whether the script exists? It's there. You are adding it with > the same patch. As requested by Wladimir, we should call the script only if the file actually exists. Sebastian Noack 2015/01/14 11:58:46 I am not sure whether I agree. The only scenario w Show quoted text On 2015/01/14 11:40:49, paco wrote: > On 2015/01/13 17:21:53, Sebastian Noack wrote: > > Why do you check whether the script exists? It's there. You are adding it with > > the same patch. > As requested by Wladimir, we should call the script only if the file actually > exists. I am not sure whether I agree. The only scenario where the script won't exist, is if somebody (accidentally) deleted it. In that case I'd certainly prefer to see an error, rather than silently ignoring it. I put Wladimir into CC. So maybe he can give some background, why he thinks this check makes sense? Wladimir Palant 2015/01/14 13:11:17 The idea was actually that this build script is to Show quoted text On 2015/01/14 11:58:46, Sebastian Noack wrote: > I am not sure whether I agree. The only scenario where the script won't exist, > is if somebody (accidentally) deleted it. The idea was actually that this build script is to be used everywhere unchanged - yet some repositories might now have dependencies, so ensure_dependencies.py won't exist. However, given that build.py itself depends on buildtools we can probably ignore that scenario and just run ensure_dependencies.py unconditionally. paco 2015/01/14 16:00:36 Will remove `if` in next patch Show quoted text On 2015/01/14 13:11:17, Wladimir Palant wrote: > On 2015/01/14 11:58:46, Sebastian Noack wrote: > > I am not sure whether I agree. The only scenario where the script won't exist, > > is if somebody (accidentally) deleted it. > > The idea was actually that this build script is to be used everywhere unchanged > - yet some repositories might now have dependencies, so ensure_dependencies.py > won't exist. However, given that build.py itself depends on buildtools we can > probably ignore that scenario and just run ensure_dependencies.py > unconditionally. Will remove `if` in next patch
	9 try:

	10 subprocess.check_call(["python", DEPENDENCY_SCRIPT])

	11 except subprocess.CalledProcessError as e:

	12 print >>sys.stderr, e

	13 print >>sys.stderr, "Failed to ensure dependencies being up-to-date!"

	14

6 import buildtools.build	15 import buildtools.build

7 buildtools.build.processArgs('.', sys.argv)	16 buildtools.build.processArgs('.', sys.argv)

OLD	NEW

« no previous file with comments | « .hgsubstate ('k') | dependencies » ('j') | dependencies » ('J')