Skip to content

<filesystem> support junctions in read_symlink#2877

Merged
StephanTLavavej merged 6 commits into
microsoft:mainfrom
strega-nil:fix-2818
Jul 28, 2022
Merged

<filesystem> support junctions in read_symlink#2877
StephanTLavavej merged 6 commits into
microsoft:mainfrom
strega-nil:fix-2818

Conversation

@strega-nil-ms
Copy link
Copy Markdown
Contributor

@strega-nil-ms strega-nil-ms commented Jul 18, 2022 •
edited
Loading

  • is_symlink(junction) is still false
  • read_symlink(junction) works and does the obvious thing
  • copy_symlink(junction) doesn't work - creating a directory symlink is bad, and creating a junction is not really supported by the windows API (and also a junction is not a symlink)
  • copy(junction) works and does the obvious thing

Fixes #2818

@strega-nil
<filesystem> support junctions in *_symlink
42adb57 
* `is_symlink(junction) -> true`
* `read_symlink(junction)` works
* `copy_symlink(junction)` works
@strega-nil-ms strega-nil-ms marked this pull request as ready for review July 18, 2022 20:59
@strega-nil-ms strega-nil-ms requested a review from a team as a code owner July 18, 2022 20:59
@strega-nil-ms strega-nil-ms changed the title <filesystem> support junctions in *_symlink <filesystem> support junctions in *_symlink Jul 18, 2022
@strega-nil-ms strega-nil-ms added bug Something isn't working filesystem C++17 filesystem labels Jul 18, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Jul 18, 2022

P0218R1_filesystem is failing with:

test_status @ 2885: check failed: is_other(file_status{file_type::junction})
Assertion failed: pass, file C:\a\1\s\tests\std\tests\P0218R1_filesystem\test.cpp, line 4044
abort() has been called

STL/tests/std/tests/P0218R1_filesystem/test.cpp

Line 2885 in ef62d3f

EXPECT(is_other(file_status{file_type::junction}));

Ideally, it's best to run a full test pass for x64 before submitting a PR. (For maintainers, if your local machine is slow, you can create a VM internally to run tests.) At a minimum, strongly related tests should be run, which will take just a few minutes. For example:

C:\GitHub\STL\tests\std\tests>dir /b *filesystem*
Dev11_1066931_filesystem_rename_noop
Dev11_1180290_filesystem_error_code
GH_001010_filesystem_error_encoding
P0218R1_filesystem
VSO_0121275_filesystem_canonical_should_handle_many_double_dots

@cpplearner
Copy link
Copy Markdown
Contributor

cpplearner commented Jul 19, 2022

I'm all for treating junctions as symlinks, but I'm not sure about some details.

  1. The standard says ([fs.op.is.symlink]):

    bool filesystem::is_symlink(file_status s) noexcept;
    Returns: s.type() == file_­type​::​symlink.

    But this PR makes is_symlink return something different.

  2. IIUC this PR makes copy_symlink create a symlink (as opposed to a junction) if the source is a junction. Since copy calls copy_symlink if is_symlink returns true, this means copy (still) will not make a faithful copy of a junction.

@strega-nil-ms
Copy link
Copy Markdown
Contributor Author

strega-nil-ms commented Jul 19, 2022

@cpplearner you're absolutely correct, thanks for pointing that out.

@strega-nil-ms strega-nil-ms changed the title <filesystem> support junctions in *_symlink <filesystem> support junctions in read_symlink Jul 19, 2022
@strega-nil
make fixes
b8dc090 
* `is_symlink(junction) = false`
* `copy_symlink(junction)` fails
* `copy(junction)` succeeds and creates a junction
* `read_symlink(junction)` correctly reads a junction
@strega-nil-ms
Copy link
Copy Markdown
Contributor Author

strega-nil-ms commented Jul 19, 2022

Waiting on LWG issue to link.

@strega-nil-ms strega-nil-ms added the blocked on LWG Waiting for WG21 to tell us what to do label Jul 20, 2022
@strega-nil-ms strega-nil-ms removed the blocked on LWG Waiting for WG21 to tell us what to do label Jul 25, 2022
@strega-nil-ms
Copy link
Copy Markdown
Contributor Author

strega-nil-ms commented Jul 25, 2022

We now have a LWG issue - LWG-3744!

StephanTLavavej
StephanTLavavej reviewed Jul 26, 2022
View reviewed changes
Comment thread stl/inc/filesystem Outdated
Comment thread stl/inc/filesystem Outdated
Comment thread stl/inc/filesystem Outdated
Comment thread stl/inc/filesystem Outdated
Comment thread stl/inc/xfilesystem_abi.h Outdated
Comment thread stl/inc/xfilesystem_abi.h Outdated
Comment thread stl/inc/filesystem Outdated
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Jul 26, 2022

Thanks for fixing this bug! I pushed minor stylistic changes, nothing should be controversial but please meow if you have concerns. 😺

@StephanTLavavej StephanTLavavej removed their assignment Jul 26, 2022
@strega-nil-ms
Copy link
Copy Markdown
Contributor Author

strega-nil-ms commented Jul 26, 2022

LGTM!
image

@StephanTLavavej StephanTLavavej self-assigned this Jul 27, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Jul 27, 2022

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit a0b84e0 into microsoft:main Jul 28, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Jul 28, 2022

Thanks for yet another <filesystem> improvement for Windows! 💾 📀 📂

@strega-nil strega-nil deleted the fix-2818 branch August 10, 2022 20:35
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@strega-nil-ms @strega-nil
@StephanTLavavej @fsb4000
<filesystem> support junctions in read_symlink (microsoft#2877)
069cb75 
Co-authored-by: nicole mazzuca <[email protected]>
Co-authored-by: Stephan T. Lavavej <[email protected]>
@LewisPringle
Copy link
Copy Markdown

LewisPringle commented Dec 5, 2023

I'm confused about the reasoning for is_symlink () returning false for juntions.

How is one to know to call read_symlink?

https://en.cppreference.com/w/cpp/filesystem/read_symlink says its an error to call read_symlink for soemthing that isn't a symlink.

My code:
if (filesystem::is_symlink (p)) {
p = filesystem_read_symlink(p);
}

doesn't work with our chosen fix. Not sure why the above should not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working filesystem C++17 filesystem

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<filesystem>: filesystem::read_symlink ignores junctions

5 participants

@strega-nil-ms @StephanTLavavej @cpplearner @LewisPringle @strega-nil