Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Windows compatibility #47

Closed
wants to merge 3 commits into from
Closed

Windows compatibility #47

wants to merge 3 commits into from

Conversation

moveleft
Copy link

@moveleft moveleft commented Mar 8, 2014

Fixes some cross platform issues.

  • When the target of an internal hyperlink is validated, it will fail if the target's path in the pages config does not use the separator native to the platform. Solved by normalizing paths in the config when validating the target.
  • On Windows converting a path to a URL will yield a URL if a trailing backslash, which causes the computation of relative paths to be incorrect. Solved by replacing os.path.sep (instead of os.path.pathsep) with forward slash.

Simon Terkildsen added 2 commits March 8, 2014 02:09
Fix erroneous build error when non native directory separator is used.
c1d872a 
The build would fail if
* a page contains an internal hyperlink
* and, the target page path, in the pages config, uses a different
  directory separator than the one native to the platform
Fix conversion from path to url for Windows.
b1b877f 
On Windows a path such as about/license.md would yield the the url
about/license\ this had an effect on the creation of relative paths
. Resources such as css and js, as well as internal hyperlinks,
would get incorrect urls.
@moveleft moveleft mentioned this pull request Mar 9, 2014
Closed
tomchristie
tomchristie reviewed Mar 10, 2014
View reviewed changes
mkdocs/build.py
@@ -25,7 +25,7 @@ def __call__(self, match):
# If the site navigation has been provided, then validate
# the internal hyperlink, making sure the target actually exists.
target_file = self.nav.file_context.make_absolute(path)
if not target_file in self.nav.source_files:
if not 'target_file' in map(os.path.normpath, self.nav.source_files):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'taget_file' shouldn't be quoted here.

Can you explain the usage of map here - it's not immediately obvious to me what it's for so it probably needs a comment or some other explanation.

@moveleft
Copy link
Author

I don't know why I have put target_file in quotes, but it's definitely never going to work like that.
The map function applies path.normpath to all strings/paths in the self.nav.source_files list. The reason is that target_file is a path in the current platforms native 'format' (e.g. c:\mkdocs\ vs \home\mkdocs), while the paths in source_files are exactly like they were entered in the pages configuration.

I'll remove the quotes and add a comment later today.

@tomchristie
Copy link
Member

Great, thanks @terkill.
Any chance of being able to add a test case that covers this too.
Am mindful that it'd currently be quite easy for me to break windows compat without realising it.

@moveleft
Copy link
Author

I've not been able create any tests which ensures platform compatibility. The only way I can figure is to run the tests on the supported platforms. But I might very well be wrong; my python experience is limited to the few lines I've written here.
But, I have created a test for ´build.PathToURL´ however this test fails on your CI server but not on any of my systems (Windows, Linux and OS X). I can't determine why, do you have any insights, @tomchristie ?

@edbrannin
Copy link
Contributor

Whoops, I didn't notice this pull request before working on Windows support this weekend. :)

See also #97, which does the minimum needed for me to install on Windows -- and includes a HOWTO for a free-for-OSS Windows-based CI service.

@d0ugal
Copy link
Member

d0ugal commented Aug 15, 2014

As per this comment, I'm going to close this for now.

@d0ugal d0ugal closed this Aug 15, 2014
@tomchristie
Copy link
Member

Wait not sure about that one - this seems valid?
Okay for us to ensure we deal with paths in a cross-platform way. Not okay for us to include environment specific setup scripts and suchlike.

@d0ugal
Copy link
Member

d0ugal commented Aug 15, 2014

ah, good point, I miss-read this when scanning the issues. Looks like it needs a rebase tho'

@d0ugal d0ugal reopened this Aug 15, 2014
This was referenced Aug 15, 2014
Closed
Closed
This was referenced Aug 17, 2014
Merged
Closed
@tomchristie
Copy link
Member

Closing due to pull request being out of date. Happy to consider any up to date PRs dealing with this issue.

@tomchristie tomchristie closed this Nov 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moveleft @tomchristie @edbrannin @d0ugal