Conversation
|
Hi Erik, thank you for your contributions to Hammer! :) A few things:
Thanks again for your contribution! |
colinschmidt
left a comment
colinschmidt
left a comment
There was a problem hiding this comment.
Looks pretty reasonable.
I assume you have tested this locally?
| def get_and_prepend_path(lib: Library) -> Tuple[Library, List[str]]: | ||
| paths = filt.paths_func(lib) | ||
| full_paths = list(map(lambda path: self.prepend_dir_path(path, lib), paths)) | ||
| full_paths = list(map(lambda path: self.prepend_dir_path(path, lib) if path else '', paths)) |
There was a problem hiding this comment.
Why did this change? Is this just a bugfix?
There was a problem hiding this comment.
prepend_dir_path has an assertion that checks to make sure the path is not empty. I am not entirely sure why the assertion exists, but this was my solution to get an empty string to be returned
There was a problem hiding this comment.
Hmm, how do empty paths end up showing up to begin with? And how is this cap table specific?
There was a problem hiding this comment.
Well because you only need a cap_table_file OR a qrc_techfile then 1 of them can be empty. Previously hammer would error if you did not supply a qrc_techfile even though the function "generate_mmmc_script" already assumed that if there was no supplied qrc_techfile that an empty string would be retrieved when calling "get_mmmc_qrc". I guess this raises the question of whether we actually want an error to be thrown if neither of these files are supplied
There was a problem hiding this comment.
Hm, seeing how other things use prepend_dir_path, I also don't see what the value of the len(path) > 0 assertion is. It is only used in certain cases (DRC/LVS decks, map files) but doesn't make whatever you specified in the tech json any more or less correct.
I think what we really should do is call an os.path.exists on the constructed path so that Python can catch bad paths in tech jsons before it's passed to the tool where sometimes at best only a warning is thrown.
There was a problem hiding this comment.
If there are no qrc techfiles, then wouldn't get_mmmc_qrc() return an empty string since the read_libs call would return an empty list?
There was a problem hiding this comment.
As a side note, we really need to detach the QRC techfile from the libraries (#315).
There was a problem hiding this comment.
Maybe we can incorporate that larger change into this PR, so that we aren't creating more work we need to undo/fix later?
|
|
Great! Might be good to add a link to https://github.com/The-OpenROAD-Project/PEX/blob/master/kits/nangate45/captables/NCSU_FreePDK_45nm.capTbl in the yaml comments/documentation for reference. |
|
Update on this: Added a test for the filter in tech_test.py and also added a property to the Library stub class for type checking purposes. Not entirely sure why only some properties from the jsonschema generated Library class need to be defined in the stub class, but mypy no longer errors. |
edwardcwang
left a comment
edwardcwang
left a comment
There was a problem hiding this comment.
Generally looks good but have a further question about this line.
| def get_and_prepend_path(lib: Library) -> Tuple[Library, List[str]]: | ||
| paths = filt.paths_func(lib) | ||
| full_paths = list(map(lambda path: self.prepend_dir_path(path, lib), paths)) | ||
| full_paths = list(map(lambda path: self.prepend_dir_path(path, lib) if path else '', paths)) |
There was a problem hiding this comment.
Hmm, how do empty paths end up showing up to begin with? And how is this cap table specific?
|
Can we also add |
5 participants
This allows for specifying a "cap table file" in the tech.json for older technologies that do not come with a qrc techfile. There should not be any issue with supplying both a cap table file and qrc tech file. The tool should always prioritize the qrc tech file. I also edited the function "get_and_prepend_path" inside the "process_library_filter" function so that if an empty path is retrieved from the tech.json then an empty string will be returned. This behavior appears to have already been expected in the "get_mmmc_qrc" function.