working_copy: implement symlinks on Windows with a helper function #2939

gulbanana · 2024-02-04T09:28:32Z

Fixes #2

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

gulbanana · 2024-02-04T09:41:20Z

Alright, we've got some test failures on windows-latest. They pass locally, but presumably the runner is not in developer mode and doesn't run as admin - Windows only supports symlinks if one of those things is true! I'll look into modifying the tests, which were previously #[cfg(unix)], so that they look for either success or a special "this Windows instance does not support this feature" error.

In order to achieve that I'll probably make the symlink function the same on both windows and unix, instead of just having the same name. That way we can get better error messages at runtime when this failure occurs on unconfigured-for-symlinks machines.

gulbanana · 2024-02-04T10:18:10Z

The exact requirements for symlinks appear to be:

winver 14972 or higher
Developer Mode on
pass flag SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE (std does this)

gulbanana · 2024-02-04T12:03:19Z

The test changes were more invasive than I hoped and so I've split them into an extra commit. This has some nasty nested error handling, and probably needs a restructure, but I don't know what patterns we prefer for this sort of thing - feedback appreciated!

lib/src/file_util.rs

ilyagr · 2024-02-04T18:25:45Z

Optionally, it would be nice to update docs/windows.md in this commit. This could also be done in a separate PR.

You could say:

Symbolic link support

jj supports symlinks on Windows under the following conditions:

winver 14972 or higher (Windows 10+)

"Developer Mode" is on

If those conditions are not satisfied, jj will (for now) crash when the working copy contains symlinks.

