Assignment 2.1: Build Automation Tool #60

ankurdubey521 · 2019-09-09T21:57:24Z

So midway through I decided I wanted to do this assignment in Python, mainly because

my skills in python are quite rusty and based on a lot of googling
was having quite a lot of trouble with setting up execSync in node
I therefore request you to be stricter with your analysis of the code

kaustubh-karkare · 2019-09-10T01:44:25Z

solutions/ankurdubey521/Build Automation Tool/.idea/Build Automation Tool.iml

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>


All these xml files should not be part of your PR. Add them to your .gitignore file, since they are relevant only for you.

kaustubh-karkare · 2019-09-10T01:47:27Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_buildconfig.py

+
+class TestJsonParser(unittest.TestCase):
+    def test_sample_json_parser_correctly(self):
+        json_path = 'json/test1.json'


The input to your BuildConfig class should be a directory, not a specific file.
The file name "build.json" is fixed, so "test1.json" violates that pattern.

You can add the "build.json" suffix in the BuildConfig class, instead of relying on the caller to add it.

kaustubh-karkare · 2019-09-10T01:49:05Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_buildconfig.py

+        config = BuildConfig(json_path)
+        self.assertEqual(config.command_names(), ['clean', 'test'])
+        self.assertEqual(config.deps('clean'), ["algorithms/clean"])
+        self.assertEqual(config.command('clean'), "rm -f test.o && rm -f test.exe")


Interesting, these utility methods can definitely be useful, but I'd recommend better naming here.
In this case, adding a "get_" prefix to all of them will make it clear that these are not "set_" or "has_" or "check_" or whatever else methods.

kaustubh-karkare · 2019-09-10T01:49:44Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_builder.py

+
+class TestBuilder(unittest.TestCase):
+    def test_compilation(self):
+        path = '/home/ankurdubey/Code/Github/project-omega/solutions' \


This path is non-transferrable. If I downloaded this code in my machine, this test would fail, which is bad.

kaustubh-karkare · 2019-09-10T01:51:58Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

+        # Parse JSON
+        json_file = open(json_path)
+        self._raw_json = json.load(json_file)
+        json_file.close()


You should use something like:

with open(json_path) as file_handle: _ = json.load(file_handle)

kaustubh-karkare · 2019-09-10T01:57:47Z

solutions/ankurdubey521/Build Automation Tool/Builder/main.py

+
+    # Parse Dependencies First
+    try:
+        deps = config.deps(command)


What if I wanted to run "build algorithms/sort_bubble"?
Since that is not in the current directory, this code would not find it (even though you have the relative path).

kaustubh-karkare · 2019-09-10T02:00:12Z

solutions/ankurdubey521/Build Automation Tool/Builder/main.py

+                dep_containing_folder_path, dep_command = dep.rsplit('/', 1)
+                dep_containing_folder_path = containing_folder_path + "/" + dep_containing_folder_path
+                execute(dep_command, dep_containing_folder_path)
+            except ValueError:


ValueError can be generated by so many things that I have no idea when this will happen. Maybe you want to use a more specific exception?

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_commandrunner.py

kaustubh-karkare · 2019-09-10T02:06:33Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_commandrunner.py

+
+    def test_nonzero_exit_code(self):
+        command = "exit 1"
+        exit_code = run(command, print_command=False, cwd='/')


print_command=False

This should be a default argument value.

kaustubh-karkare · 2019-09-10T02:08:08Z

solutions/ankurdubey521/Build Automation Tool/Builder/main.py

+                # Command in Child Folder
+                dep_containing_folder_path, dep_command = dep.rsplit('/', 1)
+                dep_containing_folder_path = containing_folder_path + "/" + dep_containing_folder_path
+                execute(dep_command, dep_containing_folder_path)


You need to add checks for circular dependencies.

kaustubh-karkare

Nice!

kaustubh-karkare · 2019-09-11T18:45:04Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

