-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assignment 2.1: Build Automation Tool #60
Changes from 13 commits
ca5e40c
a338501
42557d3
1f28c4f
f33e3f7
6270d50
f0be0a3
1e5bd0d
cd94bc8
9bcb8cb
c633b29
ac947b8
d71d9c4
277b7d6
f1c42c3
5f26c51
22580e3
a1b25bd
c0da0bc
166c5db
923ead1
5da1c4c
980e9e1
c421aa6
eabddd1
7df0d4a
fc505ed
9c83a4b
4ba29bc
0cf3e73
54a3e62
31226cc
28a1c63
60c414e
4e1cadf
86c116c
cfae026
b77e20b
3658eb8
ce7515e
2ec874f
64f9272
18f8219
13b446d
baf6e64
f574672
4f9bdfe
d9f55eb
1176bfa
1f9d18b
f516ae0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/venv/ | ||
.idea | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
import json | ||
|
||
|
||
class Command: | ||
"""Stores Command Information""" | ||
def __init__(self, *, name, command_string, deps=None, files=None): | ||
self._name = name | ||
self._command_string = command_string | ||
kaustubh-karkare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if files is None: | ||
self._hasFiles = False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really a separate variable for _hasFiles? Why not just check if _files is None? In languages like C++, where a list variable cannot be null, this makes more sense, but not sure it is needed in python. |
||
else: | ||
self._hasFiles = True | ||
self._files = files | ||
if deps is None: | ||
self._hasDependencies = False | ||
else: | ||
self._hasDependencies = True | ||
self._dependencies = deps | ||
|
||
def get_name(self): | ||
return self._name | ||
|
||
def get_files(self): | ||
if self._hasFiles: | ||
return self._files | ||
raise Command.NoFilesException("No Files for {}".format(self._name)) | ||
|
||
def get_command_string(self): | ||
return self._command_string | ||
|
||
def get_dependencies(self): | ||
if self._hasDependencies: | ||
return self._dependencies | ||
raise Command.NoDependenciesException("No Dependencies for {}".format(self._name)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't even think you need to raise exceptions like this. Ie - if deps aren't specified, the default meaning is that there are no deps, so you can just return an empty list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ What do you think of this? |
||
|
||
class NoDependenciesException(Exception): | ||
pass | ||
|
||
class NoFilesException(Exception): | ||
pass | ||
|
||
|
||
class BuildConfig: | ||
"""Parses and Stores build.config""" | ||
def __init__(self, json_containing_folder): | ||
# Parse JSON | ||
json_path = json_containing_folder + "/build.json" | ||
with open(json_path) as file_handle: | ||
self._raw_json = json.load(file_handle) | ||
kaustubh-karkare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
self._name_to_command = {} | ||
for command in self._raw_json: | ||
self._name_to_command[command['name']] = command | ||
|
||
def get_command(self, command_name): | ||
if command_name in self._name_to_command: | ||
entry = self._name_to_command[command_name] | ||
name = entry['name'] | ||
command_string = entry['command'] | ||
deps = None | ||
if 'deps' in entry: | ||
deps = entry['deps'] | ||
files = None | ||
if 'files' in entry: | ||
files = entry['files'] | ||
return Command(name=name, command_string=command_string, deps=deps, files=files) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of creating a Command object on demand, why not do all this in your init method, so that it only needs to be done once? |
||
else: | ||
raise BuildConfig.UnknownCommandException('No such command "{}" found.'.format(command_name)) | ||
kaustubh-karkare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
class UnknownCommandException(Exception): | ||
pass | ||
|
||
|
||
if __name__ == '__main__': | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import subprocess | ||
|
||
|
||
def run(command_string, cwd, print_command=False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd just make this a method in your Builder class below. |
||
"""Run Command and Return Exit Code. Optionally Print the command itself""" | ||
if print_command: | ||
print(command_string) | ||
return subprocess.run(command_string, shell=True, cwd=cwd).returncode | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
from Builder.lib.commandrunner import run | ||
from Builder.lib.buildconfig import BuildConfig, Command | ||
|
||
import sys | ||
import os | ||
|
||
|
||
class Builder: | ||
def __init__(self): | ||
self.unresolved_commands = set() | ||
|
||
def _build_rule_handler(self, command_name, command_dir_abs, root_dir_abs, dry_run=False): | ||
"""Parses JSON, Resolves Dependencies and Executes Command/Do a dry run""" | ||
|
||
# Remove Trailing '/' from paths if present | ||
if command_dir_abs.endswith('/'): | ||
command_dir_abs = command_dir_abs[:-1] | ||
if root_dir_abs.endswith('/'): | ||
root_dir_abs = root_dir_abs[:-1] | ||
|
||
# Mark the command's dependencies as unresolved | ||
self.unresolved_commands.add((command_name, command_dir_abs)) | ||
|
||
# Parse build.json in current directory | ||
config = BuildConfig(command_dir_abs) | ||
command = config.get_command(command_name) | ||
|
||
# Process Dependencies | ||
try: | ||
deps = command.get_dependencies() | ||
if not dry_run: | ||
print("Executing {} dependencies for {} in {}...".format(len(deps), command_name, command_dir_abs)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't look too useful, since it doesn't include dependencies.
|
||
for dep in deps: | ||
dep_dir_abs = '' | ||
dep_name = '' | ||
if dep.startswith('//'): | ||
# Command path referenced from root directory | ||
dep_dir_abs = root_dir_abs + '/' + dep[2:] | ||
dep_dir_abs, dep_name = dep_dir_abs.rsplit('/', 1) | ||
elif '/' in dep: | ||
# Command in child directory | ||
dep_dir_abs, dep_name = dep.rsplit('/', 1) | ||
dep_dir_abs = command_dir_abs + "/" + dep_dir_abs | ||
else: | ||
# Command in same directory | ||
dep_name = dep | ||
dep_dir_abs = command_dir_abs | ||
|
||
# Check for circular dependencies | ||
if (dep_name, dep_dir_abs) in self.unresolved_commands: | ||
raise self.CircularDependencyException( | ||
"Detected a Circular Dependency between {}:{} and {}:{}" | ||
.format(dep_dir_abs, dep_name, command_dir_abs, command_name)) | ||
|
||
self._build_rule_handler(dep_name, dep_dir_abs, root_dir_abs, dry_run) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that your |
||
|
||
except Command.NoDependenciesException: | ||
print("No dependencies found for {} in {}...".format(command_name, command_dir_abs)) | ||
|
||
# Mark the command's dependencies as resolved | ||
self.unresolved_commands.remove((command_name, command_dir_abs)) | ||
|
||
if not dry_run: | ||
# Execute Command after processing dependencies | ||
print("Executing {} in {}".format(command_name, command_dir_abs)) | ||
return_value = run(command.get_command_string(), command_dir_abs, print_command=True) | ||
|
||
# Stop Execution if Command Fails | ||
if return_value != 0: | ||
exit(-1) | ||
|
||
def execute_build_rule(self, command_name, command_dir_abs, root_dir_abs): | ||
# Do a dry run and check for syntax, do validation and check for circular dependencies | ||
self._build_rule_handler(command_name, command_dir_abs, root_dir_abs, dry_run=True) | ||
|
||
# If everything is fine do an actual run | ||
self._build_rule_handler(command_name, command_dir_abs, root_dir_abs) | ||
|
||
class CircularDependencyException(Exception): | ||
pass | ||
|
||
|
||
if __name__ == '__main__': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might still be useful to keep this main application logic in a separate file from your library. |
||
command = sys.argv[1] | ||
builder = Builder() | ||
relative_path = '' | ||
if '/' in command: | ||
# Handle relative paths | ||
relative_path, command = command.rsplit('/', 1) | ||
path = os.getcwd() + "/" + relative_path | ||
Builder.execute_build_rule(command, path, os.getcwd()) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
[ | ||
{ | ||
"name": "clean", | ||
"deps": ["algorithms/clean"], | ||
"command": "rm -f test.o && rm -f test.exe" | ||
}, | ||
{ | ||
"name": "test", | ||
"files": ["test.cpp"], | ||
"command": "g++ -std=c++11 -c test.cpp" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import unittest | ||
from Builder.lib.buildconfig import Command, BuildConfig | ||
|
||
|
||
class TestJsonParser(unittest.TestCase): | ||
|
||
@classmethod | ||
def setUpClass(cls): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively,
avoids dealing with class level stuff. |
||
json_path = 'json' | ||
cls.config = BuildConfig(json_path) | ||
|
||
def test_build_clean_parses_correctly(self): | ||
command_clean = self.config.get_command('clean') | ||
self.assertEqual(command_clean.get_name(), 'clean') | ||
self.assertEqual(command_clean.get_command_string(), 'rm -f test.o && rm -f test.exe') | ||
self.assertEqual(command_clean.get_dependencies(), ["algorithms/clean"]) | ||
|
||
def test_build_test_parses_correctly(self): | ||
command_test = self.config.get_command('test') | ||
self.assertEqual(command_test.get_name(), 'test') | ||
self.assertEqual(command_test.get_command_string(), 'g++ -std=c++11 -c test.cpp') | ||
self.assertEqual(command_test.get_files(), ["test.cpp"]) | ||
|
||
def test_no_deps_throws_correct_exception(self): | ||
command_test = self.config.get_command('test') | ||
self.assertRaises(Command.NoDependenciesException, command_test.get_dependencies) | ||
|
||
def test_no_files_throws_correct_exception(self): | ||
command_clean = self.config.get_command('clean') | ||
self.assertRaises(Command.NoFilesException, command_clean.get_files) | ||
|
||
def test_unknown_command_name_raises_correct_exception(self): | ||
self.assertRaises(BuildConfig.UnknownCommandException, self.config.get_command, 'YOLO') | ||
|
||
|
||
if __name__ == '__main__': | ||
unittest.main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import unittest | ||
import subprocess | ||
import os | ||
from Builder.main import Builder | ||
|
||
|
||
class TestBuilder(unittest.TestCase): | ||
|
||
def test_compilation_basic(self): | ||
builder = Builder() | ||
path = os.getcwd() + '/test_builder_files/test_compilation_basic' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assumes that you're in a very specific directory. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, you addressed this concern, but in a way that I didn't expect. |
||
builder.execute_build_rule('run', path, path) | ||
exec_path = '"' + path + '/test.out' + '"' | ||
result = subprocess.run(exec_path, shell=True, capture_output=True, text=True) | ||
self.assertEqual('1 2 3 4 5 \n1 2 3 4 5 \n1 2 3 4 5 \n', result.stdout) | ||
builder.execute_build_rule('clean', path, os.getcwd()) | ||
kaustubh-karkare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def test_commands_referenced_from_root(self): | ||
builder = Builder() | ||
path = os.getcwd() + '/test_builder_files/test_commands_referenced_from_root' | ||
builder.execute_build_rule('run', path, path) | ||
output_file_path = path + '/output' | ||
result = '' | ||
with open(output_file_path) as file_handle: | ||
result = file_handle.readable() | ||
builder.execute_build_rule('clean', path, os.getcwd() + '/test_builder_files/test_commands_referenced_from_root') | ||
self.assertEqual(True, result) | ||
|
||
def test_dry_run_does_not_write_files(self): | ||
builder = Builder() | ||
path = os.getcwd() + '/test_builder_files/test_commands_referenced_from_root' | ||
builder._build_rule_handler('run', path, path, dry_run=True) | ||
output_file_path = path + '/output' | ||
self.assertEqual(False, os.path.isfile(output_file_path)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. self.assertFalse( |
||
|
||
def test_basic_circular_dependency_throws_exception(self): | ||
builder = Builder() | ||
path = os.getcwd() + '/test_builder_files/test_basic_circular_dependency_throws_exception' | ||
self.assertRaises(Builder.CircularDependencyException, builder.execute_build_rule, 'A', path, path) | ||
|
||
def test_basic_circular_dependency2_throws_exception(self): | ||
builder = Builder() | ||
path = os.getcwd() + '/test_builder_files/test_basic_circular_dependency2_throws_exception' | ||
self.assertRaises(Builder.CircularDependencyException, builder.execute_build_rule, 'A', path, path) | ||
|
||
|
||
if __name__ == '__main__': | ||
unittest.main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
[ | ||
{ | ||
"name": "A", | ||
"command": "echo \"A\"", | ||
"deps": ["//A"] | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
[ | ||
{ | ||
"name": "A", | ||
"command": "echo \"A\"", | ||
"deps": ["X/A"] | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
[ | ||
{ | ||
"name": "A", | ||
"command": "echo \"A\"", | ||
"deps": ["B"] | ||
}, | ||
{ | ||
"name": "B", | ||
"command": "echo \"B\"", | ||
"deps": ["A"] | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
[ | ||
{ | ||
"name": "clean", | ||
"command": "echo \"clean A\"" | ||
}, | ||
{ | ||
"name": "run", | ||
"command": "echo \"run A\" >> ../output" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
run A |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[ | ||
{ | ||
"name": "clean", | ||
"command": "echo \"clean B\"" | ||
}, | ||
{ | ||
"name": "run", | ||
"deps": ["//A/run"], | ||
"command": "echo \"run B\" >> ../output" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[ | ||
{ | ||
"name": "clean", | ||
"command": "rm output" | ||
}, | ||
{ | ||
"name": "run", | ||
"deps": ["B/run"], | ||
"command": "echo \"run root\" >> output" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
[ | ||
{ | ||
"name": "clean", | ||
"command": "rm -f *.o" | ||
}, | ||
{ | ||
"name": "sort_bubble", | ||
"files": ["sort_bubble.cpp"], | ||
"command": "g++ -c sort_bubble.cpp" | ||
}, | ||
{ | ||
"name": "sort_merge", | ||
"files": ["sort_merge.cpp"], | ||
"command": "g++ -c sort_merge.cpp" | ||
}, | ||
{ | ||
"name": "sort_quick", | ||
"files": ["sort_quick.cpp"], | ||
"command": "g++ -c sort_quick.cpp" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
#include<vector> | ||
#include<algorithm> | ||
|
||
void sort_bubble(std::vector<int> &arr) { | ||
sort(arr.begin(), arr.end()); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
#include<vector> | ||
#include<algorithm> | ||
|
||
void sort_merge(std::vector<int> &arr) { | ||
sort(arr.begin(), arr.end()); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
#include<vector> | ||
#include<algorithm> | ||
|
||
void sort_quick(std::vector<int> &arr) { | ||
sort(arr.begin(), arr.end()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, it is possible to add .gitignore here to exclude this file too.