GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29248: Fix readlink bug os by SSE4 · Pull Request #5577 · python/cpython

Conversation

@SSE4

@SSE4 SSE4 commented Feb 7, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

see https://bugs.python.org/issue29248

a year already has passed since patch was provided. what should be done to make this into python release?

/cc @gvanrossum @berkerpeksag @asvetlov

https://bugs.python.org/issue29248

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SSE4
bpo-29248: Fix readlink bug os
2cb8ba6 
Signed-off-by: SSE4 <[email protected]>
@the-knights-who-say-ni

the-knights-who-say-ni commented Feb 7, 2018

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Feb 7, 2018
@bedevere-bot bedevere-bot added the awaiting review label Feb 7, 2018
@SSE4
add Misc/NEWS.d entry
3c8ce39 
Signed-off-by: SSE4 <[email protected]>
gvanrossum
gvanrossum reviewed Feb 7, 2018
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_os.py
and os.path.exists(r'C:\ProgramData'),
'Test directories not found')
def test_29248(self):
target = os.readlink(r'C:\Users\All Users')

@gvanrossum gvanrossum Feb 7, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add the explanation from the tracker issue (why we're using this link instead of creating a fresh one to test) here as a comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SSE4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@SSE4 SSE4 Feb 7, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SSE4
add explanation from the tracker issue
9893c55
@berkerpeksag

berkerpeksag commented Feb 7, 2018

Copy link
Copy Markdown
Member

@SSE4 please let us know when you sign the CLA form so we can merge this PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag
Cosmetic fixes
d23bd99
@SSE4

SSE4 commented Feb 7, 2018

Copy link
Copy Markdown
Contributor Author

@berkerpeksag I already did earlier today
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
berkerpeksag
berkerpeksag reviewed Feb 7, 2018
View reviewed changes
Comment thread Misc/NEWS.d/next/Windows/2018-02-07-17-50-48.bpo-29248.Xzwj-6.rst Outdated
@@ -0,0 +1 @@
fix os.readlink() on Windows

@berkerpeksag berkerpeksag Feb 7, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the news entry more descriptive and add "Patch by Craig Holmquist and Your Name."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SSE4 SSE4 Feb 7, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SSE4
make the news entry more descriptive
b0e82eb
serhiy-storchaka
serhiy-storchaka reviewed Feb 7, 2018
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
}
print_name = rdb->SymbolicLinkReparseBuffer.PathBuffer +
rdb->SymbolicLinkReparseBuffer.PrintNameOffset;
(rdb->SymbolicLinkReparseBuffer.PrintNameOffset / 2);

@serhiy-storchaka serhiy-storchaka Feb 7, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can PrintNameOffset be odd?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@eryksun eryksun Feb 8, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PathBuffer is of type wchar_t. Strictly this should use sizeof(wchar_t) instead of 2. Alternatively it can use two casts to have the compiler do the work: (wchar_t *)((char *)rdb->SymbolicLinkReparseBuffer.PathBuffer + rdb->SymbolicLinkReparseBuffer.PrintNameOffset).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SSE4 SSE4 Feb 8, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
eryksun
eryksun reviewed Feb 8, 2018
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
(rdb->SymbolicLinkReparseBuffer.PrintNameOffset / 2);

result = PyUnicode_FromWideChar(print_name,
rdb->SymbolicLinkReparseBuffer.PrintNameLength/2);

@eryksun eryksun Feb 8, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly this should also use sizeof(wchar_t) instead of hard coding 2.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SSE4 SSE4 Feb 8, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
SSE4 added 2 commits February 8, 2018 09:48
@SSE4
use sizeof(wchar_t) instead of hard coding 2
a816bcc
@SSE4
PrintNameOffset can be odd, use two casts to have the compiler do the…
3234a71 
… work
@SSE4

SSE4 commented Feb 9, 2018

Copy link
Copy Markdown
Contributor Author

