Add Windows support to preview with minimal changes & CI support

[tomaarsen]

Closes #306
Closes #298
Closes #307 (supersedes it)

Hello!

Pull Request overview

• Allow doc-builder preview on Windows without drastic implementation changes.
• Add Windows to the CI test suite.

Scope

This PR is intended to add support for doc-builder preview for Windows. Other commands, such as building with --html are out of scope, as I don't believe that these commands are as useful. However, this support can also be added if requested in the future.

Details

My experimentation has shown me that there were at least three main issues that prevented doc-builder from working on Windows. This PR tackles these three without negatively affecting other OSes.

Issue 1

Windows users of doc-builder preview will be greeted with this Exception:

FileNotFoundError: [WinError 2] The system cannot find the file specified

This is caused by these subprocess.run commands:

doc-builder/src/doc_builder/commands/preview.py

Lines 132 to 147 in f398867

	subprocess.run(
	["npm", "ci"],
	stdout=subprocess.PIPE,
	check=True,
	encoding="utf-8",
	cwd=working_dir,
	)
	# start sveltekit in dev mode
	subprocess.run(
	["npm", "run", "dev"],
	check=True,
	encoding="utf-8",
	cwd=working_dir,
	env=env,
	)

On windows, shell=True must be set (False is the default) to run a command in this manner.

The fix

I have set this behaviour using platform.system() == "Windows", causing these changes to only affect Windows users. See the platform.system() docs here.

Issue 2

The _toctree.yml file will commonly contain files in directories, e.g. api/main. However, on Windows, doc-builder finds those files as api\\main. Exceptions are thrown due to this mismatch in check_doc_integrity. A workaround is to use api\\main in the _toctree.yml, but that will then only work for Windows users, which is even worse than the current behaviour.

The fix

The fix is elementary, we already rely on Path in this check_doc_integrity, so all we have to do is normalize both the toctree paths and the locally-discovered paths using str(path) with path as a Path instance. No need for OS-dependent conditionals here.

Issue 3

This issue originates from improper URL modification in the TypeScript get endpoint, causing endless exceptions trying to load a path starting with C:\C:\ when the browser is started up:

This issue originates here:

doc-builder/kit/src/routes/endpoints/toc.ts

Line 26 in f398867

const filepath = path.join(import.meta.url.replace('file://', ''), '../..', '_toctree.yml');

In this line, the file:// is replaced in a non-recommended manner. Please have a look at the this comment and the FileURLToPath documentation, which shows that improper use of slicing or replacing can have unintended consequences for cross-platform URLs.

The fix

The cross-platform fix is uses the recommended FileURLToPath function, which works for all platforms.

Tests

For convenience, I have added Windows to the CI test runner. Although I can imagine that the test suite doesn't cover all cases, I imagine the Windows support in the CI would be useful.

Closed issues

As mentioned, this PR closes some issues. I'd like to briefly discuss each:

• [Windows] hardcoded '/' in preview #306. This is not a bug. tmp_dir in the example is a Path instance, which is responsible for correctly handling paths through the __truediv__ method.
• Windows compatibility issues #298. The first item in thislist is indeed an issue. The second item is somewhat right: the preview indeed raises an error, but it is not caused by a forward slash for working_dir in start_sveltekit_dev.
• Handle windows paths with back slashes #307. This PR makes unnecessarily many changes I'm afraid. It also mixes os.path.join with Path instances, which is not necessary. The true problem with this PR is that it adds shell=True at all times, modifying behaviour for Unix users. My PR only makes changes for Windows users.

---

Let me know if you need anything else from me at this point!

• Tom Aarsen

[coyotte508]

Thanks for the fixes! 🤗

Tagging @mishig25 or @sgugger to check the python changes

[sgugger]

Thanks for working on this!

[tomaarsen]

Gladly! It'll come in handy for building the SetFit docs later :)