Skip to content

Commit e014686

Browse files
Dario Berzanoktf
Dario Berzano
authored andcommitted
Improve validation of defaults
* Add validation to `doctor` and `init` * Stop build earlier if incompatible defaults are found * Add test
1 parent b5df227 commit e014686

File tree

7 files changed

+229
-65
lines changed

7 files changed

+229
-65
lines changed

alibuild_helpers/build.py

+20-17
Original file line numberDiff line numberDiff line change
@@ -257,20 +257,26 @@ def doBuild(args, parser):
257257
toolHash=getDirectoryHash(dirname(__file__)),
258258
distHash=os.environ["ALIBUILD_ALIDIST_HASH"]))
259259

260-
(systemPackages, ownPackages, failed) = getPackageList(packages=packages,
261-
specs=specs,
262-
configDir=args.configDir,
263-
preferSystem=args.preferSystem,
264-
noSystem=args.noSystem,
265-
architecture=args.architecture,
266-
disable=args.disable,
267-
defaults=args.defaults,
268-
dieOnError=dieOnError,
269-
performPreferCheck=lambda pkg, cmd : dockerStatusOutput(cmd, dockerImage),
270-
performRequirementCheck=lambda pkg, cmd : dockerStatusOutput(cmd, dockerImage),
271-
overrides=overrides,
272-
taps=taps,
273-
log=debug)
260+
(systemPackages, ownPackages, failed, validDefaults) = getPackageList(packages = packages,
261+
specs = specs,
262+
configDir = args.configDir,
263+
preferSystem = args.preferSystem,
264+
noSystem = args.noSystem,
265+
architecture = args.architecture,
266+
disable = args.disable,
267+
defaults = args.defaults,
268+
dieOnError = dieOnError,
269+
performPreferCheck = lambda pkg, cmd : dockerStatusOutput(cmd, dockerImage),
270+
performRequirementCheck = lambda pkg, cmd : dockerStatusOutput(cmd, dockerImage),
271+
performValidateDefaults = lambda spec : validateDefaults(spec, args.defaults),
272+
overrides = overrides,
273+
taps = taps,
274+
log = debug)
275+
if validDefaults and args.defaults not in validDefaults:
276+
return (error, "Specified default `%s' is not compatible with the packages you want to build.\n" % args.defaults +
277+
"Valid defaults:\n\n- " +
278+
"\n- ".join(sorted(validDefaults)), 1)
279+
274280
if failed:
275281
return (error, "The following packages are system requirements and could not be found:\n\n- " + "\n- ".join(sorted(list(failed))) +
276282
"\n\nPlease run:\n\n\taliDoctor %s\n\nto get a full diagnosis." % args.pkgname.pop(), 1)
@@ -448,9 +454,6 @@ def doBuild(args, parser):
448454
spec = specs[p]
449455
if "source" in spec:
450456
debug("Commit hash for %s@%s is %s" % (spec["source"], spec["tag"], spec["commit_hash"]))
451-
ok, out = validateDefaults(spec, args.defaults)
452-
if not ok:
453-
return (error, out, 1)
454457

455458
# Calculate the hashes. We do this in build order so that we can guarantee
456459
# that the hashes of the dependencies are calculated first. Also notice that

alibuild_helpers/doctor.py

+35-16
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import logging
99
from alibuild_helpers.log import debug, error, banner, info, success, warning
1010
from alibuild_helpers.log import logger
11-
from alibuild_helpers.utilities import getPackageList, format, detectArch, parseDefaults, readDefaults
11+
from alibuild_helpers.utilities import getPackageList, format, detectArch, parseDefaults, readDefaults, validateDefaults
1212
from alibuild_helpers.utilities import dockerStatusOutput
1313
import subprocess
1414

@@ -163,20 +163,28 @@ def unreachable():
163163
if err:
164164
error(err)
165165
exit(1)
166-
(fromSystem, own, failed) = getPackageList(packages=packages,
167-
specs=specs,
168-
configDir=args.configDir,
169-
preferSystem=args.preferSystem,
170-
noSystem=args.noSystem,
171-
architecture=args.architecture,
172-
disable=args.disable,
173-
defaults=args.defaults,
174-
dieOnError=lambda x, y : unreachable,
175-
performPreferCheck=lambda pkg, cmd : checkPreferSystem(pkg, cmd, homebrew_replacement, dockerImage),
176-
performRequirementCheck=lambda pkg, cmd : checkRequirements(pkg, cmd, homebrew_replacement, dockerImage),
177-
overrides=overrides,
178-
taps=taps,
179-
log=info)
166+
167+
def performValidateDefaults(spec):
168+
(ok,msg,valid) = validateDefaults(spec, args.defaults)
169+
if not ok:
170+
error(msg)
171+
return (ok,msg,valid)
172+
173+
(fromSystem, own, failed, validDefaults) = getPackageList(packages = packages,
174+
specs = specs,
175+
configDir = args.configDir,
176+
preferSystem = args.preferSystem,
177+
noSystem = args.noSystem,
178+
architecture = args.architecture,
179+
disable = args.disable,
180+
defaults = args.defaults,
181+
dieOnError = lambda x, y : unreachable,
182+
performPreferCheck = lambda pkg, cmd : checkPreferSystem(pkg, cmd, homebrew_replacement, dockerImage),
183+
performRequirementCheck = lambda pkg, cmd : checkRequirements(pkg, cmd, homebrew_replacement, dockerImage),
184+
performValidateDefaults = performValidateDefaults,
185+
overrides = overrides,
186+
taps = taps,
187+
log = info)
180188

