-
Notifications
You must be signed in to change notification settings - Fork 3
Cleanup, ::error:: usage, input validation, README update #38
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,24 +13,41 @@ inputs: | |
| runs: | ||
| using: "composite" | ||
| steps: | ||
| - name: "Validate input" | ||
| shell: bash | ||
| run: | | ||
| validate_boolean() { | ||
| local input=$1 | ||
| local option_name=$2 | ||
| if ! [[ "$input" =~ ^(true|false)$ ]]; then | ||
| echo "::error::Invalid value for option $option_name. Expected 'true' or 'false', but found '$input'" | ||
| exit 1; | ||
| fi | ||
| } | ||
|
|
||
| validate_boolean "${{ inputs.use-latex }}" use-latex | ||
| validate_boolean "${{ inputs.warnings-as-errors }}" warnings-as-errors | ||
|
|
||
| - name: "Install tth (for old-style documentation)" | ||
| shell: bash | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install --no-install-recommends tth | ||
|
|
||
| - name: "Install TeX Live" | ||
| if: ${{ inputs.use-latex == 'true' }} | ||
| shell: bash | ||
| run: | | ||
| sudo apt-get install --no-install-recommends texlive-latex-base texlive-latex-recommended texlive-latex-extra texlive-fonts-recommended | ||
|
|
||
| - name: "Check for GAP manual" | ||
| shell: bash | ||
| run: | | ||
| cd $GAPROOT/doc/ref | ||
| if [ ! -f manual.six ]; then | ||
| cd $GAPROOT | ||
| make html || : | ||
| # build a HTML version of the GAP reference manual to allow subsequent | ||
| # build an HTML version of the GAP reference manual to allow subsequent | ||
| # steps to pass when building a package manual that contains refs to | ||
| # the GAP reference manual. | ||
| # There is a caveat, though: building the GAP reference manual in turn | ||
|
|
@@ -39,8 +56,11 @@ runs: | |
| # install a full package distribution when running tests. | ||
| # See also <https://github.com/gap-actions/build-pkg-docs/issues/22>. | ||
| fi | ||
|
|
||
| - name: "Compile documentation" | ||
| shell: bash | ||
| env: | ||
| SOURCE_DATE_EPOCH: 0 # prevent time stamps in generated PDF | ||
| run: | | ||
| if [ -f "makedoc.g" ]; then | ||
| $GAP -c 'PushOptions(rec(relativePath:="../../..")); Read("makedoc.g"); QUIT;' 2>&1 | tee $RUNNER_TEMP/output.log | ||
|
|
@@ -55,28 +75,28 @@ runs: | |
| [ -d ../../etc ] && echo "../../etc exists" || ln -s $GAPROOT/etc ../../etc | ||
| cd doc && ./make_doc 2>&1 | tee $RUNNER_TEMP/output.log | ||
| elif [ -f "doc/make_doc" ]; then | ||
| echo "doc/make_doc exists but is not executable!" | ||
| echo "::error::doc/make_doc exists but is not executable!" | ||
| exit 1 | ||
| else | ||
| echo "no makedoc.g file or doc/make_doc script found!" | ||
| echo "::error::no makedoc.g file or doc/make_doc script found!" | ||
| exit 1 | ||
| fi | ||
| env: | ||
| SOURCE_DATE_EPOCH: 0 # prevent time stamps in generated PDF | ||
|
|
||
| - name: "Check for warnings" | ||
| if: ${{ inputs.warnings-as-errors == 'true' }} | ||
| shell: bash | ||
| # The below checks for warnings produced whilst the docs were being built, | ||
| # apart from the warning LaTeX produces when labels may have changed. | ||
| # As discussed in https://github.com/BNasmith/alco/issues/24, the LaTeX | ||
| # label warnings are sometimes false positives. Moreover, GAPDoc can | ||
| # identify this issues, hence we ignore the ones from LaTeX. | ||
| # identify these issues, hence we ignore the ones from LaTeX. | ||
| run: | | ||
| if grep -i -e "warning\b" $RUNNER_TEMP/output.log | grep -qiv "LaTeX Warning: Label(s) may have changed."; then | ||
| echo "Warnings were found when building the documentation!" | ||
| echo "::error::Warnings were found when building the documentation!" | ||
| grep -i -e "warning\b" $RUNNER_TEMP/output.log | ||
| exit 1 | ||
| fi | ||
|
|
||
| - name: "Check documentation is compiled" | ||
| shell: bash | ||
| run: | | ||
|
|
@@ -93,15 +113,18 @@ runs: | |
| for doc_info in doc_infos do | ||
| for filename in filenames do | ||
| if not IsExistingFile( doc_info.(filename) ) then | ||
| Error( | ||
| Exec( | ||
| Concatenation( | ||
| "echo \"::error::", | ||
| "The documentation has supposedly been built, but the file ", | ||
| doc_info.(filename), | ||
| " specified in PackageDoc.", | ||
| filename, | ||
| " does not exist." | ||
| " does not exist.", | ||
| "\"" | ||
| ) | ||
| ); | ||
| FORCE_QUIT_GAP(1); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are the same but FORCE_QUIT_GAP has been around longer and works in more GAP versions so is appropriate for this kind of script
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. So, we should always use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The capitalised versions have been marked obsolete. My thought process was to keep using the CAPS versions on scripts that are likely to be used with "old" GAP versions ( If using the CAPS versions everywhere is preferred, I don't mind changing this. I'm guessing they're not going to be actually removed in the near future anyway?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are in general relatively conservative about removing stuff. And only remove them after no package uses them (if we remove them at all). Right now > 100 packages use So it would be safe to use it everywhere -- but honestly, I also am fine with using a mix, it's not going to cause problems 🤷 |
||
| fi; | ||
| od; | ||
| od; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reson for thix change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is explained in the PR description I think? Or do you mean why this is necessary, why one can't simply insert the "magic string" into the error message? (I suppose one could, possibly after alao inserting a
\nbefore it?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I tried using GAP's
Print(...)before, but in my experience this was rather unreliable. Sometimes it wouldn't print at all (mostly when GAP exited immediately afterwards), sometimes it would concatenate with the previous line, sometimes it would concatenate with the next line, etc.Using
Error("\n::error::This is the line GitHub puts in red.\n")would work, this would show up in the GitHub logs aswith the second and fourth line being in red and shown to the user on the Summary. Using
Exec, we get justwhich is a lot cleaner IMO.