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

Build: fail PDF command (latexmk) if exit code != 0 #10113

Merged
merged 5 commits into from
Mar 7, 2023

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 6, 2023

We were forcing exit_code=0 if we detected that Output written on (.*?) was found in the standard output of the latexmk command. This was producing confusions to our users because the PDF generation failed but we were considering it successful.

Now, we are relying on the exit code itself as we do for all the other commands making sure the command succeeded before proceding with the build. Now, if the PDF command fails, we fail the build completely.

Closes #10015

We were forcing `exit_code=0` if we detected that `Output written on (.*?)` was
found in the standard output of the `latexmk` command. This was producing
confusions to our users because the PDF generation failed but we were
considering it successful.

Now, we are relying on the exit code itself as we do for all the other commands
making sure the command succeeded before proceding with the build. Now, if the PDF
command fails, we fail the build completely.
@humitos humitos requested a review from a team as a code owner March 6, 2023 19:21
@humitos humitos requested a review from stsewd March 6, 2023 19:21
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

I'm +1 with this change. @ericholscher may want to review this as well.

@stsewd stsewd requested a review from ericholscher March 6, 2023 19:30
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This is an obvious and good thing to do, but I do think we'll hear from users on this. It would be really wonderful if we could message this in the build failure smartly, so they know what happened.

readthedocs/doc_builder/backends/sphinx.py Show resolved Hide resolved
readthedocs/doc_builder/backends/sphinx.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Mar 7, 2023

It would be really wonderful if we could message this in the build failure smartly, so they know what happened.

I wrote a simple and quick proposal for the message. I'm not happy with it tho 😅 . Please, suggest a better option if you have something in mind. This is how this page will look like:

Screenshot_2023-03-07_09-48-15

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. I was thinking it wouldn't be too difficult, and I think the UX is much better. I really like this pattern!

readthedocs/doc_builder/exceptions.py Outdated Show resolved Hide resolved
Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@humitos humitos enabled auto-merge (squash) March 7, 2023 14:34
@humitos humitos merged commit 92a7182 into main Mar 7, 2023
@humitos humitos deleted the humitos/build-fail-pdf-cmd branch March 7, 2023 18:26
ericholscher added a commit that referenced this pull request Mar 16, 2023
ericholscher added a commit that referenced this pull request Mar 16, 2023
ericholscher added a commit that referenced this pull request Mar 16, 2023
ericholscher added a commit that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants