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

Add option follow_links controling the docs_dir walker to follow directories links #1134

Merged
merged 5 commits into from Oct 4, 2017

Conversation

Projects
None yet
4 participants
@pivaldi
Contributor

pivaldi commented Feb 2, 2017

Hello,

In our company (OVYA), all projects have their own documentation and we want to assemble then in a general documentation via "git sub modules".
In each documentation we set docs_dir=content and, in the general documentation, we place all the sub modules in the directory docs.
In creating a symlink (unix system) from docs/PROJECT_NAME/content to content/projects/PROJECT_NAME we will expect to have all the documentation in one place.
Unfortunately, Mkdocs does not follows the symlinks that we created…

This pull request add a new option that allows to follow directories links.

@@ -181,7 +181,6 @@ def run_validation(self, value):
raise ValidationError(
"The URL isn't valid, it should include the http:// (scheme)")

This comment has been minimized.

@waylan

waylan Feb 2, 2017

Member

This is causing a flake 8 failure.

@@ -259,15 +258,14 @@ def post_validation(self, config, key_name):
"can mean the source files are overwritten by the output or "
"it will be deleted if --clean is passed to mkdocs build."
"(site_dir: '{0}', docs_dir: '{1}')"
).format(config['site_dir'], config['docs_dir']))
).format(config['site_dir'], config['docs_dir']))

This comment has been minimized.

@waylan

waylan Feb 2, 2017

Member

This is also causing a flake8 failure.

elif (config['site_dir'] + os.sep).startswith(config['docs_dir'] + os.sep):
raise ValidationError(
("The 'site_dir' should not be within the 'docs_dir' as this "
"leads to the build directory being copied into itself and "
"duplicate nested files in the 'site_dir'."
"(site_dir: '{0}', docs_dir: '{1}')"
).format(config['site_dir'], config['docs_dir']))
).format(config['site_dir'], config['docs_dir']))

This comment has been minimized.

@waylan

waylan Feb 2, 2017

Member

Both of these line changes are causing flake8 failures.

@waylan

This comment has been minimized.

Member

waylan commented Feb 2, 2017

