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

Parameter --base does not work on windows #1454

Open
toadslop opened this issue Jun 20, 2024 · 14 comments
Open

Parameter --base does not work on windows #1454

toadslop opened this issue Jun 20, 2024 · 14 comments
Labels
bug Something isn't working windows

Comments

@toadslop
Copy link
Contributor

Assume I have a website in folder /my/folder/public. My website has many relative urls.

If I cd into /my/folder and run lychee --base ./public ./public. On Linux, this works -- ./public is joined to the working directory and the relative urls and produced correctly and all of the links work where they should.

On Windows, this is the output:

[./public\index.html]:
✗ [ERR] file:///C:/my-svg.svg | Failed: Cannot find file
✗ [ERR] file:///C:/styles.css | Failed: Cannot find file
...

It looks like on Windows Lychee is discarding the working directory instead of prepending it.

I also noticed when I checked the Windows documentation for the file protocal, the output looked a bit different than what Lychee rendered as well. You can check the examples on this page to see what I'm talking about.

@mre
Copy link
Member

mre commented Jun 24, 2024

It looks like on Windows Lychee is discarding the working directory instead of prepending it.

Hum, no idea why. I don't have a Windows machine to test this.

Have you tried absolute paths as described on the page you linked?

lychee --base file:///C:/my/folder/public ./public

Apart from that, I think we need a better path handling crate, because we manually concatenate some of those paths and there are many edge-cases like yours.

@mre mre added bug Something isn't working windows labels Jun 24, 2024
@mre
Copy link
Member

mre commented Jun 24, 2024

Related windows path bug: #972

@toadslop
Copy link
Contributor Author

Hey, thanks for getting back to me. Yeah, I did try with an absolute path and I experienced the same behavior. I attempted with both windows style \ paths and linux style / paths:

lychee --base file:///C:/my/folder/public ./public
lychee --base file:///C:\my\folder\public ./public

My results were still:

[./public\index.html]:
✗ [ERR] file:///C:/my-svg.svg | Failed: Cannot find file
✗ [ERR] file:///C:/styles.css | Failed: Cannot find file
...

@toadslop
Copy link
Contributor Author

Since I have a Windows device I wouldn't mind looking into the behavior myself if you could help point me to the file in the repo where the path handling is defined.

@mre
Copy link
Member

mre commented Jun 25, 2024

Sure, that would be nice. I think the bug is somewhere here:

pub(crate) fn resolve(src: &Path, dst: &Path, base: &Option<Base>) -> Result<Option<PathBuf>> {
let resolved = match dst {
relative if dst.is_relative() => {
// Find `dst` in the parent directory of `src`
let Some(parent) = src.parent() else {
return Err(ErrorKind::InvalidFile(relative.to_path_buf()));
};
parent.join(relative)
}
absolute if dst.is_absolute() => {
// Absolute local links (leading slash) require the `base_url` to
// define the document root. Silently ignore the link in case the
// `base_url` is not defined.
let Some(base) = get_base_dir(base) else {
return Ok(None);
};
let Some(dir) = dirname(&base) else {
return Err(ErrorKind::InvalidBase(
base.display().to_string(),
"The given directory cannot be a base".to_string(),
));
};
join(dir.to_path_buf(), absolute)
}
_ => return Err(ErrorKind::InvalidFile(dst.to_path_buf())),
};
Ok(Some(absolute_path(resolved)))
}

@toadslop toadslop changed the title Paramete --base does not work on windows Parameter --base does not work on windows Jun 26, 2024
@toadslop
Copy link
Contributor Author

Have something to report. Will put in a PR once I finalize, but the bug is caused by the behavior of Path::join. From the Rust documentation:

Creates an owned [PathBuf](vscode-file://vscode-app/c:/Users/bnhei/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-sandbox/workbench/workbench.html) with path adjoined to self.

If path is absolute, it replaces the current path. // <-- THIS

See [PathBuf::push](vscode-file://vscode-app/c:/Users/bnhei/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-sandbox/workbench/workbench.html) for more details on what it means to adjoin a path.

A relative URL in an HTML document starts with '/', which is how an absolute URL in a file system path starts. So when you provide a relative url like "/style.css" in your HTML file, Path::join sees this as an absolute filepath and discards the parent directory.

@toadslop
Copy link
Contributor Author

Seems like a fix for this would be to prepend relative URLs with a period before trying to join them to a file system path.

@toadslop
Copy link
Contributor Author

Identified another bug while investigating this as well. No time to write about it now as I have to go to the office now, but will follow up later with details.

@mre
Copy link
Member

mre commented Jun 27, 2024

If path is absolute, it replaces the current path. // <-- THIS

Right! I fell into this trap a few times by now. It's kind of a sharp edge of the standard library.
That's probably the issue. Thanks for investigating.

Your fix makes sense to me. Thankful for a pull request if you find the time. 😃

@toadslop
Copy link
Contributor Author

Actually, I found a ton of windows bugs when I ran the test suite. Might take me a while to work through them to the point where I can write a proper test for the issue that I found.

@mre
Copy link
Member

mre commented Jun 29, 2024

Ouch. Sorry to hear that and thankful for any support.

@mre
Copy link
Member

mre commented Jun 29, 2024

If it makes the process easier, you could create separate issues for each one. But only if you think it's useful.

@toadslop
Copy link
Contributor Author

toadslop commented Jun 29, 2024

Yeah, probably will. Most of the issues are related to windows filepath handling. For example, the test suites are failing because the tests are assuming that tempdir will return a unix style path, but on windows the path starts with c:/, which gets picked up by reqwest as an invalid scheme.

@mre
Copy link
Member

mre commented Nov 7, 2024

@toadslop, we made a few path-related changes in the latest lychee versions. If possible, can you double-check what the current status is on Windows? A list of open issues would be nice. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working windows
Projects
None yet
Development

No branches or pull requests

2 participants