Fix latex issues via value added scripts

[michael-okeefe]

Pull request overview

• Fixes Misc. LaTeX Errors Not Caught in Normal Workflow #8026

Purpose and Approach

Fixes Misc. LaTeX Errors Not Caught in Normal Workflow by adding a python-based LaTeX log parsing capability in Python that summarizes the relevant issues and warnings in the LaTeX log files by file and line number and also adds a JSON summary of the specific issues that can be further processed and/or displayed by the CI. This pull request also then fixes the majority of these issues across all the docs. These issues include package-based issues and warnings (for e.g., issues reported by \siunits) as well as issues and warnings from both the TeX and LaTeX processing pipeline.

The only new code added is the doc/tools/parse_latex_log.py file. That file uses no external dependencies beyond the python standard library. Fixes to the docs were found and fixed using the errors and warnings reported by the LaTeX log parser. The hope is that this script will make LaTeX errors and warnings more visible and actionable for future EnergyPlus documentation authors.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If feature, test running new feature, try creative ways to break it
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[Myoldmopar]

This looks great! CI caught the warnings and parsed them out nicely. However, we need to remove the other warnings that were already present before the CI improvements. If you can clean those up, I think we'll be good to go.

[Myoldmopar]

And to be clear, there are a couple options:

• Make a commit that cleans out just the extra LATEXWARNING:: stuff that is showing up, but leaves the LaTeX "mistakes" in place so that we demonstrate perfect CI behavior. Then follow it up with a commit that gets the LaTeX code looking all clean. Or...
• Just do both of those at once and get a clean result.

I don't care either way. Once both of those steps are done, however, mark this as ready for review and we'll get it in.

Thanks @nealkruis @michael-okeefe

[nealkruis]

We can either pipe stdout into a txt file, or have an option to silence the python script output.

If I'm understanding things correctly, that would mean that we would see the three tests fail, but there wouldn't be any output below them. This would be followed by the new failure messages from parsing the *_errors.json files.

We could change it so that executing the python script is just another custom target or custom command in CMake instead of a CTest. That way you wouldn't see the extra failed tests.

Either way, once we get that sorted out, we'll clean up the actual LaTeX warnings/errors so this can go in cleanly.

[Myoldmopar]

We could change it so that executing the python script is just another custom target or custom command in CMake instead of a CTest. That way you wouldn't see the extra failed tests.

Yep, this would be ideal. Please make it so, but leave the LaTeX warnings in place, and we'll make sure it all looks good, then do one clean commit and merge.

Thanks!

[nealkruis]

Results are in! It definitely prevents other tests from being run successfully if python sends a bad return code.

@michael-okeefe let's set it up to always return zero so it doesn't prevent other tests from running.

@Myoldmopar since these aren't really Test Results, would it make sense to have the CI put them somewhere else (e.g., in "Build Messages" or in a separate section just for documentation)?

[michael-okeefe]

I just modified the script so that it never returns a non-zero exit code and pushed that up (it was already on-par with the latest from develop). Shall I also start cleaning up the remaining LaTeX issues that were identified?

[Myoldmopar]

This looks fantastic. Once you address those LaTeX issues and we get a clean build we'll merge this in. Thanks for the refinements here, @nealkruis @michael-okeefe !

[Myoldmopar]

Oh, and about your comment on the build results/test results. Unfortunately, since this is run as a post-processing ctest, I think it's too late for CI to grab these as "build results". But they do show up fine as parsed test results.

[michael-okeefe]

This looks fantastic. Once you address those LaTeX issues and we get a clean build we'll merge this in. Thanks for the refinements here, @nealkruis @michael-okeefe !

Sounds great. I just started on addressing the last of the LaTeX issues. Thanks!

[Myoldmopar]

This looks pretty great as-is, though a couple thoughts came to mind that could be considered:

• Do we want to make DOCS_TESTING just an internal variable, nothing exposed to the cache?
• If the doc test is run as a post build step, I would be happy to tell CI to search for the errors.json files after the build rather than after the test.

[Myoldmopar]

Sometime in the future we need to move away from this PythonInterp as it was deprecated in 2018 in favor of find_package(Python3), but first we need to move our minimum cmake up.

[Myoldmopar]

OK, so as soon as you enable both BUILD_DOCS and BUILD_TESTING, this will be forced to ON. No problem. Since this is automatically triggered to ON/OFF, I wonder if we should either mark it advanced, or just take it out of the cache entirely and treat it like a local cmake variable.

[Myoldmopar]

It would be cool to just reuse the previously found Python, but I know that may be an annoying thing to manage, so it's OK.

[Myoldmopar]

OK, so if this is just a POST_BUILD on the PDF building portion, then the file should be available once "make" is complete. In this case, I think I actually could have CI find the error files early enough to catch these as build errors, not test failures. Let me know if I am correct in the build step and I can make an alteration to CI.

[Myoldmopar]

Is this list really there just for testing? Or is this something we have to maintain?

[michael-okeefe]

Yeah, I was debating about this and ended up leaving it in as I found it useful but you are right -- it could become a maintenance issue if anyone else were to depend on it. On a second look, I'd vote to just remove it.

[Myoldmopar]

👍

[michael-okeefe]

I've fixed this and the remaining LaTeX issues that were flagged. Just pushed that up.

I also added a check in the python script to delete the target JSON file if it exists before writing as a persistent JSON error file would be confusing for those working off of their local laptops and actively fixing issues.

[Myoldmopar]

This looks great! I'm not sure why Mac hasn't reported yet, but Mac wasn't involved, so I'm OK with not waiting on Mac for this. The only comment I had unresolved was whether we should just eliminate the DOCS_TESTING cache variable since it is controlled via the two cache variables BUILD_DOCS and BUILD_TESTING. This doesn't have to be held up for that, but I'd like to know what you think of that change at least.

[nealkruis]

We'll still need DOCS_TESTING for the documentation CMake project (if it's created independently). I'll make it an internal variable in the EnergyPlus CMake project.

[Myoldmopar]

I don't see how this could've broken that one failing Windows file. I'm feeling OK with just letting this merge as-is. I've got a branch of Decent CI that moves the "test errors" into "build errors" which is much more appropriate here. I won't worry about merging that immediately but I'll try some time soon.