In addition to the flake8 failures, I have two concerns:

  1. This adds an additional setting. Generally we try to reduce, not add settings. Is this setting necessary? Or is this perhaps something that can be handled with a plugin (see #206)?

  2. Following links is a system specific thing. We have always been careful to offer complete support for all systems. I expect this feature will fail for Windows. That means that the same project would have different behavior on Windows than on a *nix (Linux/Mac OS) system, which is unacceptable. If you commit a MkDocs project to Git on Mac OS and I check it out on Windows, I should get the same results from a build as you. I realize a third party plugin could work inconsistently, but I'm uncomfortable with the core acting that way.

@pivaldi

This comment has been minimized.

Contributor

pivaldi commented Feb 2, 2017

  1. The additional setting is to keep backward compatibility

  2. The change concerns essentially os.walk(dir) to os.walk(dir, followlinks=False/True) which is as OS agnostic than Python is.
    To my mind "followlinks=True" should be the default in the Mkdocs but can break compatibility. Don't see any reason to not follow links in a such software.

@facelessuser

This comment has been minimized.

Contributor

facelessuser commented Feb 2, 2017

Following links is a system specific thing.

Is it? I use symlinks on Windows (not in my documentation, but in general). Does followlinks work with Window's symlinks? I haven't done Python scripting that needs to be symlink aware on Windows, just curious.

@waylan

This comment has been minimized.

Member

waylan commented Feb 2, 2017

@pivaldi I apologize. It appears I wasn't clear about what I was looking for with my first response.

The additional setting is to keep backward compatibility

Yes, I recognize that. My point is that in the past when we had to add a setting to preserve backward compatibility when adding a new feature, then we chose to not add the feature in most cases. How does this feature overcome that? Is it importation enough (to the majority of users) to justify adding a setting? Is there a way to do it without adding a setting (perhaps via a plugin). If so, then that would be preferable to adding a setting. I'm looking for arguments to convince me that we should break our "rule" against adding settings in this case. How does the need for this feature outweigh the preference to not add settings?

The change concerns essentially os.walk(dir) to os.walk(dir, followlinks=False/True) which is as OS agnostic than Python is.

That is exactly my point. In the past when Python has had differing behavior between systems, then we didn't use the Python Standard Library to accomplish that task. Instead we offered an alternative which behaves the same on all systems. Or, if it has not been reasonable to build and maintain such a solution ourselves, then we have not added the feature at all. How is this different?

For that matter, @facelessuser questions whether it actually is different. Maybe not. I don't know. I assumed is was different but I didn't check. However, if it is not different, then confirmation of that would go a long way to accepting this proposal.

@waylan

This comment has been minimized.

Member

waylan commented Feb 2, 2017

By the way, if the feature is important enough, then it may make sense to just enable the feature by default and issue a "info" message when a link is followed (to warn users--a warning seems too strong). Then we can avoid the setting altogether. We are pre-version 1.0 after all. Some arguments for that may be considered as well.

@waylan

This comment has been minimized.

Member

waylan commented Feb 2, 2017

So I felt like this may have been discussed before and did a search. I found #869 which proposed adding followlinks=True to os.walk in copy_media_files. That proposal was accepted and implemented in #948. It would be weird to have different behavior for the pages. That being the case, I think the same should happen here. The "followlink" behavior should be the default with no setting to change the behavior.

If you update the PR to remove the setting and make followlinks=True the default (and fix the one flake8 error which still exists), I'll take another look at this.

I suppose there is still a possibility it would be rejected for differing behavior between systems, but then we would remove the same from copy_media_files and that seems less likely now that it has been there for as least one release cycle with no complaints.

- Make followlinks the default for dir walker
- Remove follow_links option
- Fix flake8 report
@waylan

waylan approved these changes Feb 3, 2017

@warmfusion

This comment has been minimized.

warmfusion commented Jul 14, 2017

We bumped up against this same issue today when I tried to do the same as @pivaldi - Not using submodules but a similar setup with a common 'hub' documentation with symlinks into projects in other cloned paths on the server.

I've patched in the change for testing using docker container running python:3.4

Heres my test container (which is pretty messy)

FROM python:3.4

EXPOSE 8000
RUN pip install mkdocs mkdocs-material  pygments

COPY patches /patches

RUN patch --verbose -p1 /usr/local/lib/python3.4/site-packages/mkdocs/config/config_options.py < /patches/follow_symlinks-1134.diff

VOLUME /site
WORKDIR /site

CMD /usr/local/bin/mkdocs serve -a 0.0.0.0:8000

Observed Errors

I did find an error where a symlink that points to a missing directory would cause a Error to be thrown and the site to be broken.

Jul 14 18:11:05 docker01 docker[12316]: [E 170714 17:11:05 ioloop:638] Exception in callback <bound method type.poll_tasks of <class 'livereload.handlers.LiveReloadHandler'>>
Jul 14 18:11:05 docker01 docker[12316]: Traceback (most recent call last):
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/tornado/ioloop.py", line 1026, in _run
Jul 14 18:11:05 docker01 docker[12316]: return self.callback()
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/livereload/handlers.py", line 67, in poll_tasks
Jul 14 18:11:05 docker01 docker[12316]: filepath, delay = cls.watcher.examine()
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/livereload/watcher.py", line 73, in examine
Jul 14 18:11:05 docker01 docker[12316]: func and func()
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/mkdocs/commands/serve.py", line 106, in builder
Jul 14 18:11:05 docker01 docker[12316]: build(config, live_server=live_server, dirty=dirty)
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/mkdocs/commands/build.py", line 377, in build
Jul 14 18:11:05 docker01 docker[12316]: utils.copy_media_files(config['docs_dir'], config['site_dir'], dirty=dirty)
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/mkdocs/utils/__init__.py", line 175, in copy_media_files
Jul 14 18:11:05 docker01 docker[12316]: copy_file(source_path, output_path)
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/site-packages/mkdocs/utils/__init__.py", line 110, in copy_file
Jul 14 18:11:05 docker01 docker[12316]: shutil.copy(source_path, output_path)
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/shutil.py", line 229, in copy
Jul 14 18:11:05 docker01 docker[12316]: copyfile(src, dst, follow_symlinks=follow_symlinks)
Jul 14 18:11:05 docker01 docker[12316]: File "/usr/local/lib/python3.4/shutil.py", line 108, in copyfile
Jul 14 18:11:05 docker01 docker[12316]: with open(src, 'rb') as fsrc:
Jul 14 18:11:05 docker01 docker[12316]: FileNotFoundError: [Errno 2] No such file or directory: '/opt/docs/dev-docs/docs/fte'
@waylan

This comment has been minimized.

Member

waylan commented Jul 14, 2017

Thanks for the feedback @warmfusion. Unfortunately, this PR is out-of-date. And today it should be merged to the 1.0.dev branch which would make it even more out-of-date. In any event, I've added it to the 1.0 milestone so we won't forget about it.

Also, I should note that #1250 discusses some Windows specific solutions which may address some of the concerns raised above. Anyone working on a solution should probably combine the two into a single PR.

@waylan waylan added Update needed and removed Update needed labels Oct 3, 2017

@waylan waylan merged commit 5bdd4a6 into mkdocs:master Oct 4, 2017

3 checks passed

codecov/project 94.01% (target 90%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

waylan added a commit that referenced this pull request Oct 4, 2017

Revert "Add option follow_links controling the docs_dir walker to fol…
…low directories links (#1134)"

This reverts commit 5bdd4a6.
@warmfusion

This comment has been minimized.

warmfusion commented Dec 19, 2017

So... why was this reverted?

@waylan

This comment has been minimized.

Member

waylan commented Dec 19, 2017

I don't recall why and it appears I didn't document why either. However, I can see that it was reverted immediately after being committed, so I can only assume I committed it in error and then undid my mistake.

As noted in a previous comment, at the time this should have been merged to the 1.0.dev branch. I'm guessing I missed that that concern hadn't been addressed unit after I accepted the PR, which is my I reverted it. However, as of today, the 1.0.dev branch has been merged into master, so it would be appropriate to merge into master today.

Another concern I see it that there are no tests for this change. We really should have some tests for this.

As I can't reopen a "merged" PR, feel free to open a new PR with the above changes and some tests and I'll accept it.

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