181189
if fromSystem:
182190
banner("The following packages will be picked up from the system:\n\n- " +
@@ -188,9 +196,20 @@ def unreachable():
188196
"\n\nThis is not a real issue, but it might take longer the first time you invoke aliBuild." +
189197
"\nLook at the error messages above to get hints on what packages you need to install separately.")
190198
if failed:
191-
banner("The following packages are system dependencies and could not be found:\n\n- "+
199+
banner("The following packages are system dependencies and could not be found:\n\n- " +
192200
"\n- ".join(failed) +
193201
"\n\nLook at the error messages above to get hints on what packages you need to install separately.")
194202
exitcode = 1
203+
if validDefaults and args.defaults not in validDefaults:
204+
banner("The list of packages cannot be built with the defaults you have specified.\n" +
205+
"List of valid defaults:\n\n- " +
206+
"\n- ".join(validDefaults) +
207+
"\n\nUse the `--defaults' switch to specify one of them.")
208+
exitcode = 2
209+
if validDefaults is None:
210+
banner("No valid defaults combination was found for the given list of packages, check your recipes!")
211+
exitcode = 3
212+
if exitcode:
213+
error("There were errors: build cannot be performed if they are not resolved. Check the messages above.")
195214
exit(exitcode)
196215

alibuild_helpers/init.py

+20-15
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
from alibuild_helpers.cmd import execute
22
from alibuild_helpers.utilities import format
3-
from alibuild_helpers.utilities import parseRecipe, getPackageList, getRecipeReader, parseDefaults, readDefaults
3+
from alibuild_helpers.utilities import parseRecipe, getPackageList, getRecipeReader, parseDefaults, readDefaults, validateDefaults
44
from alibuild_helpers.log import debug, error, warning, banner, info
55
from alibuild_helpers.log import dieOnError
66
from alibuild_helpers.workarea import updateReferenceRepos
@@ -51,20 +51,25 @@ def doInit(setdir, configDir, pkgname, referenceSources, dist, defaults, dryRun)
5151
specs = {}
5252
defaultsReader = lambda: readDefaults(configDir, defaults, error)
5353
(err, overrides, taps) = parseDefaults([], defaultsReader, debug)
54-
getPackageList(packages=[ p["name"] for p in pkgs ],
55-
specs=specs,
56-
configDir=configDir,
57-
preferSystem=False,
58-
noSystem=True,
59-
architecture="",
60-
disable=[],
61-
defaults=defaults,
62-
dieOnError=lambda *x, **y: None,
63-
performPreferCheck=lambda *x, **y: (1, ""),
64-
performRequirementCheck=lambda *x, **y: (0, ""),
65-
overrides=overrides,
66-
taps=taps,
67-
log=debug)
54+
(_,_,_,validDefaults) = getPackageList(packages=[ p["name"] for p in pkgs ],
55+
specs=specs,
56+
configDir=configDir,
57+
preferSystem=False,
58+
noSystem=True,
59+
architecture="",
60+
disable=[],
61+
defaults=defaults,
62+
dieOnError=lambda *x, **y: None,
63+
performPreferCheck=lambda *x, **y: (1, ""),
64+
performRequirementCheck=lambda *x, **y: (0, ""),
65+
performValidateDefaults=lambda spec : validateDefaults(spec, defaults),
66+
overrides=overrides,
67+
taps=taps,
68+
log=debug)
69+
dieOnError(validDefaults and defaults not in validDefaults,
70+
"Specified default `%s' is not compatible with the packages you want to build.\n" % defaults +
71+
"Valid defaults:\n\n- " +
72+
"\n- ".join(sorted(validDefaults)))
6873

6974
for p in pkgs:
7075
spec = specs.get(p["name"])

alibuild_helpers/utilities.py

+15-6
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,17 @@ def validateSpec(spec):
6262
# Use this to check if a given spec is compatible with the given default
6363
def validateDefaults(finalPkgSpec, defaults):
6464
if not "valid_defaults" in finalPkgSpec:
65-
return (True, "")
65+
return (True, "", [])
6666
validDefaults = asList(finalPkgSpec["valid_defaults"])
6767
nonStringDefaults = [x for x in validDefaults if not type(x) == str]
6868
if nonStringDefaults:
69-
return (False, "valid_defaults needs to be a string or a list of strings. Found %s." % nonStringDefaults)
69+
return (False, "valid_defaults needs to be a string or a list of strings. Found %s." % nonStringDefaults, [])
7070
if defaults in validDefaults:
71-
return (True, "")
71+
return (True, "", validDefaults)
7272
return (False, "Cannot compile %s with `%s' default. Valid defaults are\n%s" %
7373
(finalPkgSpec["package"],
7474
defaults,
75-
"\n".join([" - " + x for x in validDefaults])))
75+
"\n".join([" - " + x for x in validDefaults])), validDefaults)
7676