+            files = None
+            if 'files' in entry:
+                files = entry['files']
+            return Command(name=name, command_string=command_string, deps=deps, files=files)


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?

kaustubh-karkare · 2019-09-11T18:48:10Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

+        self._name = name
+        self._command_string = command_string
+        if files is None:
+            self._hasFiles = False


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.

kaustubh-karkare · 2019-09-11T18:49:39Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

+    def get_dependencies(self):
+        if self._hasDependencies:
+            return self._dependencies
+        raise Command.NoDependenciesException("No Dependencies for {}".format(self._name))


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.

^ What do you think of this?

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

kaustubh-karkare · 2019-09-11T18:55:23Z

solutions/ankurdubey521/Build Automation Tool/Builder/main.py

+        try:
+            deps = command.get_dependencies()
+            if not dry_run:
+                print("Executing {} dependencies for {} in {}...".format(len(deps), command_name, command_dir_abs))


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

kaustubh-karkare · 2019-09-11T19:04:22Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/commandrunner.py

+import subprocess
+
+
+def run(command_string, cwd, print_command=False):


I'd just make this a method in your Builder class below.

kaustubh-karkare · 2019-09-11T19:05:23Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_buildconfig.py

+class TestJsonParser(unittest.TestCase):
+
+    @classmethod
+    def setUpClass(cls):


Alternatively,

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

avoids dealing with class level stuff.

kaustubh-karkare · 2019-09-11T19:07:16Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_builder.py

+
+    def test_compilation_basic(self):
+        builder = Builder()
+        path = os.getcwd() + '/test_builder_files/test_compilation_basic'


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?

Interesting, you addressed this concern, but in a way that I didn't expect.

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_builder.py

kaustubh-karkare · 2019-09-11T19:12:32Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_builder.py

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


