Implement os.link on Windows

[briancurtin]

BPO	8879
Nosy	@jcea, @amauryfa, @giampaolo, @briancurtin
Dependencies	bpo-7566: Add ntpath.sameopenfile support for Windows
Files	issue8879.diff issue8879_v2.diff py3k_quick_hack_for_issue8879.patch issue8879_unicode.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/briancurtin'
closed_at = <Date 2011-01-04.04:13:06.136>
created_at = <Date 2010-06-02.18:47:36.745>
labels = ['extension-modules', 'type-feature', 'OS-windows']
title = 'Implement os.link on Windows'
updated_at = <Date 2011-01-04.04:13:06.136>
user = 'https://github.com/briancurtin'

bugs.python.org fields:

activity = <Date 2011-01-04.04:13:06.136>
actor = 'brian.curtin'
assignee = 'brian.curtin'
closed = True
closed_date = <Date 2011-01-04.04:13:06.136>
closer = 'brian.curtin'
components = ['Extension Modules', 'Windows']
creation = <Date 2010-06-02.18:47:36.745>
creator = 'brian.curtin'
dependencies = ['7566']
files = ['18982', '19059', '19091', '19862']
hgrepos = []
issue_num = 8879
keywords = ['patch', 'needs review']
message_count = 20.0
messages = ['106908', '117236', '117238', '117325', '117642', '117828', '117834', '118013', '122305', '122306', '122319', '122341', '122363', '122415', '122748', '122749', '122750', '122767', '122770', '122771']
nosy_count = 5.0
nosy_names = ['jcea', 'amaury.forgeotdarc', 'ocean-city', 'giampaolo.rodola', 'brian.curtin']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue8879'
versions = ['Python 3.2']

[briancurtin]

Add os.link support for Windows

(mostly a reminder to myself to finish the patch I have)

[briancurtin]

Here's a patch of the functionality. The doc change is small and easy, I'll add it shortly.

One oddity is that os.path.samefile is False for a link and its source, but True for a symlink and its source. I find that to be quite odd, but stepping through the code I don't see where this is anything we are doing. I haven't been able to figure out if this is really expected behavior from Windows. When the files are open, os.path.sameopenfile shows that the link and its source are the same, which is expected.

[ocean-city]

With following implementation, issamefile return True
for hard link. I heard GetFinalPathNameByHandle
returns different paths for hard links.

>>> import nt, os
>>> def issamefile(path1, path2):
...     fd1 = os.open(path1, os.O_RDONLY)
...     fd2 = os.open(path2, os.O_RDONLY)
...     fi1 = nt._getfileinformation(fd1)
...     fi2 = nt._getfileinformation(fd2)
...     os.close(fd1)
...     os.close(fd2)
...     return fi1 == fi2
...
>>> issamefile("src.txt", "lnk.txt")
True

[briancurtin]

In a round-about way, that's what the tests do via sameopenfile.

Maybe the behavior of os.path.samefile for Windows hard links should be documented? Possibly just a small note explaining that os.path.sameopenfile is an alternate solution due to how GetFinalPathNameByHandle works.

[briancurtin]

Now that I think of it, that behavior is expected. Hard links are *supposed* to be different directory entries, so they would come out of that function as-is, and the current tests are correctly implemented.

Uploaded to http://codereview.appspot.com/2290042

[briancurtin]

test_tarfile fails with this patch.

[ocean-city]

I think this is because os.stat and os.lstat doesn't set st_nlink.
It is only set via os.fstat. I'll attach quick hack patch. (Again,
this is just quick hack patch)

I confirmed this passes test_os and test_tarfile.

[briancurtin]

I created bpo-10027 for the st_nlink issue to handle it separately.

[briancurtin]

Removing link to bpo-10027. It's fixed for py3k but the issue should stay open for backport to other branches.

[briancurtin]

Fixed in r86733.

[amauryfa]

The patch uses the ANSI version, and converts the filename from unicode to bytes; this will fail for names that cannot be encoded with the "mbcs" codec.

All other functions in the posix module first try the Wide version of the win32 API, and use the ANSI version as a fallback, when the argument is a bytes string. PyUnicode_FSConverter should be avoided on Windows. See posix_mkdir() for a good example.

Here is an example that fails on a Western Windows (where ANSI=cp1252). Note that even the file name is not encodable in mbcs, it is correctly displayed in the Explorer for example.

>>> name = "\u65e5\u672c"     # "Japan"
>>> open(name, "w").close()   # Appears correctly in the explorer
>>> import os
>>> os.link(name, name + "_1")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'mbcs' codec can't encode characters in position 0--1: invalid character

[ocean-city]

r86733 introduces st_ino setup in attribute_data_to_stat(),
but Fild ID is not guaranteed to be same when file is not opened.
So I think it's meaningful only for os.fstat().

Please see
http://msdn.microsoft.com/en-us/library/aa363788%28v=VS.85%29.aspx

[briancurtin]

I'll come up with a patch for Amaury's message.

Hirokazu - I didn't see that MSDN page, thanks. Without st_ino, I'll need to find a way around the block of lines 1941-1954 in Lib/tarfile.py. That's what was causing a test failure in the first place because it ends up assuming Windows files are always REGTYPE, which sets some things later on in that function differently than they should be for links, namely tarinfo.size.

[ocean-city]

Umm, I'm not sure how to fix this yet, but
if we create the function like os._stat_with_open_handle()
which returns stat struct and open handle on windows,
I think we can set/use st_ino. (Because FileID won't change
while file is opened) I'm not sure if it is appropriate API
change or not, though. :-(

[briancurtin]

Amaury -- how does issue8879_unicode.diff look? Made the suggested change and added a test.

[amauryfa]

Looks good, except that win32_error("link", NULL) should be called instead of posix_error(). errno is not guaranteed to be correctly set by the win32 api, and GetLastError() ususally gives more accurate errors.

[briancurtin]

Committed in r86854 with your win32_error suggestion. Thanks for your help and input.

[jcea]

This patch seems to break quite a few buildbots.

for instance: http://www.python.org/dev/buildbot/all/builders/x86%20OpenIndiana%203.x/builds/81/steps/test/logs/stdio

"""
test_mbcs_name (test.test_os.LinkTests) ... test test_os failed -- Traceback (most recent call last):
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_os.py", line 893, in test_mbcs_name
    self._test_link(self.file1, self.file2)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_os.py", line 876, in _test_link
    with open(file1, "w") as f1:
UnicodeEncodeError: 'ascii' codec can't encode characters in position 15-16: ordinal not in range(128)
"""

[briancurtin]

Maybe the test should be Windows-only?
I don't really know the answer...things tend to fall apart when I get involved with Unicode, encoding, codecs, etc. :/

[jcea]

I am not familiar with this either, but better to restrict the test to platforms with actual mbcs encoding.