1458461 - Use pipenv to manage environment for mach doc command

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Documentation, enhancement)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Splitting this out from bug 1437593 as my patch is causing an intermittent failure, and we don't want to block on vendoring pipenv.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Bug 1458461 - Use pipenv for |mach doc| environment;

Review commit: https://reviewboard.mozilla.org/r/241208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241208/

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hunt [:davehunt] ⌚️UTC+0 from comment #1)
> Created attachment 8972667 [details]
> Bug 1458461 - Use pipenv for |mach doc| environment;
> 
> Review commit: https://reviewboard.mozilla.org/r/241208/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/241208/

This patch was previously reviewed as part of bug 1437593 and was backed out due to an intermittent generating the docs. I've triggered a try run, which demonstrates the failure here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3c1362d940fd36b002474cb38f1525a9e8336a

I believe we can ignore most of the errors output by the task as these also exist without this patch. The fatal issue appears to be "Resource temporarily unavailable". I haven't been able to replicate this locally, but we can probably investigate by adding debug or increasing the verbosity of Sphinx in a try run.

Comment 3

[(not currently active) Ted Mielczarek]

I did a little digging this morning, but I don't have any ideas yet.

sphinx is getting `EAGAIN` writing a traceback to stderr (the fact that it's writing a traceback seems OK, the log is full of them):
https://github.com/sphinx-doc/sphinx/blob/645044d7f03c7fcb4c39a8edf40aa66f6d6c2b64/sphinx/cmdline.py#L104

stderr is actually a codec writer because mach's `Mach.run` method reopens `sys.stderr` with `codecs.getwriter` so it will properly encode Unicode:
https://dxr.mozilla.org/mozilla-central/rev/f877359308b17e691209e1afb7193b8e86f175ce/python/mach/mach/main.py#350

The traceback shows that the error is coming when writing to the file that the codec writer wraps, though, which is actually stderr:
[task 2018-05-02T19:01:05.346Z]   File "/usr/lib/python2.7/codecs.py", line 370, in write
[task 2018-05-02T19:01:05.346Z]     self.stream.write(data)

Calling `write` on a `file` object in Python winds up just calling `fwrite` on the underlying `FILE*`:
https://github.com/python/cpython/blob/6d3d02c69a65abcd64eb6d83baac5675f9208318/Objects/fileobject.c#L1871

`mach doc` in this task is invoked by way of `run-task`, which pipes the process output using `subprocess.Popen`:
https://dxr.mozilla.org/mozilla-central/rev/f877359308b17e691209e1afb7193b8e86f175ce/taskcluster/docker/recipes/run-task#101

I checked, and that means that Python simply calls `os.pipe` to create the file descriptor:
https://github.com/python/cpython/blob/6d3d02c69a65abcd64eb6d83baac5675f9208318/Lib/subprocess.py#L865

So fundamentally the mach process is erroring because `fwrite` is returning `EAGAIN` trying to write to the stdout pipe. The only way this should happen is if the pipe is set to non-blocking mode. I don't see anywhere in our code that that would be happening.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8972667 [details]
Bug 1458461 - Use pipenv for |mach doc| environment;

https://reviewboard.mozilla.org/r/241208/#review247242

This is fine once we sort out the failure.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8972667 [details]
Bug 1458461 - Use pipenv for |mach doc| environment;

https://reviewboard.mozilla.org/r/241208/#review254386

Sorry for the random drive-by review. I forgot this bug already existed and implemented almost this exact same patch before I realized. This will break doc uploading though, so giving an r- for now.

::: tools/docs/Pipfile:7
(Diff revision 1)
> +backports-abc = "*"
> +futures = "*"
> +livereload = "*"
> +recommonmark = "*"
> +singledispatch = "*"
> +sphinx = "<1.7.0"
> +sphinx_rtd_theme = "*"
> +sphinx-js = "*"
> +typing = "*"

This is going to break the DocUp task (I only know because I ran into the same problem). This is because when it tries to install boto3 with `install_pip_package` (which is apparently incompatible with the version of pip that pipenv uses):
https://searchfox.org/mozilla-central/source/tools/docs/mach_commands.py#179

The `futures` and `typing` packages are dependencies of `boto3`, so they can be removed from here.

::: tools/docs/Pipfile:7
(Diff revision 1)
> +url = "https://pypi.python.org/simple"
> +verify_ssl = true
> +name = "pypi"
> +
> +[packages]
> +backports-abc = "*"

When using \* for the specifiers, will it still be possible to update only a single package? Or would it be an all or nothing proposition?

Say I wanted to *only* update `sphinx-rtd-theme` in the future, how could I do that?

Comment 6

[Andrew Halberstadt [:ahal]]

Btw, here's my change on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3eb677e43e5c77f579bc8c54e562434d79327cde&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=181109418
 
The only difference is that I pinned the packages in Pipfile.. but using * might be better if there is a way to do what I'm asking in the second issue. The good news is that the intermittent Doc issue seems to have magically fixed itself!

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8972667 [details]
Bug 1458461 - Use pipenv for |mach doc| environment;

https://reviewboard.mozilla.org/r/241208/#review254386

> When using \* for the specifiers, will it still be possible to update only a single package? Or would it be an all or nothing proposition?
> 
> Say I wanted to *only* update `sphinx-rtd-theme` in the future, how could I do that?

You should be able to run `pipenv update --outdated` to find out what updates are available, and `pipenv update <pkg>` to update a single package along with any transient dependencies. This will require you to have pipenv installed, unless we introduce a mach command to use pipenv in the initial virtualenv. Something like `mach pipenv /path/to/Pipfile [arg, ...]` where the args just get passed directly to pipenv.

Comment 8

[Andrew Halberstadt [:ahal]]

Thanks, `pipenv update` is what I was missing (I was using `pipenv lock` before). In that case yes, let's not pin the dependencies in the Pipfile.

I think a |mach pipenv| could be useful, though you could also just run |mach python -mpipenv update ...|. Since I have a patch that works with the DocUp task already, I'll take this bug over.

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1458461 - [docs] Use pipenv to manage |mach doc| python environment,

Review commit: https://reviewboard.mozilla.org/r/248212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248212/

Comment 10

[Andrew Halberstadt [:ahal]]

I decided to pin the packages because I think package updates should happen in their own bugs. There's already a bug on file to update sphinx (bug 1437526). Maybe we can update all the packages at the same time in that bug.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8982265 [details]
Bug 1458461 - [docs] Use pipenv to manage |mach doc| python environment,

https://reviewboard.mozilla.org/r/248212/#review254470

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4830f72e8297
[docs] Use pipenv to manage |mach doc| python environment, r=davehunt

Comment 13

[Andrew Halberstadt [:ahal]]

Fyi looks like the intermittent happens when I upgrade all of the doc dependencies to their latest versions:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=510eb6ec7ca72918830aae1492ae62f419463f5b

So we'll still likely need to address it at some point.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4830f72e8297