7777
def format(s, **kwds):
7878
return to_unicode(s) % kwds
@@ -277,13 +277,14 @@ def parseDefaults(disable, defaultsGetter, log):
277277

278278
def getPackageList(packages, specs, configDir, preferSystem, noSystem,
279279
architecture, disable, defaults, dieOnError, performPreferCheck, performRequirementCheck,
280-
overrides, taps, log):
280+
performValidateDefaults, overrides, taps, log):
281281
systemPackages = set()
282282
ownPackages = set()
283283
failedRequirements = set()
284284
testCache = {}
285285
requirementsCache = {}
286286
packages = packages[:]
287+
validDefaults = [] # empty list: all OK; None: no valid default; non-empty list: list of valid ones
287288
while packages:
288289
p = packages.pop(0)
289290
if p in specs:
@@ -336,6 +337,14 @@ def getPackageList(packages, specs, configDir, preferSystem, noSystem,
336337
if spec["package"] in disable:
337338
continue
338339

340+
# Check whether the package is compatible with the specified defaults
341+
if validDefaults is not None:
342+
(ok,msg,valid) = performValidateDefaults(spec)
343+
if valid:
344+
validDefaults = [ v for v in validDefaults if v in valid ] if validDefaults else valid[:]
345+
if not validDefaults:
346+
validDefaults = None # no valid default works for all current packages
347+
339348
# For the moment we treat build_requires just as requires.
340349
fn = lambda what: filterByArchitecture(architecture, spec.get(what, []))
341350
spec["requires"] = [x for x in fn("requires") if not x in disable]
@@ -352,7 +361,7 @@ def getPackageList(packages, specs, configDir, preferSystem, noSystem,
352361
spec["recipe"] = recipe.strip("\n")
353362
specs[spec["package"]] = spec
354363
packages += spec["requires"]
355-
return (systemPackages, ownPackages, failedRequirements)
364+
return (systemPackages, ownPackages, failedRequirements, validDefaults)
356365

357366
def dockerStatusOutput(cmd, dockerImage=None, executor=getstatusoutput):
358367
if dockerImage:

tests/test_doctor.py

+126
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
from __future__ import print_function
2+
try:
3+
from unittest.mock import patch, call, MagicMock # In Python 3, mock is built-in
4+
from io import StringIO
5+
except ImportError:
6+
from mock import patch, call, MagicMock # Python 2
7+
from StringIO import StringIO
8+
try:
9+
from collections import OrderedDict
10+
except ImportError:
11+
from ordereddict import OrderedDict
12+
13+
from alibuild_helpers.doctor import doDoctor
14+
from argparse import Namespace
15+
import unittest
16+
17+
RECIPE_DEFAULTS_RELEASE = """package: defaults-release
18+
version: v1
19+
---
20+
"""
21+
22+
RECIPE_PACKAGE1 = """package: Package1
23+
version: v1
24+
prefer_system: .*
25+
prefer_system_check: /bin/false
26+
---
27+
"""
28+
29+
RECIPE_SYSDEP = """package: SysDep
30+
version: v1
31+
system_requirement: .*
32+
system_requirement_check: /bin/false
33+
---
34+
"""
35+
36+
RECIPE_BREAKDEFAULTS = """package: BreakDefaults
37+
version: v1
38+
valid_defaults:
39+
- its_not_there
40+
---
41+
"""
42+
43+
RECIPE_TESTDEF1 = """package: TestDef1
44+
version: v1
45+
valid_defaults:
46+
- common_default
47+
- default1
48+
requires:
49+
- TestDef2
50+
---
51+
"""
52+
53+
RECIPE_TESTDEF2 = """package: TestDef2
54+
version: v1
55+
valid_defaults:
56+
- common_default
57+
- default2
58+
---
59+
"""
60+
class DoctorTestCase(unittest.TestCase):
61+
62+
@patch("alibuild_helpers.doctor.banner")
63+
@patch("alibuild_helpers.doctor.warning")
64+
@patch("alibuild_helpers.doctor.error")
65+
@patch("alibuild_helpers.doctor.exists")
66+
@patch("alibuild_helpers.utilities.open")
67+
def test_doctor(self, mockOpen, mockExists, mockPrintError, mockPrintWarning, mockPrintBanner):
68+
mockExists.return_value = True
69+
mockOpen.side_effect = lambda f: { "/dist/package1.sh" : StringIO(RECIPE_PACKAGE1),
70+
"/dist/testdef1.sh" : StringIO(RECIPE_TESTDEF1),
71+
"/dist/testdef2.sh" : StringIO(RECIPE_TESTDEF2),
72+
"/dist/sysdep.sh" : StringIO(RECIPE_SYSDEP),
73+
"/dist/defaults-release.sh" : StringIO(RECIPE_DEFAULTS_RELEASE),
74+
"/dist/breakdefaults.sh" : StringIO(RECIPE_BREAKDEFAULTS) }[f]
75+
76+
# Collect printouts
77+
def resetOut():
78+
return { "warning": StringIO(), "error": StringIO(), "banner": StringIO() }
79+
mockPrintError.side_effect = lambda e: out["error"].write(e+"\n")
80+
mockPrintWarning.side_effect = lambda e: out["warning"].write(e+"\n")
81+
mockPrintBanner.side_effect = lambda e: out["banner"].write(e+"\n")
82+
83+
args = Namespace(workDir="/work",
84+
configDir="/dist",
85+
docker=False,
86+
debug=False,
87+
preferSystem=[],
88+
noSystem=False,
89+
architecture="osx_x86-64",
90+
disable=[],
91+
defaults="release")
92+
93+
# Test: all should go OK (exit with 0)
94+
out = resetOut()
95+
with self.assertRaises(SystemExit) as cm:
96+
args.packages=["Package1"]
97+
doDoctor(args, MagicMock())
98+
self.assertEqual(cm.exception.code, 0)
99+
100+
# Test: system dependency not found
101+
out = resetOut()
102+
with self.assertRaises(SystemExit) as cm:
103+
args.packages=["SysDep"]
104+
doDoctor(args, MagicMock())
105+
self.assertEqual(cm.exception.code, 1)
106+
107+
# Test: invalid default
108+
out = resetOut()
109+
with self.assertRaises(SystemExit) as cm:
110+
args.packages=["BreakDefaults"]
111+
doDoctor(args, MagicMock())
112+
self.assertEqual(cm.exception.code, 2)
113+
self.assertRegexpMatches(out["error"].getvalue(), "- its_not_there")
114+
115+
# Test: common defaults
116+
out = resetOut()
117+
with self.assertRaises(SystemExit) as cm:
118+
args.packages=["TestDef1"]
119+
doDoctor(args, MagicMock())
120+
self.assertEqual(cm.exception.code, 2)
121+
self.assertRegexpMatches(out["banner"].getvalue(), "- common_default")
122+
self.assertNotRegexpMatches(out["banner"].getvalue(), "- default1")
123+
self.assertNotRegexpMatches(out["banner"].getvalue(), "- default2")
124+
125+
if __name__ == '__main__':
126+
unittest.main()

tests/test_init.py

+2-1
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ def test_doDryRunInit(self, mock_os, mock_path, mock_info):
5656
dryRun=True)
5757
self.assertEqual(mock_info.mock_calls, [call('This will initialise local checkouts for zlib,AliRoot\n--dry-run / -n specified. Doing nothing.')])
5858

59+
@mock.patch("alibuild_helpers.init.banner")
5960
@mock.patch("alibuild_helpers.init.info")
6061
@mock.patch("alibuild_helpers.init.path")
6162
@mock.patch("alibuild_helpers.init.os")
@@ -64,7 +65,7 @@ def test_doDryRunInit(self, mock_os, mock_path, mock_info):
6465
@mock.patch("alibuild_helpers.init.updateReferenceRepos")
6566
@mock.patch("alibuild_helpers.utilities.open")
6667
@mock.patch("alibuild_helpers.init.readDefaults")
67-
def test_doRealInit(self, mock_read_defaults, mock_open, mock_update_reference, mock_parse_recipe, mock_execute, mock_os, mock_path, mock_info):
68+
def test_doRealInit(self, mock_read_defaults, mock_open, mock_update_reference, mock_parse_recipe, mock_execute, mock_os, mock_path, mock_info, mock_banner):
6869
fake_dist = {"repo": "alisw/alidist", "ver": "master"}
6970
mock_open.side_effect = lambda x: {
7071
"/alidist/defaults-release.sh": StringIO("package: defaults-release\nversion: v1\n---"),

0 commit comments

Comments
 (0)