How should <branch/change checkers> assemble the list of files to check ? #UMDP3_check

[r-sharp]

This question arises from looking at UMDP3 checker, but it could be applied to any scripts we want to target code being changed.

The original UMDP3 checker was grown not designed. This means it has several, completely separate ways of deciding what files to check based on branch v trunk v WC and command line v suite
Originally, IIRC, as a script run manually on a WC it used output from 'fcm bdiff' to get a list of files to check.

This doesn't work in quite the same way in most other circumstances though.

• in rose-stem it can only see changes committed to the (mirror) branch
• when dealing with the trunk, bdiff produces no results
• When rose-stem is run on a 'test branch' it tries to auto correct to the dev branch.

Ultimately this means that the current structure of the (AI translated code) branches quite early on based on "is trunk" and "is suite" rather than calling a "get checklist" function which could handle a vairety of methods beghind the scene.

The result is a range of code specific to the runtime circumstances (umdp3 checker really is about 3 separate codes with little overlap in anything other than intent...)

Secondary issues are that when working on a WC (and possibly a branch, but not in rose-stem ==I think==) it also relies on the full output of fcm bdiff to only check the changed lines. Or when run in rose stem, it tries to decide if "other repos" (linked tickets) are in play and then tries to check the code in them too - but doesn't have a branch - only the fully extracted code to look at.

At present, the git version of 'fcm bdiff', while superior in many ways to the actual 'fcm bdiff', does not have the option to generate a list of changed/added/deleted lines - It could be added, however I'm not sure we should be restricting the checks to that level....

And /finally/ the output of UMDP3 checker in the automatic tests on the trunk (nigtly/weekly) produce so much spurious output, it seems noone is looking at them any more.

So the question is : what is it we want any change checker to check ?

1. All the code ?
2. or only the changed files ?
3. or only the changed lines ?

approach 1 has the following issues :

• time/performance - there might be a lot of code to check.
• spotting 'issues' that pre-existed the change in question (devs don't like this)
  approach 2 can still spot 'issues' that aren't a result of the change, but less of them as it's only dealing with a smaller subset of files.

approach 3 is only available when looking at a branch, and has the small hickup that in my tests, it fails to highlight the removal of an "implicit None" as that's not an issue on 'a line' but requires a check which looks at the whole file - which it's currently not getting..

So - as we prgogress from FCM to Git - and I'm looking at it anyway, if we were designing something to target checks within a change - what would we want it to do ?

[jennyhickson]

"All the code" only works if the code is in a good state or it is unreasonable for us to expect devs to make it pass. With other linters this means we generally use expections to exclude rules we can't make pass by default (and have good intentions about sorting out the preexisting issues) - but I'm not sure this is the way to go with the UMDP3 checker.

I have less opinions on 2 vs 3 - I think reliably picking up errors is more important, and 2 is certainly easier to acheive that with. The dev might also fix up other things highlighted "while they're there" which is always nice. If they're saddled with the checker failing and its not their fault then we've always had a policy of just explaining that on the ticket.

If going for 2 or 3 then theres still the question of what we want it to do if you're on trunk/main/stable. Report errors in all files makes most sense - though as you noticed, theres so much output at the moment that this is only useful if you're looking at it with the intent of fixing some. Its not possible to use it to catch cases of new errors slipping in.

One curveball option would be to always do the "check all the code" option (assuming time/performance are possible), but have all pre-existing errors listed in a "known about" file. This would mean that both branches and trunks would only report new issues and allow devs and nightly testing to accurately flag that things are getting worse. This is different to the "exclude rules we can't pass" process because we are still applying all rules to new code. We could then reduce the "known about" file in any way we choose as its a clear "tech debt" list.

[james-bruten-mo]

Not sure I have much more to add than Jenny.

I quite like the idea of just using lines - it'll reduce the number of spurious errors we get and therefore any errors it does pick up are easier to say, this must be fixed. How hard do you think it'd be to just check lines changed?

Running on the "trunk", aside from looking for a list of things to fix, isn't useful as we know it'd just output a few different errors. One option for nightly testing therefore might be to generate a diff based on changes in the last day - scraping the hashes and commit times from the git log must be possible, and then the diff (either files or lines) could be generated from that?

[r-sharp]

Curiously, whilst reading Jenny's comments, I was thinking along similar lines to what James is suggesting for "trunk".
See if it can be run on a diff between the "previous run" and "now". (would require some method for storing the commit hash of the previous run between runs)

@james-bruten-mo : I suspect running on just lines (as the Perl one currently does) would be pretty simple. We'd just need a highly respected Git Guru (@t00sa) to come up with the git command to get "a patchfile" (or old fashioned 'diff' output) from two git commits as that's pretty much what 'fcm bdiff' is producing (presumably those two points being whatever 'git bdiff' has already decided are the parent and the current)

I don't like the lines only approach purely because it misses the 'IMPLICIT NONE' case I was testing - whereas a full file check should catch it. There may be other times the lines only approach might miss required things being removed, I just haven't spotted them yet.
THat said - the patch or diff file should contain the lines removed - we'd just have to make the test smarter to examine them for things that shouldn't have been removed. It might not be easy (my mind is already boggling at possible corner cases) but it's probably doable. Checking all code modules in a file have an implicit none is just easier when looking at the full file...

I think my gut was for dev testing - check changed files
for trunk testing - we need to rethink and come up with a better solution. (such as a huge list of failures we know are there or some way to limit the scope of the check)

I never really dug into what to do with 'linked' tickets and or multiple repos stuff.
The current Perl version relies on the fact that the suite extracts ALL the source code and it checks that, which means no 'change' info - hence the constant reporting of 'historic' breaches of standard. The best it manages at 'dev' testing is it doesn't check code from other repos unless there's a change mentioned in the variables.rc file - and then it does a full source check on that repos.
My initial reacion is when tickets are linked, the branch in each/every repos should be checked with a run of the "style checker" rather than trying to do it from a single repos/compile all the time.

[r-sharp]

Also - @jennyhickson , The way I'm looking at restructuring the code is it could potentially fall into 2 very distinct halves - One to identify what to check - the other to split into threads/processes and do the checking.
The existing Perl version already calls out to shellcheck and perlcheck for those types of files. There's no reason why what I'm looking at couldn't call out to a third party Fortran Linter or something, including being able to build into the new version an exceptions list - this Fortran file, fails these tests, ignore them... Given the state of the code - it might nueter the checker somewhat, but it would cause less distress while all those good intentions come to fruition, especailly if we could fix things on a file by file basis not an error type by error type across the whole repos.

[james-bruten-mo]

That's a good point about testing for "implicit none"'s etc. It's definitely solvable, but probably not worth the extra complexity, especially in getting this ready for git migration.