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.

Sign up for GitHub

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

Jump to bottom

Run documentation tests on GitHub Actions and fix submodule error #8321

Merged
merged 10 commits into from
Sep 30, 2020
Merged

Run documentation tests on GitHub Actions and fix submodule error #8321

merged 10 commits into from
Sep 30, 2020

Conversation

nealkruis
Copy link
Member

Pull request overview

This PR:

  1. Enables documentation testing (LaTeX error log parsing) and configures the output from the parsing process to align with what is detectable using GitHub Actions problem matchers so that warnings/errors can be posted as annotations.
  2. Ignores third party submodules that were accidentally included and causing git related errors in the post-checkout step in GitHub Actions.

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
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions

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)

@nealkruis nealkruis added Defect Includes code to repair a defect in EnergyPlus DoNotPublish Includes changes that shouldn't be reported in the changelog NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Sep 29, 2020
@nealkruis nealkruis added this to the EnergyPlus Future milestone Sep 29, 2020
@nealkruis nealkruis self-assigned this Sep 29, 2020
@nealkruis
Copy link
Member Author

nealkruis commented Sep 29, 2020 •
edited
Loading

@Myoldmopar I put an intentional warning in the documentation to illustrate this is working in the Big Ladder fork.

It's now cleaned up and ready for review/merge.

@Myoldmopar
Copy link
Member

This looks really good, however the annotation appears to want to link out to the problem file, but isn't doing it. Is there any chance that can be improved so that the annotation link actually points back to the correct source file? Or maybe I'm missing something about the annotation.

@nealkruis
Copy link
Member Author

I believe this is an issue with the way annotations work: actions/runner#566

Hopefully GitHub fixes this up soon, because it would be a nice feature.

Myoldmopar
Myoldmopar approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good here, this is a nice start for making use of annotations.

.github/workflows/doc-problem-match.json
]
}
]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool. It would be nice to adapt the pattern matching that we use in Decent CI into our other builds.

.github/workflows/documentation.yml
run: cmake -DTEX_INTERACTION=batchmode -DDOCS_TESTING=ON ..

- name: Add problem matcher
run: echo "::add-matcher::.github/workflows/doc-problem-match.json"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

doc/tools/parse_latex_log.py
@@ -168,40 +168,6 @@ def find_multiply_defined_labels(root_dir, label):
target = "\\label{" + label + "}"
return find_locations(root_dir, target)


def parse_warning(line, src_dir):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this should be gone.

doc/tools/parse_latex_log.py
warns = []
m1 = LABEL_MULTIPLY_DEFINED_WARN.match(self._current_issue)
m2 = HYPER_UNDEFINED_WARN.match(self._current_issue)
if m1 is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And so presumably this is where we would match other LaTeX warnings if we wanted.

third_party/.gitignore
btwxt/vendor/googletest
btwxt/.gitmodules
penumbra/vendor/googletest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Myoldmopar
Copy link
Member

Thanks @nealkruis, merging this.

For those developers out there who review doc changes, you will now see some LaTeX build issues pop up as Github Action Annotations, like on this set of build results: https://github.com/bigladder/EnergyPlus/actions/runs/279081457

@Myoldmopar Myoldmopar merged commit 75fcd72 into NREL:develop Sep 30, 2020
@nealkruis nealkruis deleted the fix-github-actions branch November 2, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees

@nealkruis nealkruis

Labels
Defect Includes code to repair a defect in EnergyPlus DoNotPublish Includes changes that shouldn't be reported in the changelog NotIDDChange Code does not impact IDD (can be merged after IO freeze)
Projects
None yet
Milestone
EnergyPlus 9.5.0
Development

Successfully merging this pull request may close these issues.

GitHub Actions don't perform documentation tests and show submodule error
5 participants
@nealkruis @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3