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

DOC: get docs to build on Windows #8502

Merged
merged 10 commits into from Nov 12, 2020

Conversation

kalenkovich
Copy link
Contributor

Reference issue

Fixes #8501

What does this implement/fix?

There are a few problems preventing docs from building on Windows. This PR is intended to fix them. As of now, it solves three out of four problems described in #8501.

The code used to combine url parts using `os.path.join` which on Windows adds '\' instead of '/' leading to an invalid url.
Unlike `/tmp`, `tempfile.gettempdir()` should work on any OS
It depends on which shell one is using.
The problem arises because conda-based graphviz installation only exposes `dot.bat` and git-bash, for example, does not know what to do with it.
@larsoner
Copy link
Member

Actually it looks like the fourth problem can be solved here:

https://github.com/mne-tools/mne-python/blob/master/mne/viz/_brain/_scraper.py#L82

We probably do not need the leading / there.

And can you add a [circle front] to your next commit message to make CircleCI build the front-page examples? One of those has an animation and it would be good to make sure it still works. Alternatively, you can make some change to https://github.com/mne-tools/mne-python/blob/master/tutorials/source-modeling/plot_mne_dspm_source_localization.py. (CircleCI by default only builds changed examples, so you either need to change that one to get it to render, or add [circle front] to tell it to build all front-page ones.)

WARNING: dot command 'dot' cannot be run (needed for graphviz output), check the graphviz_dot setting

you might have to install `graphviz`_ manually.
Make sure to agree to add graphviz to path during installation.
Copy link
Member

Choose a reason for hiding this comment

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

Or it looks like you could also do:

export PATH=~/Miniconda3/Library/bin/graphviz:$PATH

or the equivalent for your system/env. Not sure if this is better, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, adding graphviz to path is a much saner solution than a separate install! I'll fix it.

I'll assume that graphviz has been installed into the conda environment and make use of the CONDA_PREFIX variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed by 2bc392e

kalenkovich and others added 2 commits November 10, 2020 20:35
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@larsoner
Copy link
Member

@kalenkovich did you try the fix above for brain-scraped examples? You should be able to check by making the change then running:

PATTERN=mne_dspm make html_dev-pattern

It should only run the mne_dspm_solutions example in tutorials/source-modeling, then you can check the RST output.

It's best to be on the latest master version of sphinx-gallery because this one will ensure the example is run and RST file written even if the example was stale

@kalenkovich
Copy link
Contributor Author

@kalenkovich did you try the fix above for brain-scraped examples? You should be able to check by making the change then running:

PATTERN=mne_dspm make html_dev-pattern

It should only run the mne_dspm_solutions example in tutorials/source-modeling, then you can check the RST output.

It's best to be on the latest master version of sphinx-gallery because this one will ensure the example is run and RST file written even if the example was stale

Not yet :(
Not sure I'll be able to get to it today, sorry about that.

I think removing the forward slash will make unix-based systems to treat the path as relative. So, it will work on Windows but break on other systems. Adding [circle front] will test it for us, right?

@larsoner
Copy link
Member

I think removing the forward slash will make unix-based systems to treat the path as relative.

Only if the original path is relative. Based on what you see on Windows it might already be absolute. And [circle front] will indeed test things for us

@larsoner
Copy link
Member

Okay I looked at the sphinx-gallery source code:

https://github.com/sphinx-gallery/sphinx-gallery/blob/master/sphinx_gallery/scrapers.py#L372-L374

Will push a fix that fixes things on Windows for me

@larsoner
Copy link
Member

@kalenkovich feel free to add an entry to latest.inc, otherwise LGTM +1 for merge

@kalenkovich
Copy link
Contributor Author

I added a line to latest.inc and fixed the code that adds graphviz to the path so that it actually works in git-bash.

Docs now build without errors on my pc. I'll mark this PR as ready for review then.

@kalenkovich kalenkovich marked this pull request as ready for review November 11, 2020 20:29
@kalenkovich kalenkovich changed the title WIP, DOC: get docs to build on Windows DOC: get docs to build on Windows Nov 11, 2020
doc/changes/latest.inc Outdated Show resolved Hide resolved
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@larsoner
Copy link
Member

Travis is unrelated, thanks @kalenkovich !

@larsoner larsoner merged commit 3e1b581 into mne-tools:master Nov 12, 2020
3 of 5 checks passed
@kalenkovich kalenkovich deleted the ek_docs-on-windows-2 branch November 12, 2020 12:27
@agramfort
Copy link
Member

thx @kalenkovich !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

building docs on Windows: a few remaining problems
4 participants