/cc @DinoV @zooba can please take a look at this issue related to Microsoft Windows?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag removed the CLA not signed label Feb 9, 2018
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 9, 2018
@berkerpeksag
use reST format in NEWS entry
0d66fb3
berkerpeksag
berkerpeksag approved these changes Feb 9, 2018
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All review comments have been addressed and this looks good to me. I will wait for a day or two to give everyone a chance to look at the patch and merge it. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Feb 9, 2018
@SSE4

SSE4 commented Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

@berkerpeksag okay, 3 days passed already, what's the next action to be done?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag added needs backport to 3.6 labels Feb 12, 2018
@berkerpeksag
berkerpeksag merged commit 3c34aad into python:master Feb 12, 2018
@miss-islington

miss-islington commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

Thanks @SSE4 for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 12, 2018
@bedevere-bot

bedevere-bot commented Feb 12, 2018

Copy link
Copy Markdown

GH-5640 is a backport of this pull request to the 3.6 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 12, 2018
@SSE4 @miss-islington
bpo-29248: Fix os.readlink() on Windows (pythonGH-5577)
1dfe7a7 
The PrintNameOffset field of the reparse data buffer
was treated as a number of characters instead of bytes.
(cherry picked from commit 3c34aad)

Co-authored-by: SSE4 <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.6 label Feb 12, 2018
berkerpeksag pushed a commit that referenced this pull request Feb 12, 2018
@miss-islington @SSE4 @berkerpeksag
bpo-29248: Fix os.readlink() on Windows (GH-5577)
a1d33f7 
The PrintNameOffset field of the reparse data buffer
was treated as a number of characters instead of bytes.

(cherry picked from commit 3c34aad)

Co-authored-by: SSE4 <[email protected]>
@berkerpeksag

berkerpeksag commented Feb 12, 2018

Copy link
Copy Markdown
Member

@Mariatta do I need to backport this to 3.7 branch manually or is there some sort of queue?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta Mariatta added needs backport to 3.7 and removed needs backport to 3.7 labels Feb 12, 2018
@miss-islington

miss-islington commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

Thanks @SSE4 for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 12, 2018

Copy link
Copy Markdown

GH-5644 is a backport of this pull request to the 3.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Feb 12, 2018
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 12, 2018
@SSE4 @miss-islington
bpo-29248: Fix os.readlink() on Windows (pythonGH-5577)
e6a1b8d 
The PrintNameOffset field of the reparse data buffer
was treated as a number of characters instead of bytes.
(cherry picked from commit 3c34aad)

Co-authored-by: SSE4 <[email protected]>
@Mariatta

Mariatta commented Feb 12, 2018

Copy link
Copy Markdown
Member

@berkerpeksag I think this was merged at the same time miss-islington was being deployed, so it missed that webhook event. I've reapplied the label to trigger the backport.
berkerpeksag reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Feb 12, 2018

Copy link
Copy Markdown
Member

@Mariatta thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Feb 12, 2018
@miss-islington @SSE4
bpo-29248: Fix os.readlink() on Windows (GH-5577)
74ebbae 
The PrintNameOffset field of the reparse data buffer
was treated as a number of characters instead of bytes.
(cherry picked from commit 3c34aad)

Co-authored-by: SSE4 <[email protected]>
@SSE4 SSE4 mentioned this pull request Feb 13, 2018
conan export fails in Windows Docker container conan-io/conan#2439
Closed
3 tasks done
@merwok merwok changed the title bpo-29248: Fix readlink bug os s/merwokbpo-29248: Fix readlink bug os Feb 18, 2018
@merwok merwok changed the title s/merwokbpo-29248: Fix readlink bug os bpo-29248: Fix readlink bug os Feb 18, 2018
@merwok

merwok commented Feb 18, 2018

Copy link
Copy Markdown
Member

@berkerpeksag I noticed that the news file is the only one in the repo to use CRLF.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eryksun eryksun eryksun left review comments
@gvanrossum gvanrossum gvanrossum left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@berkerpeksag berkerpeksag berkerpeksag approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@SSE4 @the-knights-who-say-ni @berkerpeksag @miss-islington @bedevere-bot @Mariatta @merwok @eryksun @gvanrossum @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.