Create unique directories, not unique filenames in _downloads

[TormodLandet]

We have a similar problem as bug #823 with files such as CMakeLists.txt and Makefile. The reader will get confused when we tell them to download a demo file along with a CMakeLists.txt file and then they get CMakeLists5.txt which is not recognized by the cmake program, so they must remember to rename the file before running the demo.

We do not require as large a change as #823 (which is why I created a new issue), only that the file base names remain unchanged and unique prefix directories are created. A quick and ugly fix is stick this at the end of conf.py, but hotpatching internal Sphinx APIs is not very maintainable in the long run ...

# Hack to support avoid renaming download files with same names
# coming from different directories (CMakeLists.txt, Makefile etc)
from sphinx.util import FilenameUniqDict
from os import path
def add_file(self, docname, newfile):
    if newfile in self:
        self[newfile][0].add(docname)
        return self[newfile][1]
    uniquename = path.basename(newfile)
    i = 0
    while uniquename in self._existing:
        i += 1
        uniquename = path.join('subdir%s' % i, path.basename(newfile))
    self[newfile] = (set([docname]), uniquename)
    self._existing.add(uniquename)
    return uniquename
FilenameUniqDict.add_file = add_file

# Hack to support sub-directories within the download directory
from sphinx.builders.html import StandaloneHTMLBuilder, ensuredir, copyfile
from docutils.utils import relative_path
from sphinx.util.console import brown
def copy_download_files(self):
    def to_relpath(f):
        return relative_path(self.srcdir, f)
    # copy downloadable files
    if self.env.dlfiles:
        ensuredir(path.join(self.outdir, '_downloads'))
        for src in self.status_iterator(self.env.dlfiles,  # self.app.status_iterator in master ...
                                        'copying downloadable files... ',
                                        brown, len(self.env.dlfiles),
                                        stringify_func=to_relpath):
            dest = self.env.dlfiles[src][1]
            subdirs, filename = os.path.split(dest)
            if subdirs:
                ensuredir(path.join(self.outdir, '_downloads', subdirs))
            try:
                copyfile(path.join(self.srcdir, src),
                         path.join(self.outdir, '_downloads', dest))
            except Exception as err:
                self.warn('cannot copy downloadable file %r: %s' %
                          (path.join(self.srcdir, src), err))
StandaloneHTMLBuilder.copy_download_files = copy_download_files

This works-for-me! Would something similar be appreciated as a patch? I guess more builders would need to be changed?

[shibukawa]

I think only HTMLBuilder needs this hack. epub/qthelp don't allow download role. Maybe applehelp/htmlhelp don't support too.

[TormodLandet]

This is still a problem, but I am seeing some strange interaction with images.

An image which is linked to several times will have the second link show up as "_images/subdir1/image.png", but the image is located in "_images/image.png" and the first ".. image::" directive shows the correct link. These are obviously the same image, so they should not be treated as different with duplicate names. I will take a look to see if I can figure out what is going on.

If I fix this, would a pull request be appreciated? Having this large hack in conf.py for an extended amount of time is not great

[TormodLandet]

The following fixed this, but it probably only works on stable, not master due to self.app.status_iterator vs status_iterator

# Hack to support sub-directories within the _images directory
def copy_image_files(self):
    # type: () -> None
    # copy image files
    if self.images:
        ensuredir(path.join(self.outdir, self.imagedir))
        for src in self.app.status_iterator(self.images, 'copying images... ',
                                            brown, len(self.images)):
            dest = self.images[src]
            subdirs, filename = os.path.split(dest)
            if subdirs:
                ensuredir(path.join(self.outdir, self.imagedir, subdirs))
            try:
                copyfile(path.join(self.srcdir, src),
                         path.join(self.outdir, self.imagedir, dest))
            except Exception as err:
                logger.warning('cannot copy image file %r: %s', path.join(self.srcdir, src), err)
StandaloneHTMLBuilder.copy_image_files = copy_image_files

Do you want a pull request?

[shibukawa]

Thank you. Yes, we want to PR. But for master branch.

Your code affects current behavior. So it should be in 1.6. A potential issue is a link URL for the file would be changed. If any other websites link to the file directly, it becomes 404.

So could you rewrite your code based on master branch and put a comment to "Incompatible changes" section in CHANGES file?

[TormodLandet]

Pull request created. Now, lets see what travis thinks about it ...

[TormodLandet]

Is there anything else I should do about this now, or will it be included in the next release? I believe I have answered/fixed all concerns
@shibukawa @tk0miya

[tk0miya]

Fixed by #5377.
Thank you for reporting!