I think this is going to move some files around in the final build. Do we expect that is going to cause trouble for anyone?

this is going to move some files around in the final build

Yes. Moreover, the paths will be unpredictable (they are generated by hashing file contents). I've already expressed this concern in#24005. However, at least the docs built for this PR look fine (I don't see any obvious problems).

If somebody has a deeper understanding of Sphinx API, maybe an alternative solution can be found. Keep in mind, although, that it must:

1. Work correctly forsinglehtml anddirhtml builders.
2. Work correctly for building multiple targets.
3. Avoid copying unneeded files.

@MikhailRyazanov Thanks for looking into this. Unfortunately, we might lack sphinx expertise to properly understand and solve this issue. - I have to admit, that the download-approach feels a bit hackish, but I don't have a better suggestion. So it might be that we'll have trouble coming up with a decision here.

If somebody has a deeper understanding of Sphinx API, maybe an alternative solution can be found. Keep in mind, although, that it must:

1. Work correctly forsinglehtml anddirhtml builders.
2. Work correctly for building multiple targets.
3. Avoid copying unneeded files.

While this all is reasonable would be nice, we do not necessarily need a 100% solution. Maybe that makes solving some parts easier.

We could decide thatsinglehtml is not supported by the directive. And/or that multiple targets are not supported and instead must be build with separate calls.

@timhoffm, IMHO hackish is how this extension currently circumvents Sphinx mechanisms by copying files directly to the output tree (at a wrongbuild phase) and trying to generate links to them (based on incorrect assumptions).

I would say that:download: isthe correct approach at least for the script files — to compare with, this is what the Sphinx-Gallery extension does (check “Examples” in the Matplotlib docs). And, conceptually, for PDF plots as well (although modern browsers can display PDF files, these are mainly intended for downloading, since SVG is a more appropriate alternative for viewing in a browser).

Basically, what do you think is wrong with using:download: for additional files? Except that some other extensions might depend on the files written byplot_directive (this is reallyhackish, since such behavior is not documented anywhere), although we don't know that any such third-party extensions exist...

Regardingsinglehtml, not supporting it would be a very strange and unjustified decision, especially since Read the Docs uses it for buildingdownloadable documentation (called “Zipped HTML” there).

We have been doing it his way from at least 2011, so this is not a something new we have started doing, and it is unclear to me if there was a better way to do this in sphinx at time. To Tim's point, it is very unclear if using:download: is actually less hackish or just differently hackish nor is it clear to me that there is a "right" phase to do this sort of thing.

That sphinx-gallery uses:download: is a point in its favor, however it was also my understanding that all of sphinx-gallery is "hackish" to work around sphinx limitations.

I agree with Tim those three things are "nice to have".

It is also not clear to be that read the docs should be usingsinglehtml for this purpose. While I 'm sure it make sense for many projects, if you build our docs that way you get (leaving broken links aside) a 77MB html file which I could not get a browser to even show me from disk...

This also makes a superficial change of adding download icons
which I think is a slight improvement over just text.

I am not worried about further extensions relying on files being there, but on people who are relying on urls that have been stable for 10+ years.

Despite all of this, I am weakly in favor of taking this PR. It reduces the amount of code we have and will let is push future issues with this back upstream andsinglehtml is likely useful to many of our users.

@tacaswell, thanks for approving this!

To Tim's point, it is very unclear if using:download: is actually less hackish or just differently hackish

As I wrote, it at least lets Sphinx itself to handle all the file management and thus avoid all the mentioned problems (when unneeded files are copied, needed files are not copied, and wrong links are generated). The only “hackish” thing about it is that formally:download: is intended for files from thesource tree rather than from thebuild directory, but in reality in accepts any paths, so this should not be a problem.

nor is it clear to me that there is a "right" phase to do this sort of thing.

According toSphinx documentation, there is “Phase 1: Reading”, when the doctree is prepared and cached on the disk (to<build>/plot_directive in our case), and then a separate “Phase 4: Writing”, when all the actual output files are written (plot directive currently ignores this phase and thus could not copy the files when building multiple targets). So the proper way would be either to insert references to the desired files into the doctree and letSphinx to copy them or, if this can't be done, to add a hook that will copy the necessary files at the writing phase. In either case, this will need better understanding of Sphinx than apparently anybody here has and will result in writing more code than simply using:download:.

This also makes a superficial change of adding download icons
which I think is a slight improvement over just text.

The visual appearance is entirely controlled by the selected Sphinx theme (through CSS). Matplotlib docs usempl-sphinx-theme, inherited frompydata-sphinx-theme, whichadds these icons. The default Sphinx theme has no icon and calmer link decorations, andsphinx_rtd_themehas a simpler icon but otherwise no special link formatting. Those who wish the old appearance, in which links to raw files are not distinct from links to normal web pages, can also achieve that easily by a simple custom CSS...

@story645 thanks for pinging. However, I don’t feel competent to review this topic and I don’t have the time to dig into this.

Looks like this needs a rebase now.

OK, “rebased” and updated the tests introduced in PR#24215.

No idea why the AppVeyor test is shown as failed, with cryptic messages ”Canceling since a higher priority waiting request for 'Tests-24205-' exists” / “More recent PR#24205 build has been started.” Should it be restarted manually?

Thanks@MikhailRyazanov! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

Thanks@QuLogic! Will it be released in 3.7.0 or 3.6.3? (And when this can be expected?)