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.

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

base_url = "/" is broken on Windows #2076

Closed
orlp opened this issue Jan 17, 2023 · 9 comments
Closed

base_url = "/" is broken on Windows #2076

orlp opened this issue Jan 17, 2023 · 9 comments
Labels
Need Windows help We need someone using Windows to help us with this issue

Comments

@orlp
Copy link
Contributor

orlp commented Jan 17, 2023

Bug Report

Using / as your base_url is broken on Windows, it works on macOS.

Environment

Windows 10

Zola version: 0.16.1

Expected Behavior

I expect / to be used as the base url, such that for example get_url(path="assets/style.css") resolves to /assets/style.css.
This is useful for being domain / ip address agnostic (and in fact probably should be the default for zola serve as opposed to hardcoding 127.0.0.1).

Current Behavior

It works as I expect on macOS. On Windows I get the following error when running zola build:

Error: Failed to build the site
Error: Failed to render section '\\?\C:\programming\website\content\_index.md'
Error: Reason: Failed to render 'index.html'
Error: Reason: Function call 'get_url' failed
Error: Reason: `get_url`: Could not parse link `/assets/style.css?h=d8a209c008fa6ee25321785ef32e698b4db897a99cad80f358eba62f9dc2aff6` as a valid URL

Step to reproduce

Set / as your base_url and attempt to use the get_url builtin on Windows.

@orlp
Copy link
Contributor Author

orlp commented Jan 17, 2023

The problematic section of code is here:

if cfg!(target_os = "windows") {

@Keats Keats added the Need Windows help We need someone using Windows to help us with this issue label Jan 17, 2023
@Keats
Copy link
Collaborator

Keats commented Jan 17, 2023

I'll need someone on Windows to write a failing test and fix it as I don't have a machine available (that and #1626)

@dennisse
Copy link

dennisse commented Jan 23, 2023

Using / as your base_url to get relative links will break any (atom/rss) feeds. Perhaps there should be an option to use relative links? Or maybe add an option to the get_url-function, so it can return relative links?

It is possible to use a filter as a temporary mitigation, but it isn't very pretty.

get_url(path="assets/style.css") | replace(from=config.base_url, to="/")

Something like get_url(path="assets/style.css", rel=true) would be nice.

@orlp
Copy link
Contributor Author

orlp commented Jan 27, 2023

Using / as your base_url to get relative links will break any (atom/rss) feeds.

The solution is to have a separate base_url specifically for the feeds, and not be forced to use absolute URLs literally everywhere else. For now I just hard embedded the feeds template and updated it like such:

<link href="https://example.com{{ feed_url | safe }}" rel="self" type="application/atom+xml"/>

@orlp
Copy link
Contributor Author

orlp commented Jan 27, 2023

I'll need someone on Windows to write a failing test and fix it as I don't have a machine available (that and #1626)

I think the more immediate issue is to remove the problematic check. People using backslashes as paths silently failing is suboptimal, but failing to build perfectly valid setups is much worse IMO.

@Keats
Copy link
Collaborator

Keats commented Jan 27, 2023

I think the more immediate issue is to remove the problematic check.

Well it was added to fix a bug so just removing it is not acceptable either.

@orlp
Copy link
Contributor Author

orlp commented Jan 27, 2023

Well it was added to fix a bug so just removing it is not acceptable either.

I disagree. The bug in question is #1632. The whole origin of this bug is that the user expects links in the Markdown such as foo\bar\baz.png to work, when this is a Windows-ism Zola should not embrace. Zola should be platform-agnostic, and users should write their asset paths in a platform-agnostic way, using forward slashes. This way their builds work everywhere (including Windows).

The test asset path in global_fns::files::tests::can_resolve_asset_path_to_valid_url is created by directly passing the OS path to the JSON encoder and not normalizing it first to use slashes. The test case should be updated, and the original 'fix' should never have been merged.

@Keats
Copy link
Collaborator

Keats commented Jan 27, 2023

That's fair enough, I didn't look back at the issue, just at the git blame talking about a bug.

@orlp
Copy link
Contributor Author

orlp commented Jan 27, 2023

Actually, looking at it, the whole test case can be removed, since its whole purpose was to test whether directly passing OS paths into the GetUrl function works (which you shouldn't do in the first place). I'll make a pull request removing the test case and erroneous Windows-only code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Windows help We need someone using Windows to help us with this issue
Projects
None yet
Development

No branches or pull requests

3 participants