[BUG] Added a returncode for make all-pdf #556
Conversation
|
Shouldn't the circle job be erroring? |
|
@choldgraf yes, this is strange. The build itself looks like its failing, but the ci job is passing. weird |
jupyter_book/commands/__init__.py
Outdated
| @@ -172,7 +172,7 @@ def build(path_book, path_output, config, toc, warningiserror, builder): | |||
| with cd(OUTPUT_PATH): | |||
| output = subprocess.run([makecmd, "all-pdf"]) | |||
| if output.returncode != 0: | |||
| return output.returncode | |||
| exit(output.returncode) | |||
There was a problem hiding this comment.
I think this should be return and we need to check if circle is acting on status codes. Does the return get returned by jupyter_book?
There was a problem hiding this comment.
Yes, it did get returned by jupyter_book in my local system. But, did not seem to work in circleci. I mean the build did stop in circleci but did not exit with an error code.
There was a problem hiding this comment.
why not just check the output code yourself in the python and raise an _error( depending on its value?
There was a problem hiding this comment.
ooh ok. so you are basically saying to use _error instead of exit ?
|
Will create an issue for the failing test in tests suite after this PR is ready to be merged. |
|
just a note that my assumption is that all of these tests should pass except for the circleci pdf jobs |
|
gotcha. I will ask @najuzilu, if she can help out with |
|
Interesting... Looks like when the |
|
Scratch that; it's |
|
oh - I just looked at the test and now I'm confused as to how @najuzilu's PDFlatex tests were ever passing in the first place haha. I think the problem is that we are running a |
|
Thanks a lot @najuzilu and @choldgraf . @choldgraf this looks ready to be merged now. |
|
nice! now we only need to fix the pdf builder 😆 |
@choldgraf I will look into the |
check for the return code and return the error code when
mak all-pdferrorsfixes #551
This will make the build fail, when pdflatex build errors.
cc: @choldgraf @mmcky
@choldgraf , any other requirements from that issue?