Adding csv mapping support for files and libraries #349

davidmedinasigasi · 2018-06-25T13:47:34Z

No description provided.

kraigher · 2018-06-25T13:58:11Z

vunit/ui.py

+            for lib_name in libs_and_files.keys():
+                files = libs_and_files[lib_name]
+                lib = self.add_library(lib_name)
+                lib.add_source_files(files)


Other functions that add files return the list of files added.

kraigher · 2018-06-25T13:59:50Z

vunit/ui.py

+            for row in content:
+                if len(row) == 2: 
+                    lib_name = row[0].strip()
+                    file_name = row[1].strip()


It would be reasonable to interpret the file name as relative to the location of the .csv file or an absolute path rather than relative to the current working directory when this code is executing,

What I mean with the file name in this context is the file name that is written inside the csv file.

For example if a have the following structure:
/directory/proj.csv
/directory/file.vhd
/run.py

If proj.csv contains "lib, file.vhd" and I run "python run.py" in the root directory it should refer to /directory/proj.csv making the semantics of the proj.csv file independent on the current working directory where the run.py script is executed.

I understand what you want me to do, I just don't know how, can you please explain a little more?

You can achieve what I describe by.

from os.path import normpath, join, dirname normpath(join(dirname(csv_file_name), file_name)) It will make file_name relative to the parent directory of the csv file unless file name is an absolute path already.

kraigher · 2018-06-25T14:00:58Z

vunit/ui.py

@@ -393,6 +394,36 @@ def add_external_library(self, library_name, path, vhdl_standard=None):
        self._project.add_library(library_name, abspath(path), vhdl_standard, is_external=True)
        return self.library(library_name)

+    def add_csv(self, project_csv_path, vhdl_standard=None):


add_source_files_from_csv would be a better name

kraigher · 2018-06-25T14:01:54Z

vunit/ui.py

@@ -393,6 +394,36 @@ def add_external_library(self, library_name, path, vhdl_standard=None):
        self._project.add_library(library_name, abspath(path), vhdl_standard, is_external=True)
        return self.library(library_name)

+    def add_csv(self, project_csv_path, vhdl_standard=None):
+        """
+        Add a project configuration, mapping all the libraries and files


The description does not describe the format of the csv file or the meaning of relative paths of the file names.

I have some WIP related to a similar approach with JSON:

https://github.com/1138-4EB/vunit/tree/json_cfg_run

where the 'generic' run-py contains:

https://github.com/1138-4EB/vunit/blob/f67be68b6e8a114a69f1ac67dba92b6f79de11c2/examples/vhdl/array_axis_vcs/cfg_run.py#L16-L20

https://github.com/1138-4EB/vunit/blob/f67be68b6e8a114a69f1ac67dba92b6f79de11c2/examples/vhdl/array_axis_vcs/cfg_run.py#L35-L48

and the example JSON itself is:

eine@f67be68#diff-c8cec296822f0068205fef2fce6117ac

I'd be glad if the format/approach was explicitly documented, in order to follow the same pattern when I return to it.

kraigher · 2018-06-25T15:13:46Z

vunit/ui.py

@@ -422,7 +424,7 @@ def add_csv(self, project_csv_path, vhdl_standard=None):
            for lib_name in libs_and_files.keys():
                files = libs_and_files[lib_name]
                lib = self.add_library(lib_name)
-                lib.add_source_files(files)
+                return lib.add_source_files(files)


bug: return inside for-loop only works for single library in the csv file

kraigher · 2018-06-26T07:56:36Z

vunit/ui.py

            for lib_name in libs_and_files.keys():
                files = libs_and_files[lib_name]
                lib = self.add_library(lib_name)
-                return lib.add_source_files(files)
+                list_of_source_files.append(lib.add_source_files(files))


This will create a list of lists and not a single list of all files added.

kraigher · 2018-06-26T08:05:06Z

vunit/ui.py

@@ -426,7 +426,10 @@ def add_source_files_from_csv(self, project_csv_path, vhdl_standard=None):
                files = libs_and_files[lib_name]
                lib = self.add_library(lib_name)
                list_of_source_files.append(lib.add_source_files(files))
-            return list_of_source_files
+
+            list_of_source_files_flattened = [val for sublist in list_of_source_files for val in sublist]


+= would have been nicer. Also it seems this should have a unit test since there were so many bugs.

kraigher

Almost there now. Some things left though.

kraigher · 2018-06-26T08:50:36Z

vunit/test/unit/test_ui.py

+
+        source_files = ui.add_source_files_from_csv('test_returns.csv')
+
+        for i in range(len(source_files)):


This test is flawed. source_files could just be empty or miss the last element and the test would still pass. Use list_of_files as the iteration length source instead. Or even better just compare the lists directly with self.assertEqual([source_file.name for source_file in source_files, list_of_files)

kraigher · 2018-06-26T08:51:30Z

vunit/ui.py

+    def add_source_files_from_csv(self, project_csv_path, vhdl_standard=None):
+        """
+        Add a project configuration, mapping all the libraries and files
+        :param project_csv_path: path to csv project specification, each line contains the name 


