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

needs_warnings not returning error code or writing to error log #344

Closed
twodrops opened this issue Aug 1, 2021 · 15 comments
Closed

needs_warnings not returning error code or writing to error log #344

twodrops opened this issue Aug 1, 2021 · 15 comments
Assignees
Milestone

Comments

@twodrops
Copy link
Collaborator

twodrops commented Aug 1, 2021

Usage of -W shall handle all warnings as errors and the sphinx-build executable shall return a non-zero value as error code. Also the usage of -w <file> shall write all warnings to a file.

This works incase of Sphinx warnings, but not for warnings created with needs_warnings. Because of this pipelines cannot be configured to break incase of needs_warnings.

To reproduce the problem, do the following steps

  1. Create needs_warnings in conf.py
  2. Create a need warning within any rST
  3. Execute sphinx-build as follows from a console
sphinx-build -M html docs _build -W --keep-going -w error.log  
  1. Observe need_warnings on the console
  2. Execute the following from the console
echo $?
  1. 0 is returned (a non-zero value is expected)
  2. Check the file error.log
  3. Only the following line is present
WARNING: Sphinx-Needs warnings were raised. See console / log output for details.

All needs_warnings are expected in the log.

Could it be that needs_warnings is always writing to stdout instead of stderror (even with -W)?

@danwos danwos added this to the 0.7.2 milestone Aug 5, 2021
@danwos
Copy link
Member

danwos commented Aug 5, 2021

Thanks for the great bug reports. It helps a lot.

To be honest, I thought -W gets handled the correct way, as I use it in some project as well.

It is true, that the output of the single warning checks are producing a normal log.infoon stdout.
But if one or more warnings are detected, a final log.warning is written, with the text:
"Sphinx-Needs warnings were raised. See console / log output for details."
This final log.warning should then stop the build, if -W option is used.

The used logger is coming from sphinx.

Will check, if raising an exception or even a specific sphinx.errors.SphinxWarning will set the exit code more stable.

@twodrops
Copy link
Collaborator Author

twodrops commented Aug 6, 2021

@danwos

You are right. I observed too that the below statement would be present in the logfile
Sphinx-Needs warnings were raised. See console / log output for details.

What would be nice if all need_warnings also appear in the log. This could be a second step, if technical possible.

@danwos danwos assigned danwos and unassigned haiyangToAI Aug 16, 2021
@danwos danwos added this to To do in To-Do Priorities Aug 17, 2021
@danwos
Copy link
Member

danwos commented Aug 17, 2021

--keep-going will always set exit code to 0, even if there are warnings.
This is based on sphinx and we can't do anything here.

For the warning message to error log: We can have a config option needs_warnings_always_warn.
If True, Sphinx-needs will raise a sphinx-warning for each not passed warning check (Default is False).

So the first not passed warning will stop the build if used together with -W.
If used with keep-going, all warnings should get written to error log.

@danwos danwos assigned haiyangToAI and unassigned danwos Aug 17, 2021
@danwos danwos moved this from Prio 3 to Prio 2 in To-Do Priorities Aug 17, 2021
@haiyangToAI haiyangToAI moved this from Prio 2 to In progress in To-Do Priorities Aug 19, 2021
@danwos danwos closed this as completed in ed5c074 Aug 23, 2021
To-Do Priorities automation moved this from In progress to Done Aug 23, 2021
@twodrops
Copy link
Collaborator Author

twodrops commented Sep 6, 2021

@danwos @haiyangToAI Thanks for the analysis on exit code and the feature for writing warnings to error log.

@danwos I wanted to verify again the part about exit code which you mentioned.

--keep-going will always set exit code to 0, even if there are warnings.

The sphinx documentation mentions this however differently (see below or here). Here is it mentioned that an exit status of 1 will be raised. Is this then a Sphinx bug?

-W
Turn warnings into errors. This means that the build stops at the first warning and sphinx-build exits with exit status 1.
--keep-going
With -W option, keep going processing when getting warnings to the end of build, and sphinx-build exits with exit status 1.

@danwos danwos reopened this Sep 6, 2021
To-Do Priorities automation moved this from Done to In progress Sep 6, 2021
@haiyangToAI
Copy link
Contributor

@danwos @twodrops I checked again about exit code when there are warnings.

