tools: buildman: Don't use the working dir as build dir
PATCH v2 1/1 tools: buildman: Don't use the working dir as build dir
From: Lothar Waßmann <[email protected]>
When the U-Boot base directory happens to have the same name as the branch that buildman is directed to use via the '-b' option and no output directory is specified with '-o', buildman happily starts removing the whole U-Boot sources eventually only stopped with the error message:
OSError: [Errno 20] Not a directory: '../<branch-name>/boards.cfg
Add a check to avoid this and also deal with the case where '-o' points to the source directory, or any subdirectory of it.
Finally, tidy up the confusing logic for removing the old tree when using -b. This is only done when building a branch.
---
Changes in v2:
- Updated to check directories on start-up as per comments on v1 patch
- Added a test
- Expanded check to handle subdirectories
tools/buildman/builderthread.py | 4 ++++ tools/buildman/control.py | 28 +++++++++++++++++++++++++--- tools/buildman/func_test.py | 9 +++++++++ 3 files changed, 38 insertions(+), 3 deletions(-)
tools/buildman/builderthread.py +4 -0
@@ -7,6 +7,7 @@ import errno import glob import os import shutil +import sys import threading import command
@@ -27,6 +28,9 @@ def Mkdir(dirname, parents = False): os.mkdir(dirname) except OSError as err: if err.errno == errno.EEXIST: + if os.path.realpath('.') == os.path.realpath(dirname): + print "Cannot create the current working directory '%s'!" % dirname + sys.exit(1) pass else: raise
tools/buildman/control.py +25 -3
@@ -81,6 +81,28 @@ def ShowActions(series, why_selected, boards_selected, builder, options): print ('Total boards to build for each commit: %d\n' % len(why_selected['all'])) +def CheckOutputDir(output_dir): + """Make sure that the output directory is not within the current directory + + If we try to use an output directory which is within the current directory + (which is assumed to hold the U-Boot source) we may end up deleting the + U-Boot source code. Detect this and print an error in this case. + + Args: + output_dir: Output directory path to check + """ + path = os.path.realpath(output_dir) + cwd_path = os.path.realpath('.') + while True: + if os.path.realpath(path) == cwd_path: + Print("Cannot use output directory '%s' since it is within the " + "current directtory '%s'" % (path, cwd_path)) + sys.exit(1) + parent = os.path.dirname(path) + if parent == path: + break + path = parent + def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, clean_dir=False): """The main control code for buildman
@@ -252,9 +274,9 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, # output directory itself rather than any subdirectory. if not options.no_subdirs: output_dir = os.path.join(options.output_dir, dirname) - if (clean_dir and output_dir != options.output_dir and - os.path.exists(output_dir)): - shutil.rmtree(output_dir) + if clean_dir and os.path.exists(output_dir): + shutil.rmtree(output_dir) + CheckOutputDir(output_dir) builder = Builder(toolchains, output_dir, options.git_dir, options.threads, options.jobs, gnu_make=gnu_make, checkout=True, show_unknown=options.show_unknown, step=options.step,
tools/buildman/func_test.py +9 -0
@@ -521,3 +521,12 @@ class TestFunctional(unittest.TestCase): self._RunControl('-b', self._test_branch, clean_dir=False) self.assertEqual(self._builder.count, self._total_builds) self.assertEqual(self._builder.fail, 0) + + def testBadOutputDir(self): + """Test building with an output dir the same as out current dir""" + self._test_branch = '/__dev/__testbranch' + with self.assertRaises(SystemExit): + self._RunControl('-b', self._test_branch, '-o', os.getcwd()) + with self.assertRaises(SystemExit): + self._RunControl('-b', self._test_branch, '-o', + os.path.join(os.getcwd(), 'test'))
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v2 1/1 tools: buildman: Don't use the working dir as build dir
Hi,
Quoted 29 lines · On Mon, 2 Apr 2018 02:43:12 -0600 Simon Glass wrote:
From: Lothar Waßmann <[email protected]>
When the U-Boot base directory happens to have the same name as the branch that buildman is directed to use via the '-b' option and no output directory is specified with '-o', buildman happily starts removing the whole U-Boot sources eventually only stopped with the error message:
OSError: [Errno 20] Not a directory: '../<branch-name>/boards.cfg
Add a check to avoid this and also deal with the case where '-o' points to the source directory, or any subdirectory of it.
Finally, tidy up the confusing logic for removing the old tree when using -b. This is only done when building a branch.
Signed-off-by Lothar Waßmann <[email protected]>Signed-off-by Simon Glass <[email protected]>---
Changes in v2:
- Updated to check directories on start-up as per comments on v1 patch
- Added a test
- Expanded check to handle subdirectoriestools/buildman/builderthread.py | 4 ++++ tools/buildman/control.py | 28 +++++++++++++++++++++++++--- tools/buildman/func_test.py | 9 +++++++++ 3 files changed, 38 insertions(+), 3 deletions(-)
[...]
Quoted 14 lines
tools/buildman/control.py +6
-0@@ -81,6 +81,28 @@ def ShowActions(series, why_selected, boards_selected, builder, options): print ('Total boards to build for each commit: %d\n' % len(why_selected['all'])) +def CheckOutputDir(output_dir): + """Make sure that the output directory is not within the current directory + + If we try to use an output directory which is within the current directory + (which is assumed to hold the U-Boot source) we may end up deleting the + U-Boot source code. Detect this and print an error in this case.
tools/buildman/control.py def ShowActions(series, why_selected, boards_selected, builder, options): · line 99
Quoted 10 lines
tools/buildman/control.py +10
-0+ + Args: + output_dir: Output directory path to check + """ + path = os.path.realpath(output_dir) + cwd_path = os.path.realpath('.') + while True: + if os.path.realpath(path) == cwd_path: + Print("Cannot use output directory '%s' since it is within the " + "current directtory '%s'" % (path, cwd_path))
s/directtory/directory/
NB: IMO its a bad habit to split format strings across multiple lines,
since it makes it harder to grep the source code for a message that was printed on the terminal.
Otherwise, looks good to me.
Lothar Waßmann
PATCH v2 1/1 tools: buildman: Don't use the working dir as build dir
Hi Lothar,
Quoted 33 lines · Lothar Waßmann
Hi,
Quoted 29 lines · On Mon, 2 Apr 2018 02:43:12 -0600 Simon Glass wrote:
From: Lothar Waßmann <[email protected]>
When the U-Boot base directory happens to have the same name as the branch that buildman is directed to use via the '-b' option and no output directory is specified with '-o', buildman happily starts removing the whole U-Boot sources eventually only stopped with the error message:
OSError: [Errno 20] Not a directory: '../<branch-name>/boards.cfg
Add a check to avoid this and also deal with the case where '-o' points to the source directory, or any subdirectory of it.
Finally, tidy up the confusing logic for removing the old tree when using -b. This is only done when building a branch.
Signed-off-by Lothar Waßmann <[email protected]>Signed-off-by Simon Glass <[email protected]>---
Changes in v2:
- Updated to check directories on start-up as per comments on v1 patch
- Added a test
- Expanded check to handle subdirectoriestools/buildman/builderthread.py | 4 ++++ tools/buildman/control.py | 28 +++++++++++++++++++++++++--- tools/buildman/func_test.py | 9 +++++++++ 3 files changed, 38 insertions(+), 3 deletions(-)[...]
[...]
Quoted 21 lines
Quoted 6 lines
tools/buildman/control.py +6
-0+def CheckOutputDir(output_dir): + """Make sure that the output directory is not within the current directory + + If we try to use an output directory which is within the current directory + (which is assumed to hold the U-Boot source) we may end up deleting the + U-Boot source code. Detect this and print an error in this case.tools/buildman/control.py def ShowActions(series, why_selected, boards_selected, builder, options): · line 99
Quoted 10 lines
tools/buildman/control.py +10
-0+ + Args: + output_dir: Output directory path to check + """ + path = os.path.realpath(output_dir) + cwd_path = os.path.realpath('.') + while True: + if os.path.realpath(path) == cwd_path: + Print("Cannot use output directory '%s' since it is within the " + "current directtory '%s'" % (path, cwd_path))s/directtory/directory/
NB: IMO its a bad habit to split format strings across multiple lines,
since it makes it harder to grep the source code for a message that was printed on the terminal.
Quoted 3 lines
Otherwise, looks good to me.
Tested-by Lothar Waßmann <[email protected]>
Yes I agree although for some reason I have tended to stick strictly to 80 columns in Python. I'm not sure why though, so will change it.
Regards,
Simon