The documentation should also say that relative file names are relative to the parent folder of the csv file.

kraigher · 2018-06-26T08:52:27Z

vunit/ui.py

+                elif len(row) > 2:
+                    LOGGER.error("More than one library and one file in csv description")
+
+            list_of_source_files = list()


= SourceFileList()

Otherwise the return value will be a list() and not a SourceFileList like the documentation claims.

kraigher · 2018-06-26T09:44:22Z

Good now you just have to fix the lint violations to make the Travis CI check pass.
You can run pylint and pycodestyle locally as decribed here: http://vunit.github.io/contributing.html#running-the-tests

pytest vunit/test/lint/

kraigher · 2018-06-26T13:37:06Z

vunit/test/unit/test_ui.py

+
+        for index, library_name in enumerate(libraries):
+            file_name = files[index]
+            file_name_from_ui = ui.add_source_files_from_csv(file_name, library_name)


This line does not make sense. You add the file_name as a csv file but the file is an empty vhdl_file.
Did you mean to do get_source_file?

kraigher · 2018-06-26T13:37:48Z

vunit/test/unit/test_ui.py

+        """
+
+        libraries = ['lib', 'lib1', 'lib2']
+        files = ['tb_example.vhdl', 'tb_example1.vhd', 'tb_example2.vhd', ' tb,ex3.vhd']


tb,ext3.vhd is unnessary?

kraigher · 2018-06-27T11:31:24Z

vunit/ui.py

+                    lib = self.library(lib_name) if lib_name in libs else self.add_library(lib_name)
+                    libs.add(lib_name)
+                    file_ = lib.add_source_file(file_name_)
+                    if file_before is not None:


This circumvents the dependency scanning performed by VUnit, why are you creating manual dependencies? VUnit will still can the files added for dependencies unless you add the no_parse argument to add_source_file.

kraigher

What is the reason you do not want to use the VUnit dependency scanning? Just having a linear dependency order is much worse. Since the order is linear some files later in the list will not depend on files further up in the list and thus it will cause unnecessary compilation. A compile order should be a tree and not a list.

kraigher · 2018-06-27T12:33:08Z

vunit/ui.py

@@ -393,6 +394,46 @@ def add_external_library(self, library_name, path, vhdl_standard=None):
        self._project.add_library(library_name, abspath(path), vhdl_standard, is_external=True)
        return self.library(library_name)

+    def add_source_files_from_csv(self, project_csv_path, vhdl_standard=None, is_in_order=True):


The default should not be to disable the VUnit dependency scanning.

kraigher · 2018-06-27T12:33:42Z

vunit/ui.py

+                    file_name_ = normpath(join(dirname(project_csv_path), no_normalized_file))
+                    lib = self.library(lib_name) if lib_name in libs else self.add_library(lib_name)
+                    libs.add(lib_name)
+                    file_ = lib.add_source_file(file_name_, no_parse=is_in_order)


No parse will also inhibit any test_benches/tests from being detected from the source file.

When sigasi generates the csv file, we resolve all dependencies in a way that the first file is a dependency for the next one, for example
lib, example.vhd
lib2, example2.vhd // This one depends on lib, example.vhd
lib, example3.vhd // This one depends on lib2, example2.vhd
lib3, example4.vhd // This one depends on lib, example3.vhd

Dependencies are never linear they are a tree. To force them to be linear creates a lot of unnecessary re-compilation.

Consider the case with 200 files with 20 groups of 10 files. Each group has a linear dependency but the groups are independent. If you can only express a linear order some group will be first and some group will be last. If a file in the first group is modified it causes 200 files to be recompiled instead of only 10.

Sigasi for sure has a dependency tree internally. It is only when you need to generate a sequence of compile commands for the initial compile that it is useful to generate an ordered list of files. For subsequent incremental compiles which is a big feature of VUnit a new ordered list of compile commands has to generated based on the files changes and the dependency tree.

In summary: I think skipping the VUnit dependency scanning in favor of a linear dependency list from Sigasi is a bad idea that will lead to worse re-compilation times. Also as the feature is implemented now it does not work since no_parse=True will also inhibit VUnit form recognizing any tests in the added files (which probably is not what you want). My recommendation would be to skip the is_in_order=True variant completely.

We're planning to improve how Sigasi export the csv description. Right now we want a fist approach to the integration with Vunit. We're thinking to optimize the compile order generated by Sigasi in the near future

kraigher · 2018-06-27T16:13:13Z

The function to get built in libraries should not be a part of this pull request. It should be reviewed separately. You need to use feature branches in your fork to separate the issues.

This reverts commit 5470ab1.

kraigher · 2018-06-28T09:25:52Z

vunit/ui.py

@@ -393,6 +394,46 @@ def add_external_library(self, library_name, path, vhdl_standard=None):
        self._project.add_library(library_name, abspath(path), vhdl_standard, is_external=True)
        return self.library(library_name)

+    def add_source_files_from_csv(self, project_csv_path, vhdl_standard=None, is_in_order=False):


The way is_in_order works now is just bad. VUnit will still scan all files for dependencies in addition to the sub-optimal linear dependency list enforced on top by is_in_order=True. I think you should remove the is_in_order option from this function as it does not make sense as implemented. I think the option should be removed and no manual dependency order defined.

davidmedinasigasi added 5 commits June 25, 2018 10:36

adding csv mapping for files and libraries

0d44436

Adding test and improving csv read

6306658

removing print

c604c0c

changing the test and using csv lib

9196108

Adding error msg

d5016de

kraigher requested changes Jun 25, 2018

View reviewed changes

Chaning variable names and adding absolute path to csv reader

f88512c

kraigher reviewed Jun 25, 2018

View reviewed changes

Adding normalized paths

23fab86

kraigher requested changes Jun 26, 2018

View reviewed changes

Adding flattend list in return

5c92f4d

kraigher requested changes Jun 26, 2018

View reviewed changes

Adding test for csv return and refactoring

41f9140

kraigher requested changes Jun 26, 2018

View reviewed changes

davidmedinasigasi added 2 commits June 26, 2018 11:02

Changing test and return in csv implementation

56f7f6d

Adding more documentation

3bc33bb

davidmedinasigasi added 3 commits June 26, 2018 13:28

Adding code style

d76881b

Trying to finish docs

79008ad

Fixing docs style

e190d26

kraigher requested changes Jun 26, 2018

View reviewed changes

davidmedinasigasi added 3 commits June 27, 2018 10:03

Adding dependcy on files and fixing test

b648c1c

Adding test for dependencies and fixing dependency calculation

a11dc12

Syncing with remote repo and merging

2f79794

kraigher reviewed Jun 27, 2018

View reviewed changes

davidmedinasigasi added 2 commits June 27, 2018 14:29

Changing dependencies calculations

c8f8287

Fixing lint trailing whitespace

a6e9085

kraigher reviewed Jun 27, 2018

View reviewed changes

davidmedinasigasi added 2 commits June 27, 2018 15:13

Changing default is it order parameter

4e34963

Fixing tests

9377d95

Adding function to ger all built in libraries

5470ab1

davidmedinasigasi added 2 commits June 28, 2018 09:50

Revert "Adding function to ger all built in libraries"

f29f706

This reverts commit 5470ab1.

Fixing no parse

7362208

kraigher requested changes Jun 28, 2018

View reviewed changes

Removing linear dependency order in function

7de6d4b

kraigher merged commit d2e7bd8 into VUnit:master Jun 28, 2018

nathanaelhuffman pushed a commit to nathanaelhuffman/vunit that referenced this pull request Jul 30, 2018

Adding csv mapping support for files and libraries (VUnit#349)

d9fc0dc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding csv mapping support for files and libraries #349

Adding csv mapping support for files and libraries #349

davidmedinasigasi commented Jun 25, 2018

kraigher Jun 25, 2018

kraigher Jun 25, 2018

kraigher Jun 25, 2018

davidmedinasigasi Jun 25, 2018

kraigher Jun 25, 2018

kraigher Jun 25, 2018

kraigher Jun 25, 2018

eine Jun 25, 2018

kraigher Jun 25, 2018

kraigher Jun 26, 2018

kraigher Jun 26, 2018

kraigher left a comment

kraigher Jun 26, 2018

kraigher Jun 26, 2018

kraigher Jun 26, 2018

kraigher commented Jun 26, 2018

kraigher Jun 26, 2018

kraigher Jun 26, 2018

kraigher Jun 27, 2018

kraigher left a comment

kraigher Jun 27, 2018

kraigher Jun 27, 2018

davidmedinasigasi Jun 27, 2018

kraigher Jun 27, 2018

kraigher Jun 27, 2018

kraigher Jun 27, 2018

kraigher Jun 28, 2018

davidmedinasigasi Jun 28, 2018

kraigher commented Jun 27, 2018

kraigher Jun 28, 2018


		source_files = ui.add_source_files_from_csv('test_returns.csv')

		for i in range(len(source_files)):

Adding csv mapping support for files and libraries #349

Adding csv mapping support for files and libraries #349

Conversation

davidmedinasigasi commented Jun 25, 2018

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

kraigher 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

kraigher commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraigher 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

kraigher commented Jun 27, 2018

Choose a reason for hiding this comment