[MRG] Better path handling #190

Titan-C · 2016-12-21T16:15:15Z

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

sciunto · 2016-12-22T12:21:12Z

sphinx_gallery/gen_gallery.py

    # cd to the appropriate directory regardless of sphinx configuration
    working_dir = os.getcwd()
-    os.chdir(app.builder.srcdir)
+    # os.chdir(app.builder.srcdir)


You probably want to remove this. :)

sciunto · 2016-12-22T12:21:48Z

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

sciunto · 2017-01-03T09:24:49Z

@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 · 2017-01-03T22:35:45Z

Yes, I do plan to release 0.1.8 soon. I'm putting the milestones now.
I did edited quite a few times this PR last week, but then got no new reviews. It still works. I put new links to the artifacts

@lesteve

lesteve · 2017-01-06T10:12:14Z

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:

lesteve · 2017-01-06T10:17:49Z

setup.cfg

 [build_sphinx]
-source-dir = docs/
-build-dir  = docs/_build
+source-dir = doc/


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.

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.

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.

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.

lesteve · 2017-01-06T10:35:44Z

sphinx_gallery/gen_rst.py

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


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

lesteve · 2017-01-06T10:56:08Z

sphinx_gallery/gen_gallery.py

-    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


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

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.

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

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

Can you remove this change if it is not needed?

lesteve · 2017-01-06T11:05:02Z

sphinx_gallery/tests/test_gen_rst.py

+    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)


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

Titan-C · 2017-01-12T17:59:35Z

Comments have been addressed.

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

lesteve · 2017-01-16T23:08:28Z

sphinx_gallery/gen_gallery.py

    gallery_conf.update(
        abort_on_example_error=app.builder.config.abort_on_example_error)

    # this assures I can call the config in other places


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

Titan-C · 2017-01-17T12:27:17Z

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

Yes, I'll move it

Titan-C · 2017-01-17T12:33:43Z

Is already done, I removed it as soon I realized it was not needed.

Titan-C · 2017-01-17T12:58:21Z

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

lesteve · 2017-01-17T13:26:32Z

sphinx_gallery/gen_gallery.py

-    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']


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 · 2017-01-17T15:31:58Z

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

Changed to disturb the least possible previous code in `summarize_failing_examples`, changed required for the PR are there.

lesteve · 2017-01-17T16:57:17Z

Just noticed the orphan: in https://49-26191147-gh.circle-artifacts.com/0/home/ubuntu/sphinx-gallery/doc/_build/html/auto_examples/index.html.

Is there a good reason why it is here?

Titan-C · 2017-01-17T17:52:23Z

Just noticed the `orphan:` in https://49-26191147-gh.circle-artifacts.com/0/home/ubuntu/sphinx-gallery/doc/_build/html/auto_examples/index.html. Is there a good reason why it is here?

I have noticed it too some time long ago. It was introduced in PR #187, and I have fixed in older PRs to be merged #160 and #151. But just forgot to fix it here too, it was not really the aim of the PR.

Titan-C · 2017-01-17T18:25:07Z

I fixed the `orphan` in PR #198

lesteve · 2017-01-17T18:29:02Z

I fixed the orphan in PR #198

Nice, thanks for doing that in a separate PR!

lesteve · 2017-01-17T19:15:34Z

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

lesteve · 2017-01-17T19:27:34Z

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

Titan-C mentioned this pull request Dec 21, 2016

[sphinx 1.5] _gdbm.error No such file or directory #186

Closed

sciunto reviewed Dec 22, 2016

View reviewed changes

soupault mentioned this pull request Dec 22, 2016

Fix deprecated option in sphinx that causes warning treated as error in travis scikit-image/scikit-image#2395

Merged

Titan-C force-pushed the relpath branch 5 times, most recently from c8b6561 to 3c7a476 Compare December 23, 2016 16:01

sciunto mentioned this pull request Dec 31, 2016

Update sphinx-gallery before release 0.13 scikit-image/scikit-image#2410

Closed

Titan-C added this to the Release v0.1.8 milestone Jan 3, 2017

Titan-C mentioned this pull request Jan 3, 2017

[NO MRG - WIP] Improved use of relative paths #133

Closed

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

Titan-C mentioned this pull request Jan 3, 2017

[MRG] Large codebase refactoring #160

Closed

lesteve reviewed Jan 6, 2017

View reviewed changes

Titan-C force-pushed the relpath branch from 6a8ec02 to 395b8fd Compare January 12, 2017 19:57

Titan-C mentioned this pull request Jan 16, 2017

[MRG + 2] [MAINT] Update to Sphinx-Gallery 0.1.7 scikit-learn/scikit-learn#7986

Merged

Normalize building paths

34d3473

lesteve reviewed Jan 16, 2017

View reviewed changes

Group gallery conf instructions

8fc6989

Fix bug in zip downloads paths

d46e91e

lesteve reviewed Jan 17, 2017

View reviewed changes

Titan-C force-pushed the relpath branch from 0adab5d to b78f01b Compare January 17, 2017 15:29

Simplify the sumarize_failing_examples

b78f01b

Titan-C force-pushed the relpath branch from 34f787e to b78f01b Compare January 17, 2017 18:16

Tweak CHANGES.rst and remove unneeded export

6e5f3ae

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

Titan-C mentioned this pull request Jan 17, 2017

Relative paths of example directories not relative to doc root #50

Closed

Titan-C deleted the relpath branch January 22, 2017 19:37

Titan-C mentioned this pull request Aug 2, 2018

[MRG] Separate rst notebook generation from execution of the script #239

Merged

[MRG] Better path handling #190

[MRG] Better path handling #190

Uh oh!

Conversation

Titan-C commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sciunto commented Dec 22, 2016

Uh oh!

sciunto commented Jan 3, 2017

Uh oh!

Titan-C commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Jan 6, 2017

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-C commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-C commented Jan 17, 2017 via email

Uh oh!

Titan-C commented Jan 17, 2017 via email

Uh oh!

Titan-C commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-C commented Jan 17, 2017 via email

Uh oh!

lesteve commented Jan 17, 2017

Uh oh!

Titan-C commented Jan 17, 2017 via email

Uh oh!

Titan-C commented Jan 17, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Jan 17, 2017

Uh oh!

lesteve commented Jan 17, 2017

Uh oh!

lesteve commented Jan 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Titan-C commented Dec 21, 2016 •

edited

Loading

Titan-C commented Jan 3, 2017 •

edited

Loading

lesteve Jan 16, 2017 •

edited

Loading

Titan-C commented Jan 17, 2017 via email •

edited

Loading