self.assertFalse(

kaustubh-karkare

Excellent progress!

Now for the final requirement: Use multithreading to execute dependency build rules in parallel when possible. And demonstrate the performance benefit via a test, possibly using sleep commands in the command string.

kaustubh-karkare · 2019-10-04T01:21:00Z

solutions/ankurdubey521/Build Automation Tool/.gitignore

@@ -0,0 +1,2 @@
+/venv/
+.idea


btw, it is possible to add .gitignore here to exclude this file too.

kaustubh-karkare · 2019-10-04T01:23:53Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

+
+class Command:
+    """Stores Command Information"""
+    def __init__(self, *, name: str, command_string: str, deps: List[str] = None, files: List[str] = None) -> None:


Given the non-intuitive distinction between Command and command_string, call the class BuildRule?

kaustubh-karkare · 2019-10-04T01:24:28Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

+    def get_dependencies(self):
+        if self._hasDependencies:
+            return self._dependencies
+        raise Command.NoDependenciesException("No Dependencies for {}".format(self._name))


^ What do you think of this?

kaustubh-karkare · 2019-10-04T01:29:49Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/builder.py

+        if command_dir_abs.endswith('/'):
+            command_dir_abs = command_dir_abs[:-1]
+        if self._root_dir_abs.endswith('/'):
+            self._root_dir_abs = self._root_dir_abs[:-1]


I usually prefer to do this validation once at the beginning, instead of during every call to this method.

kaustubh-karkare · 2019-10-04T01:36:21Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/file_watcher.py

+    def _get_file_edit_times(self, file_list: List[str]) -> Dict[str, int]:
+        """Populate a Dict of last edit times of files and return it"""
+        file_edit_times = {}
+        for file in file_list:


Slightly more descriptive variable name:

for file_path in file_list:

kaustubh-karkare · 2019-10-04T01:45:02Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_file_watcher.py

+        output_file = file_path + "output.txt"
+        with open(input_file, 'w') as file:
+            file.write("Hello World 1.0")
+        with open(output_file, 'w') as file:


s/file/file_handle/

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_file_watcher.py

kaustubh-karkare · 2019-10-04T01:48:46Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_file_watcher.py

+
+        # Activate Watcher and change tracked file
+        file_watcher = FileWatcher()
+        process = Process(target=file_watcher.watch_and_execute, args=([input_file], copy_file, input_file, output_file))


Interesting, you're using a separate process here.

kaustubh-karkare · 2019-10-04T01:51:17Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/builder.py

+
+
+class Builder:
+    def __init__(self):


The way you're using this, the constructor must be followed by a call to execute_build_rule. That kind of expectation is unexpected, and while in theory, you could explain it in documentation, there is a better way:

Make execute_build_rule a @classmethod, which creates the object, and then calls _build_rule_handler too. That way, you only need to call one method from outside this file.

kaustubh-karkare · 2019-10-04T01:55:04Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/file_watcher.py

+    def watch_and_execute(self, file_list: List[str], function: Callable[[any], any], *args: any) -> None:
+        """Execute a function whenever a file from file_list changes"""
+        print("Listening for changes on {}...".format(file_list))
+        file_edit_times = self._get_file_edit_times(file_list)


You are making an assumption here that the file list is fixed, which IMO we can avoid.
Ie - if you re-explore the build-rule-graph each time, you can detect and appropriately handle changes to the build.json files too. Yes, it is costlier (so I guess you can make it configurable) but might be more useful too, right?

… a common interface for both watch and non watching modes

kaustubh-karkare

Cool, I'm pretty happy with the overall state of things, we're almost done.

My comments are now primarily to clean up inconsistencies, improve readability (stuff that was affecting the overall score) and a couple of minor extensions for learning purposes.

Btw, I tend to unashamedly ask for naming changes that span multiple files. Because if you have good tests, even large refactors should be cheap and safe.

solutions/ankurdubey521/Build Automation Tool/Builder/lib/algorithms.py

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

kaustubh-karkare · 2019-10-08T23:18:20Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_file_watcher.py

+
+            # Cleanup
+            with open(input_file, 'w') as file_handle:
+                file_handle.write("Hello World 1.0")


Why do you have to reset this?

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

kaustubh-karkare · 2019-10-08T23:26:48Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

+                dependency_graph[node_tuple[0]] = []
+                for item in parallel_builder._dependency_graph[node_tuple]:
+                    dependency_graph[node_tuple[0]].append(item[0])
+            pprint.pprint(dependency_graph, indent=4)


Fine for debugging, but you wouldn't want this if you're running thousands of tests in parallel, which is what happens in large codebases. All the outputs will interfere with each other, and most people running the tests wont even understand the output.

kaustubh-karkare · 2019-10-08T23:32:45Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

+            parallel_builder = ParallelBuilder(path, MAX_THREAD_COUNT)
+            process = Process(target=parallel_builder.execute,
+                              args=('Z', path))
+            sleep(16)


This is the only example that I checked the file contents for, because without that, I couldn't see where the number 16 came from.

kaustubh-karkare · 2019-10-08T23:39:44Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/buildconfig.py

@@ -0,0 +1,84 @@
+"""" Sample build.json


This is an fairly detailed and specific example. Better suited for a readme file?

kaustubh-karkare · 2019-10-21T15:55:47Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/algorithms.py

+            toposort.append(front)
+            if front in graph_adj_list:
+                for dest_node in graph_adj_list[front]:
+                    if not visited[dest_node]:


I was thinking about this line for a while ... trying to figure out when it will be false. Can you help me out?

kaustubh-karkare · 2019-10-22T06:03:11Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/file_watcher.py

+logger = logging.getLogger(__name__)
+logger.addHandler(logging.NullHandler())


Instead of creating separate logger objects for each file, I think you should be passing around a single one (provided to each class as part of its constructor). That way, you only need to configure it once from your "application section" of the code.

The current setup is not thread-safe: ie - I'm trying to run multiple instances of your ParallelBuilder class for whatever reason in parallel, and I want their output to go to different destinations, that isn't possible.

kaustubh-karkare · 2019-10-22T13:02:17Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/parallel_builder.py

+        if self._root_dir_abs.endswith('/'):
+            self._root_dir_abs = self._root_dir_abs[:-1]


Can we move this to the constructor now?

kaustubh-karkare · 2019-10-22T13:03:15Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/parallel_builder.py

+
+        # Parse build.json in current directory
+        config = BuildConfig.load_from_build_directory(command_dir_abs)
+        command = config.get_build_rule(command_name)


Based on the usage, s/command_name/build_rule_name/

kaustubh-karkare · 2019-10-22T13:19:28Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/parallel_builder.py

+            # Update _dependency_graph
+            if dependency_tuple not in self._dependency_graph:
+                self._dependency_graph[dependency_tuple] = []
+            self._dependency_graph[dependency_tuple].append(dependent_tuple)
+
+            # Update _dependency_list
+            if dependent_tuple not in self._dependency_list:
+                self._dependency_list[dependent_tuple] = []
+            self._dependency_list[dependent_tuple].append(dependency_tuple)


These names "_dependency_graph" and "_dependency_list" are not intuitive: there's no way I'd figure out what is contained in them based on purely the name and, it is very easy to confuse between them.

Something like "_rule_name_to_dependency_rule_names" and "_rule_name_to_dependent_rule_names" makes it easier to figure out, although "rule_name" isn't right since you're using a tuple. I couldn't think of a better name while reviewing, but you get the point.

kaustubh-karkare · 2019-10-22T14:04:32Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

+            parallel_builder = ParallelBuilder(path, MAX_THREAD_COUNT)
+            parallel_builder.execute('Z', path)
+            toposort = [item[0] for item in parallel_builder._topologically_sorted_build_rule_names]
+            print(toposort)


Once you're done testing, remove print statements :)

kaustubh-karkare · 2019-10-22T14:06:07Z

solutions/ankurdubey521/Build Automation Tool/Builder/lib/parallel_builder.py

+    def get_last_build_pass_status(self) -> bool:
+        return self._last_build_passed


Is this primarily for testing?

kaustubh-karkare · 2019-10-22T14:11:43Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

+        with tempfile.TemporaryDirectory() as tmpdir:
+            path = os.path.join(tmpdir, "test")
+            write_test_files(self._testMethodName, path)
+            parallel_builder = ParallelBuilder(path, MAX_THREAD_COUNT)


Looks like these 4 lines are common across all your tests. How about using the setUp and tearDown methods to avoid the duplication?

kaustubh-karkare · 2019-10-22T14:16:23Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

+                result = file_handle.readable()
+            self.assertEqual(True, result)
+            # CLEANUP
+            parallel_builder = ParallelBuilder(path, MAX_THREAD_COUNT)


You cant reuse the existing ParallelBuilder object?

kaustubh-karkare · 2019-10-22T14:27:27Z

solutions/ankurdubey521/Build Automation Tool/Builder/tests/test_parallel_builder.py

+            path = os.path.join(tmpdir, "test")
+            write_test_files(self._testMethodName, path)
+            parallel_builder = ParallelBuilder(path, MAX_THREAD_COUNT)
+            parallel_builder._explore_and_build_dependency_graph('Z', path)


Alternative optional design suggestion: instead of having to call this manually, how about doing it from the ParallelBuilder constructor? Instead of exploring dependencies for a single rule, it creates the graph for everything under that directory (not specific to any rule, because that isn't actually needed), and then you can choose to execute a single rule (for which the information is now already cached).

kaustubh-karkare

Cool, this is sufficiently close to the end goal I had in mind. As always, there is more that can be done, but I'm going to approve, since I understand that it can be boring to keep working on the same problem for so long.

ankurdubey521 added 6 commits September 9, 2019 23:42

Add Config Parser

ca5e40c

Add Command Line Runner

a338501

Implement Builder

42557d3

Add tests for Builder

1f28c4f

Fix tests and PEP8 violations

f33e3f7

Fix test and add docstrings

6270d50

kaustubh-karkare requested changes Sep 10, 2019

View reviewed changes

ankurdubey521 added 7 commits September 10, 2019 23:57

General Syntax and Naming Fixes

f0be0a3

Implement Command Class and add basic validation

1e5bd0d

Fix Main.py

cd94bc8

Add support for Relative Paths

9bcb8cb

Add support for Root Referenced Commands

c633b29

Implement Check for Circular Dependencies

ac947b8

Move main function to it's own class

d71d9c4

kaustubh-karkare requested changes Sep 11, 2019

View reviewed changes

ankurdubey521 added 14 commits October 3, 2019 17:55

Pre-Generate all Command Objects while initializing BuildConfig Object

277b7d6

Remove _hasFiles and _hasDependencies from BuildConfig

f1c42c3

Print Logs when doing a dry run

5f26c51

Move Builder Class to it's own file and Save root_dir_abs

22580e3

Move command runner to Builder Class

a1b25bd

Use unittest setup method instead of @classmethod

c0da0bc

Catch any exceptions that might be thrown in main.py by Builder

166c5db

Fixes in Builder Tests

923ead1

Add type hints

5da1c4c

Switch to Argparse

980e9e1

Implement class for listening changes to files

c421aa6

Remove redundant for loop from file_watcher

eabddd1

Integrate File watching in Build System

7df0d4a

Make Root Directory Configurable

fc505ed

kaustubh-karkare requested changes Oct 4, 2019

View reviewed changes

ankurdubey521 added 12 commits October 6, 2019 01:22

Code Quality Fixes

9c83a4b

Add Topological Sort Code

4ba29bc

Implement execution of dependency rules in parallel

0cf3e73

Cleanup

54a3e62

Include file of dependencies when listening for changes. Also provide…

31226cc

… a common interface for both watch and non watching modes

Document ParallelBuilder

28a1c63

Document TopologicalSort

60c414e

Document buildconfig

4e1cadf

Add global constants

86c116c

Remove Redundant Exceptions

cfae026

Run tests from Tempdir instead of running them from source

b77e20b

Remove Redundant path correction function from TestFileWatcher

3658eb8

kaustubh-karkare requested changes Oct 8, 2019

View reviewed changes

ankurdubey521 added 6 commits October 9, 2019 16:17

Refactoring

ce7515e

Implement Logging

2ec874f

Stop spawning more threads if build fails for dependency

64f9272

Move Test file contents to Testing Scripts

18f8219

Use os.path.join instead of manually joining paths

13b446d

Optimize imports and use os.path.join in tests

baf6e64

kaustubh-karkare requested changes Oct 22, 2019

View reviewed changes

ankurdubey521 added 6 commits October 31, 2019 02:26

Fix Logging

f574672

Refactoring

4f9bdfe

Implement JSON Caching

d9f55eb

Fix Tests

1176bfa

Remove redundant paramaters from _execute_build_rule

1f9d18b

Implement parallel execution using Popen polling

f516ae0

kaustubh-karkare approved these changes Nov 3, 2019

View reviewed changes

ankurdubey521 closed this Nov 3, 2019

		import subprocess


		def run(command_string, cwd, print_command=False):

		logger = logging.getLogger(__name__)
		logger.addHandler(logging.NullHandler())

		if self._root_dir_abs.endswith('/'):
		self._root_dir_abs = self._root_dir_abs[:-1]

		def get_last_build_pass_status(self) -> bool:
		return self._last_build_passed

		@@ -0,0 +1,2 @@
		/venv/
		.idea

Assignment 2.1: Build Automation Tool #60

Assignment 2.1: Build Automation Tool #60

Conversation

ankurdubey521 commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaustubh-karkare left a comment • edited Loading

Choose a reason for hiding this comment

kaustubh-karkare left a comment •

edited

Loading

kaustubh-karkare left a comment •

edited

Loading