I tried two tests folder tests/doc_test/doc_needlist and tests/doc_test/doc_needs_warnings with options -W and -W --keep-going:

  1. With Sphinx-build command: sphinx-build -M html . _build -W, the both above mentioned tests doc_needlist and doc_needs_warnings return exit code: 2.

  2. With Sphinx-build command: sphinx-build -M html . _build -W --keep-going, the exit code for doc_needlist is 1, but for doc_needs_warnings exit code is 0.

@twodrops
Copy link
Collaborator Author

twodrops commented Oct 6, 2021

@haiyangToAI I tried to look into tests/doc_test/doc_needlist and tests/doc_test/doc_needs_warnings, but the difference between them was not directly clear. Could you please explain what the difference is, and what leads to exit code of 0 with the second test above.

@haiyangToAI
Copy link
Contributor

@twodrops sorry for the confusion of those two test example.

The difference between the two above is:

  • for doc_needlist, there is only Sphinx warning, no Sphinx-needs warning. When using --keep-going, return code is 1
  • for doc_needs_warnings, there is only Sphinx-needs warning, no Sphinx warning. When using --keep-going, return code is 0

Then I realized, it seems not a valid comparison and a little confusion. I tried to look into again.

Here is the conclusion:

When using --keep-going:

  • if Sphinx warning exists, return code is 1, no matter Sphinx-needs warning exists or not
  • if Sphinx warning does not exist, only Sphinx-needs warning exists, then return code is 0

In a word, return code only depends on Sphinx warning when using --keep-going.

Example of Sphinx warning can be like this: WARNING: html_static_path entry '_static' does not exist

@twodrops
Copy link
Collaborator Author

twodrops commented Oct 7, 2021

Thanks @haiyangToAI for the detailed explanation.

It would be nice to find out why sphinx-needs warnings behaves differently to sphinx-warnings.

  1. Is there anything else to be done while sphinx-needs raises an exception and a related warning, so that the error code is set or
  2. Is it a general sphinx limitation that sphinx extensions cannot set error code for warnings?

The problem is that we cannot break pipelines for sphinx-needs warnings without this behavior.

@twodrops
Copy link
Collaborator Author

@haiyangToAI @danwos How can we conclude this topic? My current understanding from the above discussion is that, sphinx-needs warnings (or in general warnings thrown by sphinx extensions?) are treated differently compared to sphinx warnings. Is this an issue coming sphinx or sphinx-needs?

@twodrops
Copy link
Collaborator Author

@danwos Any clue on this?

@danwos
Copy link
Member

danwos commented Oct 27, 2021

@haiyangToAI figured out the reason:
The status code calculation happens in Sphinx far too early and is based on a warning_count.
See https://github.com/sphinx-doc/sphinx/blob/81a4fd973d4cfcb25d01a7b0be62cdb28f82406d/sphinx/application.py#L345
As our warning-checks happen in the cleanup-phase, which gets executed by Sphinx after the status code calculation, the value for status_code may not be valid anymore.

@haiyangToAI is currently checking if we can make a dirty-hack and change this value on our own.

@twodrops
Copy link
Collaborator Author

twodrops commented Oct 27, 2021

@danwos
I guess you meant the comment here readthedocs/sphinx_rtd_theme#1179 for this issue :)

EDIT: You realized it already :)

@danwos
Copy link
Member

danwos commented Oct 27, 2021

Yeah, already moved and deleted it :)

@twodrops
Copy link
Collaborator Author

@haiyangToAI figured out the reason: The status code calculation happens in Sphinx far to early and is based on a warning_count. As our warning-checks happen in the cleanup-phase, which gets executed by Sphinx after the status code calculation, the value for status_code may not be valid anymore.

Thanks for finding this out.
This could be a feature request for sphinx as well right. It should be possible for sphinx extensions to report warnings in the same way as sphinx does. Sphinx shall then consider this as part of the warning_count

@twodrops
Copy link
Collaborator Author

@danwos @haiyangToAI There seems to be an issue at Sphinx for the behavior I mentioned above
sphinx-doc/sphinx#9142

danwos pushed a commit that referenced this issue Oct 28, 2021
* Fixed sphinx-needs warnings return code

* Added check condition and more test scenarios

Fix for #344
To-Do Priorities automation moved this from In progress to Done Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants