Issue #5352 - generate_static_pages cannot deal with directories being turned into regular pages

cms/bin/generate_static_pages.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
 import re
 import errno
 import codecs
 import ConfigParser
 import logging
 from argparse import ArgumentParser
+import shutil
 
 from cms.utils import get_page_params, process_page
 from cms.sources import create_source
 
 MIN_TRANSLATED = 0.3
 
 
 def generate_pages(repo, output_dir):
     known_files = set()
 
     def write_file(path_parts, contents, binary=False):
         encoding = None if binary else 'utf-8'
         outfile = os.path.join(output_dir, *path_parts)
         if outfile in known_files:
             logging.warning('File %s has multiple sources', outfile)
             return
         known_files.add(outfile)
 
-        if os.path.exists(outfile):
+        if os.path.isfile(outfile):

[Vasily Kuznetsov, 2018/09/24 11:58:47]

Are you sure if the two branches of this if statement exhaust all the
possibilities? What if it's some other weird object like a named pipe? We should
probably do something sensible there, like raise an error right away.

[Tudor Avram, 2018/09/24 14:04:51]

Also, one more thing: do we want to raise an error if the old version of the
website has some kind of a weird object or just remove it altogether (and assume
the new version is correct and should replace it)?

[Vasily Kuznetsov, 2018/09/24 14:42:23]

I think if there's some kind of weird object in there, we're probably dealing
with some unexpected situation and it's probably safer to stop. It seems that
this scenario is very unlikely anyway so it's probably fine.

[Tudor Avram, 2018/09/24 16:10:00]

Done.

             with codecs.open(outfile, 'rb', encoding=encoding) as handle:
                 if handle.read() == contents:
                     return
+        elif os.path.isdir(outfile):
+            shutil.rmtree(outfile)
 
         try:

[Vasily Kuznetsov, 2018/09/24 11:58:47]

I think it would be good to extract this whole block into a separate function.
Otherwise it's a bit too much logic altogether. This could also take care of
repeated `os.path.dirname(outfile)`.

We can also do a quick check: `if dir != output_dir:` and call the directory
creation only when it's actually needed.

After the code is in a separate method, we might also notice that the loop in
line 55 is doing more or less the same thing as `os.makedirs()` and maybe we can
just use `os.mkdir()` since we're already doing the iteration anyway. I leave it
up to you to decide which option makes more sense.

[Tudor Avram, 2018/09/24 13:26:45]

Yeah, I think it makes sense to put this into a different function. 

As your suggestion with replacing os.makedirs() comes, I chose to do it only if
the path already exists, because: 
1) It would rarely be the case for `os.path.isfile(path_so_far)` to be true and
then it os.makedirs() would only be called the second time if we would have a
file in the previous version of the website that now would become a directory.
For example, having `en/foo` as a file in the initial version and then the new
website being `en/foo/bar/index`,  would be a case where that loop actually
changes anything.
2) Yeah, I can replace `os.makedirs()` with multiple `os.mkdir`, but I just
assumed that os.makedirs() does something more clever in the backend (making it
more efficient) than my rough for loop.

[Vasily Kuznetsov, 2018/09/24 14:42:23]

I don't expect many directories to be created when any website is generated and
in creating directories the slowest part is the filesystem, so I don't think
`os.makedirs()` can be much faster than your loop.

I'm also not against using `os.makedirs()` as long as the code is simple -- I
was just thinking that it might become simpler and more straightforward with
`os.mkdir()`. 

[Tudor Avram, 2018/09/24 16:10:00]

Done.

             os.makedirs(os.path.dirname(outfile))
         except OSError as e:
             if e.errno != errno.EEXIST:
                 raise
+            path_so_far = output_dir
+            for part in path_parts[:-1]:
+                path_so_far = os.path.join(path_so_far, part)
+                if os.path.isfile(path_so_far):
+                    os.remove(path_so_far)
+                    os.makedirs(os.path.dirname(outfile))
+                    break
 
         with codecs.open(outfile, 'wb', encoding=encoding) as handle:
             handle.write(contents)
 
     with create_source(repo, cached=True) as source:
         config = source.read_config()
         defaultlocale = config.get('general', 'defaultlocale')
         locales = list(source.list_locales())
         if defaultlocale not in locales:
             locales.append(defaultlocale)
@@ skipping 61 matching lines @@
                     os.rmdir(path)
     remove_unknown(output_dir)
 
 
 if __name__ == '__main__':
     parser = ArgumentParser('Convert website source to static website')
     parser.add_argument('source', help="Path to website's repository")
     parser.add_argument('output', help='Path to desired output directory')
     args = parser.parse_args()
     generate_pages(args.source, args.output)