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

[MRG] Better path handling #190

Merged
merged 6 commits into from Jan 17, 2017

Conversation

Projects
None yet
3 participants
@Titan-C
Member

Titan-C commented Dec 21, 2016

This replaces #133
As brought back to life by #186, Sphinx-Gallery does not handle the cases when the documentation structure is in a different place than the location of the makefile and the conf.py files.

This PR aims to fix that and preceding issues stated in #40 #50 #115 #119 #120

For test completeness I know build in circle CI the documentation in various location, with different commands a different themes.

As can be seen from the build logs(https://circleci.com/gh/Titan-C/sphinx-gallery/12?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link). The examples are run only on the first build, later builds can take advantage of the cached files as they are referenced in the same locations and the output to the described location. For review. This are the 3 builds

Since it is possible I have missed cases where links/images/references/etc are not well resolved, the links above point to different places of the documentation I have checked, but please help me explore. And even better tell me if we can elaborate a way to test on this.

There is an issue with circle ci and the cached conda installation that is addressed in #189

seen_backrefs = set()
computation_times = []
# cd to the appropriate directory regardless of sphinx configuration
working_dir = os.getcwd()
os.chdir(app.builder.srcdir)
# os.chdir(app.builder.srcdir)

This comment has been minimized.

@sciunto

sciunto Dec 22, 2016

You probably want to remove this. :)

@sciunto

This comment has been minimized.

sciunto commented Dec 22, 2016

This patch works well for scikit-image: scikit-image/scikit-image#2395

@sciunto

This comment has been minimized.

sciunto commented Jan 3, 2017

@Titan-C Do you plan to release a new version soon? That would be helpful for scikit-image.

PS: just realized that we are in the same building :)

@Titan-C Titan-C changed the title from Better path handling to [MRG] Better path handling Jan 3, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 6, 2017

I was on holiday around Christmas and I am currently going through the backlog. I'll look at this one today.

@lesteve

Looks good in general. I have a few comments though:

@@ -7,6 +7,6 @@ with-doctest=1
doctest-extension=rst
[build_sphinx]
source-dir = docs/
build-dir = docs/_build
source-dir = doc/

This comment has been minimized.

@lesteve

lesteve Jan 6, 2017

Contributor

This was broken before right, which would mean nobody is relying on it?

I would rather remove this section from setup.cfg and use an explicit sphinx-build command in circle.yml.

If there is a good reason to keep it (maybe this is generated by sphinx-quickstart), this should do the same as cd doc; make html in order not to confuse users.

This comment has been minimized.

@Titan-C

Titan-C Jan 12, 2017

Member

Yes, this was always broken as it is the setup to build the docs from setuptools, and setup tools launches the build from where setup.py is called. So this never worked, you can even see that it has the old docs path, because it was never used, and thus never took care of updating.
But now that we can call sphinx-build from any place I thought to make use of this. Because if setuptools can build the docs it can also directly upload them to pythonhosted.org when uploading the package to PyPI. Of course we stopped hosting the docs in pythonhosted long ago in favor of readthedocs.
In circle.yml all 3 statements in the test do the same, is only to test the build from different places a calling from different commands.

This comment has been minimized.

@lesteve

lesteve Jan 16, 2017

Contributor

But now that we can call sphinx-build from any place I thought to make use of this.

I think that is what I have an issue with, python setup.py build_sphinx should build the doc in the same place as cd doc && make html (principle of least surprise). In circle.yml use explicit sphinx-build commands to test out different locations and themes.

This comment has been minimized.

@Titan-C

Titan-C Jan 16, 2017

Member