It might also be worth adding that, for colocated repos to work, the git option must be set. (Again, this can go in separately if it's a chore)

lib/src/file_util.rs

lib/tests/test_local_working_copy.rs

lib/src/local_working_copy.rs

gulbanana · 2024-02-07T13:11:03Z

Optionally, it would be nice to update docs/windows.md in this commit. This could also be done in a separate PR.

You could say:

Symbolic link support

jj supports symlinks on Windows under the following conditions:

winver 14972 or higher (Windows 10+)

"Developer Mode" is on

If those conditions are not satisfied, jj will (for now) crash when the working copy contains symlinks.

It might also be worth adding that, for colocated repos to work, the git option must be set. (Again, this can go in separately if it's a chore)

I've updated those docs, and addressed (I think!) the requested changes.

docs/windows.md

lib/src/file_util.rs

lib/src/local_working_copy.rs

lib/src/file_util.rs

The main goal is to avoid having a symlink in our source tree. Currently, there is no good way to work with the `jj` repo with `jj` on Windows. Currently `jj` just crashes with symlinks. This is being worked on, see e.g. martinvonz#2939, but it will always depend on whether Developer Mode is enabled in Windows or whether symlinks are materialized as text files with symlinks. Finally, MkDocs has trouble following symlinks on Windows, so building docs wouldn't work there. Another advantage is that, previously, we were lucky that MkDocs treats `insta` header in `cli-reference@.md.snap` as a Markdown header and follows symlinks at all. Now, we no longer depend on that.

The main goal is to avoid having a symlink in our source tree. Currently, there is no good way to work with the `jj` repo with `jj` on Windows. Currently `jj` just crashes with symlinks. This is being worked on, see e.g. #2939, but it will always depend on whether Developer Mode is enabled in Windows or whether symlinks are materialized as text files with symlinks. Finally, MkDocs has trouble following symlinks on Windows, so building docs wouldn't work there. Another advantage is that, previously, we were lucky that MkDocs treats `insta` header in `cli-reference@.md.snap` as a Markdown header and follows symlinks at all. Now, we no longer depend on that.

gulbanana · 2024-02-14T14:55:27Z

I've modified the previous approach based on Martin's patch to create regular files on unsupported platforms. The supportness check goes from a cfg to a flag stored in the TreeState - always true on unix, true on windows if Developer Mode is enabled; turns out we can check that by looking in the registry. The symlink tests are now enabled on all platforms.

I've also addressed the other feedback, with the possible exception of the concern in #2939 (comment) - @yuja may want to weigh in.

gulbanana · 2024-02-16T05:22:45Z

I think that's everything for now.

Cargo.lock

lib/src/file_util.rs

lib/src/local_working_copy.rs

CHANGELOG.md

docs/windows.md

lib/src/local_working_copy.rs

lib/src/file_util.rs

gulbanana · 2024-02-25T07:09:34Z

Addressed feedback and rebased.

CHANGELOG.md

lib/src/file_util.rs

CHANGELOG.md

lib/src/file_util.rs

yuja

lgtm, thanks.

… variable

enables symlink tests on windows, ignoring failures due to disabled developer mode, and updates windows.md

gulbanana force-pushed the windows-symlinks branch 2 times, most recently from bbc712b to bbd9254 Compare February 4, 2024 11:59

gulbanana force-pushed the windows-symlinks branch from bbd9254 to 57c8fcb Compare February 4, 2024 12:04

yuja reviewed Feb 4, 2024

View reviewed changes

lib/src/file_util.rs Show resolved Hide resolved

martinvonz reviewed Feb 4, 2024

View reviewed changes

lib/src/file_util.rs Outdated Show resolved Hide resolved

lib/tests/test_local_working_copy.rs Show resolved Hide resolved

lib/tests/test_local_working_copy.rs Outdated Show resolved Hide resolved

lib/src/local_working_copy.rs Show resolved Hide resolved

gulbanana force-pushed the windows-symlinks branch 3 times, most recently from 1509ee3 to 7f06842 Compare February 7, 2024 13:08

ilyagr reviewed Feb 7, 2024

View reviewed changes

docs/windows.md Outdated Show resolved Hide resolved

yuja reviewed Feb 8, 2024

View reviewed changes

lib/src/file_util.rs Show resolved Hide resolved

lib/src/file_util.rs Outdated Show resolved Hide resolved

lib/src/file_util.rs Outdated Show resolved Hide resolved

lib/src/local_working_copy.rs Outdated Show resolved Hide resolved

lib/src/file_util.rs Show resolved Hide resolved

ilyagr mentioned this pull request Feb 9, 2024

Clean up the generated CLI reference and remove the need fo a symlink using the include-markdown plugin #2986

Merged

gulbanana force-pushed the windows-symlinks branch 2 times, most recently from b18047d to 1dfff8a Compare February 14, 2024 14:55

gulbanana force-pushed the windows-symlinks branch 2 times, most recently from b3668be to 55e87b2 Compare February 15, 2024 14:24

martinvonz reviewed Feb 17, 2024

View reviewed changes

yuja reviewed Feb 18, 2024

View reviewed changes

lib/src/local_working_copy.rs Show resolved Hide resolved

lib/src/file_util.rs Show resolved Hide resolved

gulbanana force-pushed the windows-symlinks branch from 55e87b2 to f725e8a Compare February 25, 2024 07:08

gulbanana force-pushed the windows-symlinks branch from f725e8a to 98fea9a Compare February 25, 2024 07:10

yuja reviewed Feb 25, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lib/src/file_util.rs Outdated Show resolved Hide resolved

lib/src/file_util.rs Outdated Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

lib/src/file_util.rs Outdated Show resolved Hide resolved

yuja approved these changes Feb 25, 2024

View reviewed changes

martinvonz mentioned this pull request Feb 28, 2024

windows: jj breaks on 'fake symlinks' generated by git #3164

Open

gulbanana added 2 commits March 5, 2024 15:03

tests: fix some failures on windows due to a missing TEMP environment…

8fe2839

… variable

working_copy: implement symlinks on windows with a helper function

474a792

enables symlink tests on windows, ignoring failures due to disabled developer mode, and updates windows.md

gulbanana force-pushed the windows-symlinks branch from 98fea9a to 474a792 Compare March 5, 2024 07:08

gulbanana enabled auto-merge (rebase) March 5, 2024 07:12

gulbanana merged commit d661f59 into martinvonz:main Mar 5, 2024
16 checks passed

gulbanana deleted the windows-symlinks branch March 24, 2024 02:41

ilyagr mentioned this pull request Jun 26, 2024

diff: symlinks with external diff tools produce invalid directory structures #3963

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

working_copy: implement symlinks on Windows with a helper function #2939

working_copy: implement symlinks on Windows with a helper function #2939

gulbanana commented Feb 4, 2024 •

edited

Loading

gulbanana commented Feb 4, 2024 •

edited

Loading

gulbanana commented Feb 4, 2024

gulbanana commented Feb 4, 2024

ilyagr commented Feb 4, 2024 •

edited

Loading

Symbolic link support

gulbanana commented Feb 7, 2024

Symbolic link support

gulbanana commented Feb 14, 2024

gulbanana commented Feb 16, 2024

gulbanana commented Feb 25, 2024

yuja left a comment

working_copy: implement symlinks on Windows with a helper function #2939

working_copy: implement symlinks on Windows with a helper function #2939

Conversation

gulbanana commented Feb 4, 2024 • edited Loading

Checklist

gulbanana commented Feb 4, 2024 • edited Loading

gulbanana commented Feb 4, 2024

gulbanana commented Feb 4, 2024

ilyagr commented Feb 4, 2024 • edited Loading

Symbolic link support

gulbanana commented Feb 7, 2024

Symbolic link support

gulbanana commented Feb 14, 2024

gulbanana commented Feb 16, 2024

gulbanana commented Feb 25, 2024

yuja left a comment

Choose a reason for hiding this comment

gulbanana commented Feb 4, 2024 •

edited

Loading

gulbanana commented Feb 4, 2024 •

edited

Loading

ilyagr commented Feb 4, 2024 •

edited

Loading