vhdl-style-guide
vhdl-style-guide copied to clipboard
jeremiah-c-leary
An attempt at issue #353
Description An attempt at issue #353, it's some quick work with still some rough edges, but I wanted to put it already here to see if you would be interested in this attempt.
As I stated in issue #353 I think the current way vsg deals with configuration is not that convenient.
The way this pull requests deals with it:
When run without --configuration vsg will search for a configuration file for each file that is passed to --filename (if that's a folder it will recursively search in there). it searches for vsg_conf.yaml in the folder, and then in the parent folder, etc. After 25 levels or reaching a root git or hg folder it stops and applies the default configuration (searching in $XDG_CONFIG_HOME and/or another environment variable before adding default configuration can also be added). The first file found is used and nothing is merged.
When run with --configuration it does no discovery of configuration files.
Also, when run with --filename, the filenames in file_list from the configuration are ignored, but their specific rules still apply if the file passed to --filename is in there. (At least, that's what is supposed to happen, I still have to debug somewhat.)
Another thing that could be easily added is that --configuration also accepts directories. and finds all configuration files in that directory.
I think that covers most use cases mentioned in #367, with not that much added complexity:
-
vsg --configuration vsg_conf.yml --filename file.vhdwill work as it does now (except for also excluding the file_list) -
vsg --configuration .will find all configuration files and only work on the files infile_list -
vsg --filename .will find all vhld files and their configuration file. That could be one per project or different ones per folder.
Hey @Emilv2 ,
I must be doing something wrong, and wondering if you could help me out. My understanding is vsg --configuration . will search all directories underneath where I called it for file names vsg_conf.yaml and read them in. Here is a directory listing:
total 40
drwxr-xr-x 16 jcleary jcleary 4096 Jan 4 18:15 design
drwxr-xr-x 2 jcleary jcleary 4096 Jan 4 18:15 doc
drwxr-xr-x 5 jcleary jcleary 4096 Jan 4 18:15 tb
-rw-r--r-- 1 jcleary jcleary 7639 Jan 4 18:15 license-lgpl-3.0.txt
drwxr-xr-x 6 jcleary jcleary 4096 Jan 4 18:15 ..
drwxr-xr-x 17 jcleary jcleary 4096 Jan 4 18:19 design_fixed
-rw-r--r-- 1 jcleary jcleary 2103 Jan 4 18:27 config.yaml
-rw-r--r-- 1 jcleary jcleary 2103 Jan 4 18:38 vsg_conf.yaml
drwxr-xr-x 6 jcleary jcleary 4096 Jan 5 19:47 .
There is another vsg_conf.yaml buried under the design directory.
I was expecting to see both configurations ran, but when I issue vsg --configuration . I get the following:
jcleary@DESKTOP-HV9NHA3:~/projects/vsg_issue_353/hardening/mkjpeg/trunk$ vsg --configuration .
ERROR: Could not find configuration file: .
Traceback (most recent call last):
File "/home/jcleary/projects/vsg_issue_353/bin/../vsg/settings.py", line 46, in open_configuration_file
with open(sFileName) as yaml_file:
IsADirectoryError: [Errno 21] Is a directory: '.'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/jcleary/projects/vsg_issue_353/bin/vsg", line 14, in <module>
main()
File "/home/jcleary/projects/vsg_issue_353/bin/../vsg/__main__.py", line 380, in main
configuration = read_configuration_files(dStyle, commandLineArguments)
File "/home/jcleary/projects/vsg_issue_353/bin/../vsg/__main__.py", line 106, in read_configuration_files
tempConfiguration = settings.open_configuration_file(sConfigFilename, commandLineArguments.junit)
File "/home/jcleary/projects/vsg_issue_353/bin/../vsg/settings.py", line 51, in open_configuration_file
raise Exception
Exception
What am I missing?
--Jeremy
Hi, vsg --configuration . is something I'm still working on, I only thought of it when writing the pull request description. Just pushed an initial version.
I started working on it a bit, and it would be a bit more complex than I thought a first:
Applying the right to left order of precedence wouldn't be really possible and not desirable I think. That would mean that vsg --configuration . and vsg --filename . would apply a different order of rules. I would keep the same order as in vsg --filename . (the configuration file in the same directory takes precedence, then it's parent, etc. no merging of configuration files).
Another corner case is what to do if someone would try to mix directories and filenames. vsg --configuration . vsg_conf.yml
Just a note / question on the option --filename: As you know I currently define the list of files that VSG is supposed to run on via the file_list in the configuration directly.
So for me, --filename would mean to override that option (to some extend) and tie the check of VSG to only that file(s) given in the argument - although I would expect any configuration given in file_list for these files in question to still apply.
So let's say my file_list has that:
{
"file_list":[
"modules/fpga/context/*.vhd",
"modules/fpga/src/external/*.vhd",
"modules/fpga/src/private/components.vhd",
"modules/fpga/src/private/fpga_registers.vhd",
"modules/fpga/src/private/fpga_top.vhd":{
"rule":{
# https://github.com/jeremiah-c-leary/vhdl-style-guide/issues/487
# comments and blank lines
"block_200":{
"disable": true
},
},
},
"modules/fpga/src/private/s10_devkit_top_arch.vhd":{
"rule":{
# the top level has another allowed architecture name
"architecture_025":{
"names":[
"fpga_lasp_top",
],
},
"length_001":{
"disable":true,
},
"signal_016":{
"disable":true,
},
},
},
"modules/fpga/src/private/sctrl_*.vhd",
"modules/fpga/src/public/*.vhd",
"modules/fpga/simulation/private/fpga_top/fpga_top_tb.vhd":{
"rule":{
# VSG doesn't like the signal declaration over multiple lines...
# todo: create an issue
"signal_016":{
"disable": true
},
},
},
"modules/fpga/simulation/private/s10_devkit_top/s10_devkit_top_tb.vhd",
"modules/fpga/simulation/public/*/*.vhd",
"modules/fpga/target_hw/devkit/s10_devkit_top_ent.vhd":{
"rule":{
# the top level entity has any port name
"port_025":{
"disable": true
},
}
},
],
}
And I now simply run vsg --filename modules/fpga/target_hw/devkit/s10_devkit_top_ent.vhd, it should only check s10_devkit_top_ent.vhd, but still ignore rule port_025 as that's what's configured for that file anyway.
Similar, if I run vsg --filename modules/fpga/src/private, VSG should check all the files in that directory, again respecting rule configurations that apply from that file_list.
vsg --filename . should run on all files in the current directory (just like the glob extension of . does) - but not crawl the directory. Crawling could be added via a recursive option (-r)
And one thought on the location of the configuration files: What about an include syntax? Would that not solve the trouble of not knowing where to look for configuration files?
If the configuration files would support sth. like include ../my_config.json and again in ../my_config.json there could be an include and so on, it would be up to the project owner to set up the configuration files as needed, but VSG wouldn't have to make any assumption on repo or file system boundaries etc.
With both things implemented, I could imagine a call to VSG being as simple as vsg (no arguments at all) in the target directory: By default it would look in the current directory for a configuration file and explode it. In case if has a file list, you check these with all the rules. If no files are defined, you spit an error.
vsg --filename xyz.vhd would only check xyz.vhd, again trying to find the local configuration.
And finally, vsg --filename xyz.vhd --configuration xyz.json would only check xyz.vhd with rules derived from xyz.json (which might include what ever).
That effectively means that you could structure a project with a central or decentralised vsg configuration. To distribute a centralised version, simply add soft links to the central configuration in your directory structure (or have a file containing include ../vsg.json).
Just a note / question on the option --filename: As you know I currently define the list of files that VSG is supposed to run on via the file_list in the configuration directly. ... Similar, if I run vsg --filename modules/fpga/src/private, VSG should check all the files in that directory, again respecting rule configurations that apply from that file_list.
Yes, that is exactly the idea.
vsg --filename . should run on all files in the current directory (just like the glob extension of . does) - but not crawl the directory. Crawling could be added via a recursive option (-r)
I don't really see the use of providing a directory and not do it recursively, if you really want to do that you can just use a glob. Any other formatting tool that I know of that accepts directories crawls through them recursively. But in the end that's just details and this way or that way doesn't really matter to me.
And one thought on the location of the configuration files: What about an include syntax?
Interesting concept, but IMO that adds more possibilities to shoot yourself in the foot (imagine what happens if someone makes a circular include...). Then I like your idea of providing some kind of root config file more, although I still think that would make things more complicated for little gain.
Just a note / question on the option --filename: As you know I currently define the list of files that VSG is supposed to run on via the file_list in the configuration directly. ... Similar, if I run vsg --filename modules/fpga/src/private, VSG should check all the files in that directory, again respecting rule configurations that apply from that file_list.
Yes, that is exactly the idea.
OK, great.
vsg --filename . should run on all files in the current directory (just like the glob extension of . does) - but not crawl the directory. Crawling could be added via a recursive option (-r)
I don't really see the use of providing a directory and not do it recursively, if you really want to do that you can just use a glob. Any other formatting tool that I know of that accepts directories crawls through them recursively. But in the end that's just details and this way or that way doesn't really matter to me.
OK, I'm just thinking about being consistent: Any linux tool that I know interprets . as the current directory, and e.g. ls . will list you all the files of the current directory, but not any in subdirectories. ls -R . though list any file at any sublevel of that directory.
. is in fact already globbing, and since VSG supports globbing, I would only find it very obscure that VSG would behave differently here. Keep in mind that VSG has a file_list as configuration, not a directory_list. If it were a directory_list, then I agree, VSG should crawl, but then I'd actually also expect the parameter to be --directory . and not --filename ..
And one thought on the location of the configuration files: What about an include syntax?
Interesting concept, but IMO that adds more possibilities to shoot yourself in the foot (imagine what happens if someone makes a circular include...). Then I like your idea of providing some kind of root config file more, although I still think that would make things more complicated for little gain.
Circular includes would have to be spotted and complained about by VSG then, of course. The concept of includes is nothing new and wouldn't be unsolved. If you though like the idea of the root config file more, then sure, that would be a way to go that might be simpler to implement.
Maybe just one point for the implementation: There should then possibly somehow also be a way to dump the configuration - I mean a list of config files (their locations) such that the user could easily double check how the configuration is assembled. I know that a simple grep for the config files should do the trick, but it could add a little shine to VSG.
OK, I'm just thinking about being consistent: Any linux tool that I know interprets . as the current directory, and e.g. ls . will list you all the files of the current directory, but not any in subdirectories. ls -R . though list any file at any sublevel of that directory.
I was thinking about being consistent with other formatters (black/isort/gofmt that I know of). find and du are other examples of tools that go recursive by default, so it's not unheard of.
. is in fact already globbing, and since VSG supports globbing, I would only find it very obscure that VSG would behave differently here. Keep in mind that VSG has a file_list as configuration, not a directory_list. If it were a directory_list, then I agree, VSG should crawl, but then I'd actually also expect the parameter to be --directory . and not --filename ..
. is not globbing, it's the name of the current directory in Unix. I don't really see why --directory should be recursive by default and --filename on a directory not (directories are also files:), but I guess it's all just a matter of taste.
Maybe just one point for the implementation: There should then possibly somehow also be a way to dump the configuration - I mean a list of config files (their locations) such that the user could easily double check how the configuration is assembled. I know that a simple grep for the config files should do the trick, but it could add a little shine to VSG.
Agree. I'd say some kind of --verbose with different levels. Shouldn't be too complicated I think.
OK, I'm just thinking about being consistent: Any linux tool that I know interprets . as the current directory, and e.g. ls . will list you all the files of the current directory, but not any in subdirectories. ls -R . though list any file at any sublevel of that directory.
I was thinking about being consistent with other formatters (black/isort/gofmt that I know of).
findandduare other examples of tools that go recursive by default, so it's not unheard of.
I'm not looking at other formatters. I'm only pointing out that VSG should be self-consistent.
If crawling is implied when giving . as an argument, then fine, but I'd find this inconsistent with the other way of configuring it where you could give a glob as a filename, e.g. .*.vhd - then you'd get any vhd files in the current directory.
That is how globbing works (as far as I understood it).
I would simply find the option --filename . to crawl recursively inconsistent with that alternative. If, on the other hand, -r would be added as recursive, then sure, -r --filename . should crawl recursively (and I agree, if it's filename or directory doesn't matter in the end - as long as it is all self-consistent (and documented)).
. is in fact already globbing, and since VSG supports globbing, I would only find it very obscure that VSG would behave differently here. Keep in mind that VSG has a file_list as configuration, not a directory_list. If it were a directory_list, then I agree, VSG should crawl, but then I'd actually also expect the parameter to be --directory . and not --filename ..
.is not globbing, it's the name of the current directory in Unix. I don't really see why--directoryshould be recursive by default and--filenameon a directory not (directories are also files:), but I guess it's all just a matter of taste.
Well, ok, point taken for .. --directory vs. --filename just to make clear that the first doesn't crawl, the latter does.
Again, the alternative would be to add an -r option.
Maybe just one point for the implementation: There should then possibly somehow also be a way to dump the configuration - I mean a list of config files (their locations) such that the user could easily double check how the configuration is assembled. I know that a simple grep for the config files should do the trick, but it could add a little shine to VSG.
Agree. I'd say some kind of
--verbosewith different levels. Shouldn't be too complicated I think.
Also logging is a solved issue. Could be as simple as --log <level> (see logging - do not reinvent anything that has already been solved). (for me "verbose" was considered a log level, but I seem to be mistaken actually, so maybe --verbose <level> would also be fine, although that looks odd to me - but we all learn ... PS: also compare to the C++ boost log levels)
Again, the alternative would be to add an -r option.
Would you even need the --filename option if you use -r?
I see your point, but I don't think it's inconsistent since .*.vhd doesn't match the directory. (I don't even think most shells (and python?) make .* match . but I'm actually not sure). Adding the recursive option is fine for me.
Totally agree about not reinventing anything. I was under the impression that --verbose (with the possibility of adding it several times for more verbosity, like -vvv) was the most common way of doing this but I could be wrong.
Would you even need the
--filenameoption if you use-r?
I think that would be confusing. What about searching for config files, should that also be non recursive by default?
Again, the alternative would be to add an -r option.
Would you even need the
--filenameoption if you use-r?
Hi @jeremiah-c-leary,
I guess your point is that -r necessarily need --filename and hence --filename could directly be removed!?
Well, you remember that I pointed to you that VSG currently is using a mixture of long (--) and short (-) options and this is not really a good idea (and really not good practice).
Unix tools use short options (-) throughout (at least the very basic ones) and long options have been added later (out of which some have an alternative short option, --help | -h or --version | -vare prominent examples.
Now it always depends on what the particular option does, but in the case of recursive, this is (at least in my eyes) a clear extension of the --filename option - in the sense that the argument following it would actually be a directory (I discussed that above already).
So -r itself would just be a flag, but doesn't take any argument.
Hey @Emilv2 ,
I am planning to release 3.0.0, hopefully soon, and I was wondering how this effort was going and whether I should hold off.
Thanks,
--Jeremy
I don't think this should be rushed, better take some time to decide what we want to put in and be sure it works.