Fixed bug in HLS parser

[tomalin]

1) https://github.com/cms-L1TK/project_generation_scripts/blob/master/WriteHDLUtils.py#L850 crashes when parsing the top-level ME code, as latter now uses more complex templating. I've added a bug fix to handle this correctly. 2) https://github.com/cms-L1TK/project_generation_scripts/blob/master/WriteHDLUtils.py#L1039 incorrectly duplicated ports of the TrackBuilder. Now removed. 3) Option "-h" of generate_hdl.py and makeReducedConfig.py now prints default value of each command line argument. 4) Use of order-preserving "set" to keep signals in SectorProcessor.vhd in sensible order. 5) Minor cosmetic changes.

N.B. After this is merged, branch "ir_phibinword_update" will be rebased on top of it.

[tomalin]

@aehart should please check code near https://github.com/cms-L1TK/project_generation_scripts/blob/master/WriteHDLUtils.py#L1039 was deleted as expected.

[tomalin]

@pwittich as you're the expert on .github/workflows/python-app.yml , any idea why the auto code checks failed with error "pylint: error: no such option: --py3k"?

[aperloff]

I'm not @pwittich , but I can tell you that the --py3k option was removed from the later Python3 versions of pylint. The rational is that the --py3k option was meant to check a piece of python2 code for python3 compatibility. It was never meant to check python3 code for python3 compatibility, which is somewhat redundant. For that you should use the standard linting options. So the authors of pylint made the decision to remove the option for any version of pylint which is a python3 module.

[tomalin]

I'm not @pwittich , but I can tell you that the --py3k option was removed from the later Python3 versions of pylint. The rational is that the --py3k option was meant to check a piece of python2 code for python3 compatibility. It was never meant to check python3 code for python3 compatibility, which is somewhat redundant. For that you should use the standard linting options. So the authors of pylint made the decision to remove the option for any version of pylint which is a python3 module.

Do you suggest that we simply delete line https://github.com/cms-L1TK/project_generation_scripts/blob/master/.github/workflows/python-app.yml#L31 , or somethign else?

[aperloff]

Do you suggest that we simply delete line https://github.com/cms-L1TK/project_generation_scripts/blob/master/.github/workflows/python-app.yml#L31 , or somethign else?

Yes, I would suggest removing that line and then uncommenting https://github.com/cms-L1TK/project_generation_scripts/blob/master/.github/workflows/python-app.yml#L33. If that's throwing too many errors or enforcing too many style constraints, then just add a .pylintrc file and selectively turn on the tests you'd like it to run.

[tomalin]

Do you suggest that we simply delete line https://github.com/cms-L1TK/project_generation_scripts/blob/master/.github/workflows/python-app.yml#L31 , or somethign else?

Yes, I would suggest removing that line and then uncommenting https://github.com/cms-L1TK/project_generation_scripts/blob/master/.github/workflows/python-app.yml#L33. If that's throwing too many errors or enforcing too many style constraints, then just add a .pylintrc file and selectively turn on the tests you'd like it to run.

Done. Though pylint reports almost 2000 errors. If you coukd add a .pylintrc file, that would be great (I'm no expert). Otherwise, I'll comment this check out.

[pwittich]

the idea for the py3k check was to run it under py2, to ensure that no py2-only features creep in. @aperloff is that no longer supported? I don't think we had decided to drop support of py2.

I'm not eager to unleash pylint with its 2000 errors.

[aperloff]

@pwittich From my reading of python-app.yml python 3.9 is installed and then pylint is used from that version of python. From what I can tell --py3k has been removed from all python3 versions of pylint (https://github.com/DataDog/documentation/pull/6238). If you'd still like to check python3 compatibility and prevent regressions to python2, then we would need to modify python-app.yml to install python2 and run pylint from there.

That said, if you ever did want to turn on linting, then you'd want to do that from within python3 as the recommendations changed between the two versions.

[tomalin]

I've added https://github.com/cms-L1TK/project_generation_scripts/blob/ian_HLSparser/.pylintrc to suppress all warning & informational messages, and leave only the error messages.

This left 2 error messages, caused by it not being able to import either ROOT or rich.traceback. How should we fix those? For the moment, I suppress them by asking .pylintc to suppress error message E0401, so the pylint check passes.

I've checked that if I insert a python2 style "print" statement into the scripts, pylint throws an error.

[tomalin]

On my local PC, I need to use commands pylint3 and python3 to run the python 3 commands. (python & pylint use python 2). Should we change top line of generator_hdl.py to "#!/usr/bin/env python3" and replace pylint by pylint3 in python-app.yml?

[aperloff]

On my local PC, I need to use commands pylint3 and python3 to run the python 3 commands. (python & pylint use python 2). Should we change top line of generator_hdl.py to "#!/usr/bin/env python3" and replace pylint by pylint3 in python-app.yml?

You should replace the default executable as the top if you intend the script to be run using python3 rather than letting the system decide (i.e. whatever python is linked to).

Since the virtual environment of the GitHub runner only contains python 3.9 it's likely that the link python is also linked to python3 which is then linked to python3.9. Mostly likely there isn't a python2 with which to get confused. That said, the best practices would say to use python3 and pylint3 or python3 -m pylint.

This left 2 error messages, caused by it not being able to import either ROOT or rich.traceback. How should we fix those? For the moment, I suppress them by asking .pylintc to suppress error message E0401, so the pylint check passes.

Your options are to:

1. Install the dependencies. That's fine for the rich module, but installing ROOT would be a pain and probably a wast of time for one error.
2. You could suppress E0401 with the line # pytlint: disable=E0401 right above the import and the opposite # pylint: enable=E0401 right below the import ROOT line. But doing this type of thing litters the code with # pylint statements.
3. You can turn off E0401 global, as you have already done.

[pwittich]

I've checked that if I insert a python2 style "print" statement into the scripts, pylint throws an error.

is this running py2 or py3? On py3 that's just a straight compile error; the use case of the py3k check was to ensure that we don't run into problems with py2 users.

we could just require py3. Are people happy with this?

[pwittich]

@pwittich From my reading of python-app.yml python 3.9 is installed and then pylint is used from that version of python. From what I can tell --py3k has been removed from all python3 versions of pylint (DataDog/documentation#6238). If you'd still like to check python3 compatibility and prevent regressions to python2, then we would need to modify python-app.yml to install python2 and run pylint from there.

yes I was thinking we should just add a second part in the yaml file that runs pylint with the py3k for py27 or some similar version.

Or we just drop py2 support.

[tomalin]

@aperloff & @pwittich Since this PR is now approved "approved" and urgently needed, since the scripts will not run without it, I propose to merge it now. We can make a further PR in response to the pythia3/pylint discussions above if we conclude that it is needed.

[aperloff]

@tomalin I'm fine with that.