Ok, least surprise in the doc builds. Travis will do the cd doc && make html to build from the Makefile path. In CircleCI python setup.py build_sphinx will do it from the project root but build in our default location doc/_build/html, additionally `sphinx-build is called for 2 different themes that output in different directories.

----------
figure_list : list of figures absolute paths
sources_dir : str absolute path of Sphinx documentation sources
"""

This comment has been minimized.

@lesteve

lesteve Jan 6, 2017

Contributor

Maybe document the return type via a Returns section since from the current docstring it is not clear that it returns a tuple

failing_examples = set([os.path.normpath(path) for path in
gallery_conf['failing_examples']])
expected_failing_examples = set([os.path.normpath(path) for path in
failing_examples = {os.path.normpath(path): traceback

This comment has been minimized.

@lesteve

lesteve Jan 6, 2017

Contributor

I don't understand this change is it related to this PR?

This comment has been minimized.

@Titan-C

Titan-C Jan 12, 2017

Member

Yes this is entirely related to the PR. The thing is now I pass the absolute path of the files during execution. But at the end when I verify which examples failed I'm matching strings containing the path and those paths must be equal and thus can't have the ../ entries that tend to appear when joining relative paths(Which is how we configure the gallery, relative to conf.py) so the way I found to fix this is to normalize the paths at the end.

This comment has been minimized.

@Titan-C

Titan-C Jan 12, 2017

Member

while debugging my other PR I realized I don't need this.

This comment has been minimized.

@lesteve

lesteve Jan 16, 2017

Contributor

while debugging my other PR I realized I don't need this.

Can you remove this change if it is not needed?

fname_template = os.path.join(gallery_conf['gallery_dir'], 'image{0}.png')
image_rst, fig_num = sg.save_figures(fname_template, 0, gallery_conf)
assert_equal(fig_num, 2)
assert re.search('/image1.png', image_rst)

This comment has been minimized.

@lesteve

lesteve Jan 6, 2017

Contributor

I pushed a change to use in which is simpler for simple string comparisons

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 12, 2017

Comments have been addressed.

@Titan-C Titan-C force-pushed the Titan-C:relpath branch from 6a8ec02 to 395b8fd Jan 12, 2017

Correct relative path handling in gallery references
This allows Sphinx-Gallery to be build by a direct call to
sphinx-build, specifying the sources directory and the build output
directory. There is no more the constrain to launch the build from the
makefile is the sources directory.

- Correct relative path when embedding links
- Correct path during build
- Summary of failing examples to work on relative path builds
- Update tests
- Update docs clean and build configs
- Figure test for mayavi
@@ -86,6 +86,7 @@ def generate_gallery_rst(app):
abort_on_example_error=app.builder.config.abort_on_example_error)
# this assures I can call the config in other places

This comment has been minimized.

@lesteve

lesteve Jan 16, 2017

Contributor

This comment is related to the line app.config.sphinx_gallery_conf = gallery_conf, isn't it?

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 17, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 17, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 17, 2017

I just realized a last minute bug thanks to @chsasank, the zip downloads were using the wrong path. Now it is good.

failing_examples = set([os.path.normpath(path) for path in
gallery_conf['failing_examples']])
expected_failing_examples = set([os.path.normpath(path) for path in
failing_examples = gallery_conf['failing_examples']

This comment has been minimized.

@lesteve

lesteve Jan 17, 2017

Contributor

I don't think you reverted all the unnecessary changes. Keep failing_examples as it was previously.

Keep the \n\nExamples if you wish.

@Titan-C Titan-C force-pushed the Titan-C:relpath branch from 0adab5d to b78f01b Jan 17, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 17, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 17, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 17, 2017

@Titan-C Titan-C force-pushed the Titan-C:relpath branch from 34f787e to b78f01b Jan 17, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 17, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 17, 2017

I fixed the orphan in PR #198

Nice, thanks for doing that in a separate PR!

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 17, 2017

LGTM, I pushed some cosmetic changes. I'll merge this if this is green.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 17, 2017

Green, merging, thanks a lot this is some very nice work to fix in a neat way a long-standing bug!

@lesteve lesteve merged commit 2c9d45b into sphinx-gallery:master Jan 17, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Titan-C Titan-C deleted the Titan-C:relpath branch Jan 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment