Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record length linting does not consider macro substitutions #309

Open
ZLLentz opened this issue Jun 12, 2023 · 8 comments
Open

Record length linting does not consider macro substitutions #309

ZLLentz opened this issue Jun 12, 2023 · 8 comments

Comments

@ZLLentz
Copy link
Member

ZLLentz commented Jun 12, 2023

The following records in my project may or may not be above length 60, but the linter can't actually know because "$(PREFIX)" will be subbed out.

pytmc.bin.db.LinterError: Records exceeding the maximum length of 60 were found:
    $(PREFIX):SIM:XPIM:ZOOM:PLC:stEPSBackwardEnable:sFlagDesc_RBV
    $(PREFIX):SIM:XPIM:FOCUS:PLC:stEPSBackwardEnable:sFlagDesc_RBV
    $(PREFIX):SIM:XPIM:FOCUS:PLC:stEPSBackwardEnable:sMessage_RBV
    $(PREFIX):SIM:XPIM:FOCUS:PLC:stEPSForwardEnable:sFlagDesc_RBV
    $(PREFIX):SIM:XPIM:MMS:STATE:PMPS:TRANS:BP:BeamClassRanges_RBV

The current behavior is to assume the replaced string is the same length as the macro string. In this case, the prefix is assumed to be 9 characters long. The PLC prefix is actually length 11 in this case, which means the linter check here probably misses additional records that are also above length 60. (In my case, I definitely need to trim some prefix off)

My worry is that for similar cases the linter is failing to catch the maximum length check and letting through some records that will be too long after a macro substitution.

@klauer
Copy link
Contributor

klauer commented Jun 12, 2023

A duplicate of #254 but described here in much better detail.

As you say, we can't really know until the IOC boots up and subs in macros. I'm not sure of a good path forward for this and am definitely open to suggestions.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2023

I searched for old issues but didn't search well enough, whoops

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2023

My main ideas are:

  • add a warning threshold at ~50
  • make some assumptions about known in-use macros based on their maximum lengths in prod

@klauer
Copy link
Contributor

klauer commented Jun 12, 2023

So - "if macro included and non-macro part is about 50 chars, warn"?

I think that's pretty reasonable, though I'm not sure how it would get back to the user. Errors are easy because the linter fails with a specific return code in an obvious fashion, whereas warnings require someone to be paying attention to the output even in the case of success.

Integrated with GHA, annotating a specific line wouldn't be easy to implement. Warnings in the summary output would be OK - but would remain somewhat hidden. Hmm...

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2023

Maybe simplest is we go around to all the prod configs, see which macros are in use, and then see how long the substitutions typically are. Then in the linter it's straightforward, we automatically sub in $(PREFIX) -> 12 characters or whichever the longest is plus some buffer.

I'd rather this error extra than not enough and PV names at around 57 should still be shortened.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2023

Another option is to allow some level of macro sub length checking based on input args during make

@NSLentz
Copy link

NSLentz commented Oct 25, 2024

@ZLLentz
Why not have the linter check the makfile for macros defined there, and just do a "best I can do" approach? We can't lint custom macros, but the PREFIX is defined in the makefile every time, so if the iocBoot folder exists and there is a makefile and an ioc, could it check the makefile?

@ZLLentz
Copy link
Member Author

ZLLentz commented Oct 28, 2024

This is true in principle, but remember that pytmc is agnostic to your specific ioc/project structure, so we need to be a bit careful in how we implement something like this.

Possibly the simplest is to add a command line argument that ingests macros and apply e.g. PREFIX to it in https://github.com/pcdshub/ioc-common-ads-ioc/blob/81acb0dae4f5e54434d9f3b7d61f3cee7d280aba/iocBoot/templates/Makefile.base#L107-L114

I guess through the above it's worth noting that the length check is actually launched from the aforementioned Makefile, so the info in that Makefile is somewhat trivially fair game to be used as an input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants