Skip to content
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

Closed
wants to merge 51 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
ca5e40c
Add Config Parser
ankurdubey521 Sep 9, 2019
a338501
Add Command Line Runner
ankurdubey521 Sep 9, 2019
42557d3
Implement Builder
ankurdubey521 Sep 9, 2019
1f28c4f
Add tests for Builder
ankurdubey521 Sep 9, 2019
f33e3f7
Fix tests and PEP8 violations
ankurdubey521 Sep 9, 2019
6270d50
Fix test and add docstrings
ankurdubey521 Sep 9, 2019
f0be0a3
General Syntax and Naming Fixes
ankurdubey521 Sep 10, 2019
1e5bd0d
Implement Command Class and add basic validation
ankurdubey521 Sep 10, 2019
cd94bc8
Fix Main.py
ankurdubey521 Sep 10, 2019
9bcb8cb
Add support for Relative Paths
ankurdubey521 Sep 10, 2019
c633b29
Add support for Root Referenced Commands
ankurdubey521 Sep 11, 2019
ac947b8
Implement Check for Circular Dependencies
ankurdubey521 Sep 11, 2019
d71d9c4
Move main function to it's own class
ankurdubey521 Sep 11, 2019
277b7d6
Pre-Generate all Command Objects while initializing BuildConfig Object
ankurdubey521 Oct 3, 2019
f1c42c3
Remove _hasFiles and _hasDependencies from BuildConfig
ankurdubey521 Oct 3, 2019
5f26c51
Print Logs when doing a dry run
ankurdubey521 Oct 3, 2019
22580e3
Move Builder Class to it's own file and Save root_dir_abs
ankurdubey521 Oct 3, 2019
a1b25bd
Move command runner to Builder Class
ankurdubey521 Oct 3, 2019
c0da0bc
Use unittest setup method instead of @classMethod
ankurdubey521 Oct 3, 2019
166c5db
Catch any exceptions that might be thrown in main.py by Builder
ankurdubey521 Oct 3, 2019
923ead1
Fixes in Builder Tests
ankurdubey521 Oct 3, 2019
5da1c4c
Add type hints
ankurdubey521 Oct 3, 2019
980e9e1
Switch to Argparse
ankurdubey521 Oct 3, 2019
c421aa6
Implement class for listening changes to files
ankurdubey521 Oct 3, 2019
eabddd1
Remove redundant for loop from file_watcher
ankurdubey521 Oct 3, 2019
7df0d4a
Integrate File watching in Build System
ankurdubey521 Oct 3, 2019
fc505ed
Make Root Directory Configurable
ankurdubey521 Oct 3, 2019
9c83a4b
Code Quality Fixes
ankurdubey521 Oct 5, 2019
4ba29bc
Add Topological Sort Code
ankurdubey521 Oct 7, 2019
0cf3e73
Implement execution of dependency rules in parallel
ankurdubey521 Oct 7, 2019
54a3e62
Cleanup
ankurdubey521 Oct 7, 2019
31226cc
Include file of dependencies when listening for changes. Also provide…
ankurdubey521 Oct 8, 2019
28a1c63
Document ParallelBuilder
ankurdubey521 Oct 8, 2019
60c414e
Document TopologicalSort
ankurdubey521 Oct 8, 2019
4e1cadf
Document buildconfig
ankurdubey521 Oct 8, 2019
86c116c
Add global constants
ankurdubey521 Oct 8, 2019
cfae026
Remove Redundant Exceptions
ankurdubey521 Oct 8, 2019
b77e20b
Run tests from Tempdir instead of running them from source
ankurdubey521 Oct 8, 2019
3658eb8
Remove Redundant path correction function from TestFileWatcher
ankurdubey521 Oct 8, 2019
ce7515e
Refactoring
ankurdubey521 Oct 9, 2019
2ec874f
Implement Logging
ankurdubey521 Oct 9, 2019
64f9272
Stop spawning more threads if build fails for dependency
ankurdubey521 Oct 9, 2019
18f8219
Move Test file contents to Testing Scripts
ankurdubey521 Oct 9, 2019
13b446d
Use os.path.join instead of manually joining paths
ankurdubey521 Oct 9, 2019
baf6e64
Optimize imports and use os.path.join in tests
ankurdubey521 Oct 10, 2019
f574672
Fix Logging
ankurdubey521 Oct 30, 2019
4f9bdfe
Refactoring
ankurdubey521 Oct 30, 2019
d9f55eb
Implement JSON Caching
ankurdubey521 Oct 30, 2019
1176bfa
Fix Tests
ankurdubey521 Oct 30, 2019
1f9d18b
Remove redundant paramaters from _execute_build_rule
ankurdubey521 Oct 30, 2019
f516ae0
Implement parallel execution using Popen polling
ankurdubey521 Oct 30, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions solutions/ankurdubey521/Build Automation Tool/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/venv/
.idea
Copy link
Owner

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.

Empty file.
Empty file.
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
if files is None:
self._hasFiles = False
Copy link
Owner

Choose a reason for hiding this comment

The 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))
Copy link
Owner

Choose a reason for hiding this comment

The 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.

Copy link
Owner

Choose a reason for hiding this comment

The 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)

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)
Copy link
Owner

Choose a reason for hiding this comment

The 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))

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):
Copy link
Owner

Choose a reason for hiding this comment

The 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

91 changes: 91 additions & 0 deletions solutions/ankurdubey521/Build Automation Tool/Builder/main.py
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))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look too useful, since it doesn't include dependencies.
Can we make the dry_run mode print out all the rules/commands it would have executed?
Output like:

> build run --dry-run
[algorithms/sort_bubble] g++ -c sort_bubble.cpp
[algorithms/sort_merge] g++ -c sort_merge.cpp
[algorithms/sort_quick] g++ -c sort_quick.cpp
[test] g++ -std=c++11 -c test.cpp
[run] g++ algorithms/sort_bubble.o algorithms/sort_merge.o algorithms/sort_quick.o test.o -o test.exe && ./test.exe

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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that your root_dir_abs is passed around unchanged, why not save it as a class variable?
Also, you can make it configurable: build run --root-dir ~/project/


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__':
Copy link
Owner

Choose a reason for hiding this comment

The 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.
I'd also recommend catching your exceptions and only showing the message, instead of the whole stack trace.

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())
Empty file.
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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively,

def setUp(self):
  self.config = BuildConfig('json')

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'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that you're in a very specific directory.
Maybe something like https://stackoverflow.com/questions/5137497/find-current-directory-and-files-directory makes it possible to run a test from any location?

Copy link
Owner

Choose a reason for hiding this comment

The 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())

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))
Copy link
Owner

Choose a reason for hiding this comment

The 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());
}
Loading