[go] windows: support app store symbolic links
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
windows: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 494c30e18b55fa8995f85658b1a26df43189de7f
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/file_windows.go
M src/syscall/types_windows.go
3 files changed, 79 insertions(+), 0 deletions(-)
diff --git a/src/internal/syscall/windows/reparse_windows.go b/src/internal/syscall/windows/reparse_windows.go
index 6e11139..c5752fa 100644
--- a/src/internal/syscall/windows/reparse_windows.go
+++ b/src/internal/syscall/windows/reparse_windows.go
@@ -5,13 +5,16 @@
package windows
import (
+ "errors"
"syscall"
+ "unicode/utf16"
"unsafe"
)
const (
FSCTL_SET_REPARSE_POINT = 0x000900A4
IO_REPARSE_TAG_MOUNT_POINT = 0xA0000003
+ IO_REPARSE_TAG_APPEXECLINK = 0x8000001B
SYMLINK_FLAG_RELATIVE = 1
)
@@ -88,3 +91,40 @@
n2 := (rb.SubstituteNameOffset + rb.SubstituteNameLength) / 2
return syscall.UTF16ToString((*[0xffff]uint16)(unsafe.Pointer(&rb.PathBuffer[0]))[n1:n2:n2])
}
+
+type GenericDataBuffer struct {
+ DataBuffer [1]uint8
+}
+
+type AppExecLinkReparseBuffer struct {
+ Version uint32
+ StringList [1]uint16
+}
+
+// Path returns path stored in rb
+// The buffer contains 4 null terminated strings inside
+// StringList. The third string contains the path to the
+// executable
+func (rb *AppExecLinkReparseBuffer) Path() (string, error) {
+ UTF16ToStringPosition := func(s []uint16) (string, int) {
+ for i, v := range s {
+ if v == 0 {
+ s = s[0:i]
+ return string(utf16.Decode(s)), i
+ }
+ }
+ return "", 0
+ }
+ stringList := (*[0xffff]uint16)(unsafe.Pointer(&rb.StringList[0]))[0:]
+ var link string
+ var position int
+ for i := 0; i <= 2; i++ {
+ link, position = UTF16ToStringPosition(stringList)
+ position++
+ if position >= len(stringList) {
+ return "", errors.New("invalid AppExecLinkReparseBuffer")
+ }
+ stringList = stringList[position:]
+ }
+ return link, nil
+}
diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index 378e2b1..baa44fe 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -481,6 +481,17 @@
return normaliseLinkPath(s)
case windows.IO_REPARSE_TAG_MOUNT_POINT:
return normaliseLinkPath((*windows.MountPointReparseBuffer)(unsafe.Pointer(&rdb.DUMMYUNIONNAME)).Path())
+ case windows.IO_REPARSE_TAG_APPEXECLINK:
+ rb := (*windows.GenericDataBuffer)(unsafe.Pointer(&rdb.DUMMYUNIONNAME))
+ appExecLink := (*windows.AppExecLinkReparseBuffer)(unsafe.Pointer(&rb.DataBuffer))
+ if appExecLink.Version != 3 {
+ return " ", errors.New("unknown AppExecLink version")
+ }
+ s, err := appExecLink.Path()
+ if err != nil {
+ return "", err
+ }
+ return normaliseLinkPath(s)
default:
// the path is not a symlink or junction but another type of reparse
// point
diff --git a/src/syscall/types_windows.go b/src/syscall/types_windows.go
index 384b5b4..80ba89f 100644
--- a/src/syscall/types_windows.go
+++ b/src/syscall/types_windows.go
@@ -1163,6 +1163,7 @@
MAXIMUM_REPARSE_DATA_BUFFER_SIZE = 16 * 1024
_IO_REPARSE_TAG_MOUNT_POINT = 0xA0000003
IO_REPARSE_TAG_SYMLINK = 0xA000000C
+
SYMBOLIC_LINK_FLAG_DIRECTORY = 0x1
_SYMLINK_FLAG_RELATIVE = 1
)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
windows: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 85fc2275788461d2abb6e226eda6ad78c8dec453
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/file_windows.go
2 files changed, 78 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
7 comments:
Commit Message:
Patch Set #2, Line 7:
windowss/windows/os/
because your change fixes os package.
Does this change fixes
https://github.com/golang/go/issues/42919
?
I tried running sample program listed in
https://github.com/golang/go/issues/42919#issue-754039436
and the program still fails. Why?
If this change fixes issue 42919, then add
Fixes #42919
line here.
Patchset:
Jan, thank you for sending your fix.
Alex
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #2, Line 95:
type GenericDataBuffer struct {I don't think you need to add this struct. See my comment in the code that uses GenericDataBuffer.
Patch Set #2, Line 99:
type AppExecLinkReparseBuffer struct {Need some documentation references that describe this struct. Like URLs on line 23 and 24.
Patch Set #2, Line 105:
// The buffer contains 4 null terminated strings insideHow do you know this API is structured in this way? Need some references.
File src/os/file_windows.go:
Patch Set #2, Line 486:
&rb.DataBufferWhy do you need to convert pointer into *windows.GenericDataBuffer first before converting it into *windows.AppExecLinkReparseBuffer?
I think you can just convert it all in one line. Like this
s/&rb.DataBuffer/&rdb.DUMMYUNIONNAME/
. Isn't it?
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #3 to this change.
windows: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: ca2a55e7211394cd61324035efae035e27349303
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/file_windows.go
2 files changed, 82 insertions(+), 1 deletion(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #4 to this change.
windows: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: b0ec856f297a3634f87564ffb09b071f43a31025
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/file_windows.go
2 files changed, 76 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
5 comments:
Commit Message:
Patch Set #2, Line 7:
windows
s/windows/os/ […]
Done
Does this change fixes […]
This change adds support for `os.Readlink()`, I'll see if I can find the code path to also support the issue in #42919 as that should be feasible.
If you change the sample code to this, you can make it work (this doesn't work on the latest master):
package main
import (
"errors"
"fmt"
"os"
"os/exec"
)func do() error {
python := `C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`
res, err := os.Lstat(python)
if err != nil {
return errors.New("python not found")
}
fmt.Println("python3 exists!")
fmt.Printf("%#v\n", res)
location, err := os.Readlink(python)
if err != nil {
return errors.New("python3 is not symlinked!")
}
cmd := exec.Command(location, "-c", "print('hello')")
btes, err := cmd.CombinedOutput()
if err != nil {
return err
}
fmt.Printf("%s\n", string(btes))
return nil
}func main() {
err := do()
if err != nil {
panic(err)
}
}
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #2, Line 99:
type AppExecLinkReparseBuffer struct {
Need some documentation references that describe this struct. Like URLs on line 23 and 24.
Done
Patch Set #2, Line 105:
// The buffer contains 4 null terminated strings insideHow do you know this API is structured in this way? Need some references.
Done
File src/os/file_windows.go:
Patch Set #2, Line 486:
&rb.DataBuffer
Why do you need to convert pointer into *windows. […]
I see what you mean, but the rdb.DUMMYUNIONNAME field is a GenericDataBuffer struct that contains the DataBuffer which in this case is a AppExecLinkReparseBuffer. But looking at the MS docs, this can also be something else. So for future extensibility I also wanted to design it as such.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/file_windows.go:
Patch Set #2, Line 486:
&rb.DataBuffer
I see what you mean, but the rdb. […]
I see what you mean now, changed.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Jan De Dobbeleer, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
5 comments:
Commit Message:
Patch Set #2, Line 7:
windows
Done
I still nothing changed here in Gerrit.
I can see you changed your commit message at
https://github.com/golang/go/pull/51081
Maybe try changing PR subject line too
s/windows/os/
This change adds support for `os. […]
Indeed.
Please add
Fixes #42919
line here.
Patchset:
More comments.
Thank you.
Alex
File src/internal/syscall/windows/reparse_windows.go:
// these structs are described here:
// https://stackoverflow.com/a/65583702
// unfortunately, there's no official Microsoft documentation yetReplace lines 95-97 with this comment
// AppExecLinkReparseBuffer is described at
// https://stackoverflow.com/a/65583702
// It appears there is no official Microsoft
// documentation for AppExecLinkReparseBuffer yet.or similar.
The comment should be proper English text - including capital letters and punctuation marks.
Patch Set #4, Line 107:
func (rb *AppExecLinkReparseBuffer) Path() (string, error) {Your code looks too complicated. How about this instead
```
func (rb *AppExecLinkReparseBuffer) Path() string {
for j, from, i, p := 0, 0, 0, (*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0])); true; i++ {
if p[i] == 0 {
// return 3rd string
if j == 2 {
return string(utf16.Decode(p[from:i]))
}
j++
from = i + 1
}
}
return ""
}
```?
Copied from syscall.Environ.
Here is playground link https://go.dev/play/p/b-S8ZEc1Hml in case you want to play with it.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
3 comments:
Commit Message:
Indeed. […]
I'm going to see if I can add support for exec.RunCommand directly first. As that should be possible, I just don't know yet why it can't resolve the link in that case.
File src/internal/syscall/windows/reparse_windows.go:
// these structs are described here:
// https://stackoverflow.com/a/65583702
// unfortunately, there's no official Microsoft documentation yet
Replace lines 95-97 with this comment […]
Done
Patch Set #4, Line 107:
func (rb *AppExecLinkReparseBuffer) Path() (string, error) {
Your code looks too complicated. How about this instead […]
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #5 to this change.
os: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: cfd1baf17ef2277d7c9cbe941377da1fb6451156
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/file_windows.go
2 files changed, 66 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #6 to this change.
os: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 67a2506cd2d180543b4edc46f65219ec5747550b
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
3 files changed, 81 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
2 comments:
Commit Message:
I'm going to see if I can add support for exec.RunCommand directly first. […]
Done
File src/os/stat_windows.go:
Patch Set #6, Line 49:
func resolveWindowsStoreApp(name string) string {While the fixes the linked issue, I would love to find another way (but have no idea how that could work). This makes os.stat and exec.Command work as intended.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #7 to this change.
os: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: f11bcbbf5a7fdd6350ac05f84ebd5508cc4811aa
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/exec/lp_windows.go
M src/os/file_windows.go
3 files changed, 70 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #8 to this change.
os: support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: aaf7044240a40d78d1fcddc085406e0ebb458547
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/exec/lp_windows.go
M src/os/file_windows.go
3 files changed, 69 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/stat_windows.go:
Patch Set #6, Line 49:
func resolveWindowsStoreApp(name string) string {
While the fixes the linked issue, I would love to find another way (but have no idea how that could […]
I found a clean way, it only needs to also still look for symbolic links in lookpath before deciding to call it quits.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #2, Line 95:
type GenericDataBuffer struct {
I don't think you need to add this struct. See my comment in the code that uses GenericDataBuffer.
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Joseph (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Patch set 8:Code-Review +1
1 comment:
Patchset:
I tested this on my machine. Here's a simple repro and test result: https://github.com/golang/go/issues/42919#issuecomment-1103251889
LGTM.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Patch set 8:Run-TryBot +1
4 comments:
Commit Message:
Patch Set #8, Line 7:
os: support app store symbolic linkss/os: support app store symbolic links/os: make Readlink support app store symbolic links/
to have CL subject line reflect what this CL does.
Patchset:
Thanks for keeping up with me.
Still couple more changes.
Alex
File src/os/exec/lp_windows.go:
Patch Set #8, Line 51:
if file, err := os.Readlink(file); err == nil {I don't see why changing this source file is required.
None of other link types needed this change. Why are the app store symbolic links different?
File src/os/file_windows.go:
if appExecLink.Version != 3 {
return " ", errors.New("unknown AppExecLink version")
}
I think that check for correct version (lines 487-489) should be part of `appExecLink.Path` calculation. The check and the calculation are clearly related to each other.
You should move lines 487-489 into `windows.AppExecLinkReparseBuffer.Path` method.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
3 comments:
Commit Message:
Patch Set #8, Line 7:
os: support app store symbolic links
s/os: support app store symbolic links/os: make Readlink support app store symbolic links/ […]
Done
File src/os/exec/lp_windows.go:
Patch Set #8, Line 51:
if file, err := os.Readlink(file); err == nil {
I don't see why changing this source file is required. […]
They do not resolve, I don't know why that's the case, but this for sure doesn't work unless we explicitly try to resolve this. Otherwise it just returns file not found.
File src/os/file_windows.go:
if appExecLink.Version != 3 {
return " ", errors.New("unknown AppExecLink version")
}
I think that check for correct version (lines 487-489) should be part of `appExecLink. […]
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #9 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 82a6a28a7bf661ade562c37df9c3983b972e1da0
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/exec/lp_windows.go
M src/os/file_windows.go
3 files changed, 69 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/exec/lp_windows.go:
Patch Set #8, Line 51:
if file, err := os.Readlink(file); err == nil {
They do not resolve, I don't know why that's the case, but this for sure doesn't work unless we expl […]
Unless we need to fix stat to actually return the right information as the right fix. If I get this correctly, it seems that stat should be able to resolve symbolic links and does so using CreateFile (which is a syscall) and that one fails on these type of links. So maybe addressing the issue there is the correct location as the will make stat and thus this work as well.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #10 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 74bdeaac09d0aa602e9377d5d3115b5bfd34489d
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/os/exec/lp_windows.go
M src/os/file_windows.go
3 files changed, 70 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #11 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 521c9693402541882fadcfac1040e60914206110
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 81 insertions(+), 5 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/exec/lp_windows.go:
Patch Set #8, Line 51:
if file, err := os.Readlink(file); err == nil {
Unless we need to fix stat to actually return the right information as the right fix. […]
I found a better way, in this specific case CreateFile fails with ERROR_CANT_ACCESS_FILE and addressing it there will also make it so the entire sample code in https://github.com/golang/go/issues/42919 works as expected.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/exec/lp_windows.go:
Patch Set #8, Line 51:
if file, err := os.Readlink(file); err == nil {
I found a better way, in this specific case CreateFile fails with ERROR_CANT_ACCESS_FILE and address […]
Alex, I have a question as the latest change implies a difference in behaviour. For these files, using os.Lstat(`C:\Users\jan\AppData\Local\Microsoft\WindowsApps\python3.exe`) will return an os.FileStat object pointing to C:\Users\jan\AppData\Local\Microsoft\WindowsApps\python3.exe. Using os.Stat(`C:\Users\jan\AppData\Local\Microsoft\WindowsApps\python3.exe`) however will follow the link and return an os.FileStat object pointing to C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.10_3.10.1264.0_x64__qbz5n2kfra8p0\python3.10.exe". I feel like it should be the other around BUT that will break the issue in #42919 as LookPath() uses os.Stat and not os.Lstat.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/exec/lp_windows.go:
Patch Set #8, Line 51:
if file, err := os.Readlink(file); err == nil {
Alex, I have a question as the latest change implies a difference in behaviour. […]
Never mind, I misread. This is actually correct.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
6 comments:
Patchset:
This is getting tricky. :-)
Alex
File src/internal/syscall/windows/syscall_windows.go:
Patch Set #11, Line 46:
ERROR_CANT_ACCESS_FILEWe are not allowed to add new exported consts to syscall package. You have to use `internal/syscall/windows` package for that.
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.If you plan to also change os.Stat, then you should change all symlink related code. For example, grep for IO_REPARSE_TAG_SYMLINK and IO_REPARSE_TAG_MOUNT_POINT.
Patch Set #11, Line 96:
defer syscall.CloseHandle(h)You did not consider scenario where err != nil and h is garbage. In that scenario syscall.CloseHandle parameter will be garbadge.
Patch Set #11, Line 102:
if err == windows.ERROR_CANT_ACCESS_FILE {Maybe also check fa.FileAttributes to see if it is a symlink. We don't want every os.Stat user to call os.Reasdlink here before they will get error. That would be too expensive.
Line 105 is part of `stat` function. Do you intend to call `stat` recursively?
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #12 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 782602a34482d39aca5eb9842b893af990bb7471
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 80 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
6 comments:
Patchset:
This is getting tricky. :-) […]
I know :-) Otherwise someone would have solved this already.
File src/internal/syscall/windows/syscall_windows.go:
Patch Set #11, Line 46:
ERROR_CANT_ACCESS_FILE
We are not allowed to add new exported consts to syscall package. […]
this is in `internal/syscall/windows` unless I'm missing the point
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.
If you plan to also change os.Stat, then you should change all symlink related code. […]
I don't understand what you mean by this.
Patch Set #11, Line 96:
defer syscall.CloseHandle(h)
You did not consider scenario where err != nil and h is garbage. In that scenario syscall. […]
Done
Patch Set #11, Line 102:
if err == windows.ERROR_CANT_ACCESS_FILE {
Maybe also check fa.FileAttributes to see if it is a symlink. We don't want every os. […]
Done
Line 105 is part of `stat` function. […]
I did. We need the actual path to then do stat again. But, looking at your previous remarks, only doing this in case of a symlink should not introduce an issue. The problem is that for regular symlinks, CreateFile does work. But not for these. So handling links by resolving the link is in my opinion the cleanest fix rather than rely on CreateFile do some magic.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #13 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: a8678b1759bf639f294a77856f424a9035277fbc
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 79 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #14 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 608b935e4c12576c119f7ea7805f2ae86db43f26
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 77 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #15 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 9bed37863b23efd7751a2435a94b8558486659bf
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 80 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #16 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 576d33e98136503bb6e806ee639b31924b09a78e
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 80 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/stat_windows.go:
I did. We need the actual path to then do stat again. […]
Found a solution that I feel is very acceptable, doesn't change the behaviour and simply works.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #17 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 57b31bd0cb26d596bfe1fa6348751a1126b40022
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 80 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.I don't understand what you mean by this.
I validated the existing functionality using a symlink I created that points to the same file. On master, this works for my symlink, but fails on the `IO_REPARSE_TAG_APPEXECLINK` symlink. I assume that's an issue in win32, as that should work imho the same, regardless. With this change, there's no difference in output for `Stat` for both, everything works as designed from an API perspective.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Jan De Dobbeleer, Brad Fitzpatrick, Robert Griesemer, Ian Lance Taylor, Russ Cox.
3 comments:
File src/internal/syscall/windows/syscall_windows.go:
Patch Set #11, Line 46:
ERROR_CANT_ACCESS_FILE
this is in `internal/syscall/windows` unless I'm missing the point
I am mistakenly assumed you are using `syscall` package. Please ignore my comment.
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.
I validated the existing functionality using a symlink I created that points to the same file. […]
If you expect this change to fix all app link features, then we need new tests that verify everything - see `os.TestSymlink` for an example. Also you can add test that runs python.exe there too.
It would be nice if we can create app link files on the fly and then delete them. If not, then add test that uses existing symlink. We can quietly skip the test, if the file is not present.
I will use your new test manually here to verify your changes.
File src/os/stat_windows.go:
Patch Set #17, Line 110:
defer syscall.CloseHandle(h)Restore original blank line between 110 and 111.
IMHO the blank line helps to see that handle h allocated on line 99 is released on line 110.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
2 comments:
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.
If you expect this change to fix all app link features, then we need new tests that verify everythin […]
Can I add an app link file to the `testdata` folder so there's an integration test (0 byte file anyways)? As I can't create a file on the fly (tried it, no avail).
File src/os/stat_windows.go:
Patch Set #17, Line 110:
defer syscall.CloseHandle(h)
Restore original blank line between 110 and 111. […]
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
Gerrit Bot uploaded patch set #18 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 773195e152b77a2cdb3be1e9ddc8ddefb3423a19
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
4 files changed, 81 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
1 comment:
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.
Can I add an app link file to the `testdata` folder so there's an integration test (0 byte file anyw […]
I doubt you can create app link file in `testdata` that will be handled properly by git and on variety of OSes that Go supports (like Linux, MacOS, Plan 9 and others).
Just hardcode whatever path that python executable uses. As long I (and others willing to install python for this exercise) can run your test, we are good enough.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
Gerrit Bot uploaded patch set #19 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: ecd4cebafef79e576b5a1c070a06efb5db90fc97
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
A src/os/stat_windows_test.go
5 files changed, 113 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
1 comment:
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.
I doubt you can create app link file in `testdata` that will be handled properly by git and on varie […]
I did this, but forgot to ping you...
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
11 comments:
Commit Message:
Patch Set #19, Line 3:
AuthorDate: 2022-05-11 09:13:21 +0000Rebase your CL onto current master, so it is up to date.
Add line that says
Fixes #42919
if this CL fixes the issue or
Updates #42919
if it fixes it, but not completely.
Patchset:
Please, see my comments.
Thank you.
Alex
File src/os/stat_windows.go:
Patch Set #11, Line 93:
// First use CreateFile.
I did this, but forgot to ping you...
No worries. Let's keep going.
File src/os/stat_windows.go:
Patch Set #19, Line 104:
if err == windows.ERROR_CANT_ACCESS_FILE && reparsePoint != 0 {It is pointless to check reparsePoint value here. The only way for us to be on line 104 is if our file is reparse point. So revert your changes on lines 61-66.
File src/os/stat_windows_test.go:
Patch Set #19, Line 1:
// Copyright 2022 The Go Authors. All rights reserved.Let's not create new test file (we already have too many). Let's put this short test into os_windows_test.go.
Patch Set #19, Line 12:
"testing"I tried building your test and it fails to build:
```
$ GOOS=windows go test -v -c -o ~/pub/test.exe os
# os_test [os.test]
os/stat_windows_test.go:8:2: imported and not used: "internal/testenv"
os/stat_windows_test.go:9:2: imported and not used: "io/fs"
os/stat_windows_test.go:11:2: imported and not used: "runtime"
os/stat_windows_test.go:18:30: undefined: os
os/stat_windows_test.go:19:14: undefined: Lstat
os/stat_windows_test.go:25:13: undefined: Stat
$
```Patch Set #19, Line 17:
pythonVersions/pythonVersion/pythonExeName/
because pythonVersion is confusing for me. Pick different name, if you don't like my suggestion.
Patch Set #19, Line 23:
returnReplace return with
t.Skip("skipping test, because Python 3 is not installed via the Windows App Store on this system; see https://golang.org/issue/42919")
and then you will not even need comments above.
Unlike return, t.Skip will at least display a warning if user runs your test with -v flag.
s/res/res.Name()/
No?
Patch Set #19, Line 30:
%s != %ss/%s != %s/got %q, but wanted %q/
for clearer error message
and %q to have file name quoted in case there is a space in the name.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
Gerrit Bot uploaded patch set #20 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: ee93d0f497bc192106dc6087d0247b816add6a0e
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
5 files changed, 97 insertions(+), 2 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
Gerrit Bot uploaded patch set #21 to this change.
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 170fa14bce86e3f3ef4d080093acb2ff1e8e6b15
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
5 files changed, 91 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
Patch set 21:Run-TryBot +1
1 comment:
Patchset:
Hello Jan,
You did not send your replies to me questions again.
But it appears you made changes I requested.
I will run try-bots. And then I will try your change on my laptop.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
1 comment:
Patchset:
LGTM after you address this
https://go-review.googlesource.com/c/go/+/384160/comment/cf571e31_6b2d316e/
not yet addressed comment.
I verified that your new test PASSes on my laptop that has python3 app installed.
Thank you.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
9 comments:
Commit Message:
Patch Set #19, Line 3:
AuthorDate: 2022-05-11 09:13:21 +0000Rebase your CL onto current master, so it is up to date.
Done
Add line that says […]
This is already in the commit message
File src/os/stat_windows.go:
Patch Set #19, Line 104:
if err == windows.ERROR_CANT_ACCESS_FILE && reparsePoint != 0 {
It is pointless to check reparsePoint value here. […]
Done
File src/os/stat_windows_test.go:
Patch Set #19, Line 1:
// Copyright 2022 The Go Authors. All rights reserved.
Let's not create new test file (we already have too many). […]
Done
Patch Set #19, Line 12:
"testing"
I tried building your test and it fails to build: […]
Done
Patch Set #19, Line 17:
pythonVersion
s/pythonVersion/pythonExeName/ […]
Done
Patch Set #19, Line 23:
return
Replace return with […]
Done
s/res/res.Name()/ […]
Done
Patch Set #19, Line 30:
%s != %s
s/%s != %s/got %q, but wanted %q/ […]
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
1 comment:
Patchset:
LGTM after you address this […]
I forgot to press reply so my comment wasn't yet pushed. I added that statement to the commit message, this isn't updated in the description here.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Robert Griesemer, Russ Cox.
Patch set 21:Auto-Submit +1Code-Review +2
2 comments:
Patchset:
I still don't see Gerrit commit message changed.
I can see you changed commit message on Github. But your Github PR message does not include issue references number in it.
I suspect Go bot that copies Github changes into Gerrit uses PR message and ignores commit messages.
See
https://go.dev/doc/contribute#sending_a_change_github
... and the final commit description will be composed by concatenating the pull request's title and description. The individual commits' descriptions will be discarded. ...
I still don't see CL message changed - see below.
But LGTM anyway.
I will mark this CL +2 and auto-submit. Then, once 2 Googlers give this CL +1, it will get automatically submitted. This URL
is what they use to monitor CLs that needs their +1s. Hopefully your CL will be there.
Please update CL message anyway. Hopefully you will update it before CL gets submitted.
Thank you.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
2 comments:
Patchset:
I still don't see CL message changed - see below. […]
I adjusted it at GitHub, seems to have flown through.
I still don't see Gerrit commit message changed. […]
I adjusted it at GitHub, seems to have flown through.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
Gerrit Bot uploaded patch set #22 to this change.
The following approvals got outdated and were removed: Auto-Submit+1 by Alex Brainman
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Fixes #42919
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 170fa14bce86e3f3ef4d080093acb2ff1e8e6b15
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
5 files changed, 93 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Robert Griesemer, Russ Cox.
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))This pointer conversion seems likely to blow up under `-d=checkptr` (compare #38355, #34972).
Instead, use a pointer-increment loop like the one in `windows.UTF16PtrToString`.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Russ Cox.
Bryan Mills removed Robert Griesemer from this change.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
Patch set 22:Run-TryBot +1Code-Review +2
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))
This pointer conversion seems likely to blow up under `-d=checkptr` (compare #38355, #34972). […]
Good catch Bryan.
Jan, let me know, if you need help with that.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))
Good catch Bryan. […]
I'm going to try to give this a go but I have to be honest, my pointer skill game here is low.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox.
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))
I'm going to try to give this a go but I have to be honest, my pointer skill game here is low.
You might be able to trigger that crash during your test if you do this
```
go test -gcflags=all=-d=checkptr ...
```Also see
https://go-review.googlesource.com/c/go/+/208617
for context.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))
You might be able to trigger that crash during your test if you do this […]
So, it turns out we don't need to parse this link format. I was finally able to debug the latest changes (changed laptop to ARM in the meantime and debugging with delve was broken). I used the same code snippet as in #42919 and it all works as designed by only doing the fallback to opening the symbolic link. @alex.b...@gmail.com can you validate on your end as well?
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
1 comment:
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))
So, it turns out we don't need to parse this link format. […]
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #23 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Alex Brainman, TryBot-Result+1 by Gopher Robot
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Fixes #42919
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: ff0eb0ce063f33bd97324db9ecfe3d8ba763f5f4
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/syscall_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
3 files changed, 53 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox.
Patch set 23:Code-Review +2
2 comments:
Patchset:
Still LGTM.
Nice and simple now.
Alex
File src/internal/syscall/windows/reparse_windows.go:
Patch Set #22, Line 112:
(*[1 << 24]uint16)(unsafe.Pointer(&rb.StringList[0]))
Done
I just run TestAppLinkSymlinkStat on top of your patchset 22. And the test still PASSes.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox.
1 comment:
Patchset:
Bryan,
This CL has +2 and no unresolved comments now.
But I still do not see it listed on
. Can you, please, tell us what else we need to submit this CL?
Thank you.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox.
Patch set 23:Run-TryBot +1
4 comments:
Patchset:
Bryan, […]
To show up on `needs-review` it also needs a TryBot +1 on the current patchset.
TRY=windows-amd64-longtest
File src/os/stat_windows.go:
Patch Set #23, Line 98:
// In this case we need to open the link and continue with the right handle.How do we know that `ERROR_CANT_ACCESS_FILE` necessarily implies an app executable file here? (Or, why would `openSymlink` be appropriate to try for other cases?)
Also, why don't these links end up in the `FILE_ATTRIBUTE_REPARSE_POINT` case at line 62? (A bit more detail in the comment would be helpful.)
Patch Set #23, Line 101:
h, err = openSymlink(name)If `openSymlink` fails, we'll still report the error as a `CreateFile` error at line 104. `openSymlink` does use `CreateFile` today, but that isn't locally obvious — I think we should either restore the original error if `openSymlink` fails, or have `openSymlink` itself return a `PathError` that we can return directly here.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
2 comments:
File src/os/stat_windows.go:
Patch Set #23, Line 98:
// In this case we need to open the link and continue with the right handle.
How do we know that `ERROR_CANT_ACCESS_FILE` necessarily implies an app executable file here? (Or, w […]
Good question, I'll have to debug this again to add that context. Will do.
Patch Set #23, Line 101:
h, err = openSymlink(name)
If `openSymlink` fails, we'll still report the error as a `CreateFile` error at line 104. […]
Will have a look.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
Gerrit Bot uploaded patch set #24 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Bryan Mills, TryBot-Result+1 by Gopher Robot
os: make Readlink support app store symbolic links
`IO_REPARSE_TAG_APPEXECLINK` are types of links that are created when
installing an application via the Windows App Store. The location
of these links is added to the Windows PATH and the system can
resolve them. This adds support for these links so go can resolve them.
Fixes #42919
Illustrated to work in [oh-my-posh](https://github.com/JanDeDobbeleer/oh-my-posh/pull/1719/commits/7618dc6cbd00c0a79924e0286b821aa50c46dc74) and confirmed by end-users.
To reproduce the current issue:
- Install a Windows Store App like [Python3.10](https://www.microsoft.com/store/productId/9PJPW5LDXLZ5)
- The executable is added to your PATH and can be called from any terminal `python3 --version`
- Resolving the executable in PowerShell points to `$Env:LOCALAPPDATA/Microsoft/WindowsApps/python3.exe` which is a symbolic link
- Trying to resolve the symbolic link in golang 1.17 results in an error as `IO_REPARSE_TAG_APPEXECLINK` isn't supported: `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3`)`
With this change `IO_REPARSE_TAG_APPEXECLINK` is now supported and `os.Readlink(`C:/Users/jan/AppData/Local/Microsoft/WindowsApps/python3.exe`)` returns the actual path to the executable.
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 885f12abd1f602ce400c3ec153d57997bc72e478
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/syscall_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
3 files changed, 60 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
2 comments:
File src/os/stat_windows.go:
Patch Set #23, Line 98:
// In this case we need to open the link and continue with the right handle.
Good question, I'll have to debug this again to add that context. Will do.
I validated all known use-cases here. Line 62 rightfully doesn't match as this is a symbolic link just like a regular one. No errors and the correct file attributes. The only difference is that for `APPEXECLINKS`, `Createfile` fails to open them (one could see that as a win32 bug). Non-existing files fail with a different error so this path is only hit in that use-case as regular files do fall within the line 62 block.
Patch Set #23, Line 101:
h, err = openSymlink(name)
Will have a look.
Made sure to also simply use `CreateFile` here as there's no gain in using `openSymlink` directly. This was we preserve the context.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
Patch set 24:-Code-Review
3 comments:
Patchset:
I discovered problem with your change. See my comments below.
Thank you.
Alex
File src/os/os_windows_test.go:
You ignore check for os.Lstat result. If you add check like this one:
```
if res.Name() != pythonPath {
t.Fatalf("Lstat %s: got %q, but wanted %q", pythonPath, res.Name(), pythonPath)
}
```you will see that your test fails.
File src/os/stat_windows.go:
Patch Set #24, Line 105:
createFileAttrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINTI just reread https://github.com/golang/go/issues/42919 .
And from
https://github.com/golang/go/issues/42919#issuecomment-740371388
A naive thought here would be to consider passing syscall.FILE_FLAG_OPEN_REPARSE_POINT to CreateFile from os.Stat similar to what os.Lstat
- this is what your change does.
And your change appears to break Lstat for AppLink file. See my comment in TestAppLinkSymlinkStat.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox.
2 comments:
File src/os/os_windows_test.go:
You ignore check for os.Lstat result. If you add check like this one: […]
When I call the following, I get `python3.exe` as `res.Name()` which is correct, no? It would never return the full path like `pythonPath`, just the file name.
```python
exe := `C:\Users\jan\AppData\Local\Microsoft\WindowsApps\python3.exe`
res, err := os.Lstat(exe)
```
File src/os/stat_windows.go:
Patch Set #24, Line 105:
createFileAttrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINT
I just reread https://github.com/golang/go/issues/42919 . […]
Validated again and added a comment.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox.
1 comment:
File src/os/os_windows_test.go:
When I call the following, I get `python3.exe` as `res. […]
Yes. You are correct that res.Name() should be set to "python3.exe", not to "C:\Users\jan\AppData\Local\Microsoft\WindowsApps\python3.exe".
I got confused reviewing your CL. I don't concentrate on your CL for long enough. So I decided to spent some time on this issue. And I came up with slightly different solution - https://go-review.googlesource.com/c/go/+/452375 . But my solution is based on all the work and discovery that you did here.
If you are happy with https://go-review.googlesource.com/c/go/+/452375, I urge you to copy the code and CL message into your CL. And then I will approve it and we will finally submit this.
Thank you.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Russ Cox.
1 comment:
File src/os/os_windows_test.go:
Yes. You are correct that res.Name() should be set to "python3. […]
done!
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Gerrit Bot uploaded patch set #25 to this change.
os: add support for AppExecLinks
This change adds support for AppExecLinks files, like
C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3.exe
The python3.exe can be installed by following these
https://www.microsoft.com/store/productId/9PJPW5LDXLZ5
instructions. The executable is added to your PATH and can be called
from command line, like `python3 --version`.
Calling GetFileAttributesEx on python3.exe returns FileAttributes with
FILE_ATTRIBUTE_REPARSE_POINT set. And os.Stat attempts to follow the
link. But Microsoft does not provide any link target for AppExecLinks
files (see
https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 ),
and Go should treat AppExecLinks as normal files and not symlinks.
This CL adjusts os.Stat implementation to return normal file
os.FileInfo
for AppExecLinks files instead of symlinks. The AppExecLinks files are
recognised as they return ERROR_CANT_ACCESS_FILE from CreateFile call.
The trick is not documented anywhere. Jan De Dobbeleer discovered the
trick. Also https://github.com/dotnet/runtime/pull/58233 appears to
also
use ERROR_CANT_ACCESS_FILE to distinguish AppExecLinks files.
The CL also adds new tests.
The CL is an extended copy of the Jan De Dobbeleer
https://go-review.googlesource.com/c/go/+/384160 CL.
Fixes #42919
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 48f6c1d489a038983f4f4e90040d5cb921110604
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/syscall_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/os/types_windows.go
4 files changed, 122 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
1 comment:
File src/os/os_windows_test.go:
done!
Well done.
Let's continue with your CL.
I will give your CL +2. But we have to wait for Go source tree to open for Go 1.21 development to submit this.
You don't have to do anything unless other reviews.
Thank you.
Alex
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
Patch set 25:Run-TryBot +1Code-Review +2
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
1 comment:
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()This call will fail if python3.exe is not installed but the reparse point exists.
The error code is 90009 and the message looks like:
> "Python was not found; run without arguments to install from the Microsoft Store, or disable this shortcut from Settings > Manage App Execution Aliases.\r\n"
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
1 comment:
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()This call will fail if python3.exe is not installed but the reparse point exists. […]
The test should be skipped if thid case is detected.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
Patch set 25:Hold +1
6 comments:
Patchset:
Setting Hold+1 until the tree reopens for Go 1.21.
I also re-reviewed the code (since a lot has changed since I last reviewed it at PatchSet 22) and added a few (minor) comments.
File src/os/os_windows_test.go:
t.Logf("lfi=%+v", lfi)
t.Logf("sfi=%+v", sfi)(https://go.dev/wiki/CodeReviewComments#useful-test-failures)
A reader of the test log won't know what `lfi` and `sfi` are here.
Either log that information too, or update the `Logf` lines here to be independent of the variable names.Perhaps:
```
t.Logf("os.Lstat(%q) = %+v", pythonPath, lfi)
t.Logf("os.Stat(%q) = %+v", pythonPath, sfi)
```Patch Set #25, Line 1298:
t.Errorf("files should be same")More detail in this failure message and/or a comment in the code would be helpful.
(*Why* do we expect these to be the same file? How do we know that `pythonPath` isn't an ordinary symlink?)
Perhaps the `SameFile` check should go after the individual `Mode()` checks, so that the `Mode` output may explain a later `SameFile` failure.
Patch Set #25, Line 1304:
if m := lfi.Mode(); m&fs.ModeSymlink != 0 {A comment here would be helpful: how do we know that `pythonPath` will not be a symlink?
File src/os/stat_windows.go:
Patch Set #24, Line 105:
createFileAttrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINT
Validated again and added a comment.
Is there anything remaining to be done for this? (If not, please mark the comment thread as resolved.)
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.We can't guarantee that this external link will always be valid or accessible. (What if someone is debugging this test on an airplane?)
The reasoning should be summarized locally in the code.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
Patch set 25:Code-Review +1
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
5 comments:
File src/os/os_windows_test.go:
t.Logf("lfi=%+v", lfi)
t.Logf("sfi=%+v", sfi)
Patch Set #25, Line 1298:
t.Errorf("files should be same")
More detail in this failure message and/or a comment in the code would be helpful. […]
Done
Patch Set #25, Line 1304:
if m := lfi.Mode(); m&fs.ModeSymlink != 0 {A comment here would be helpful: how do we know that `pythonPath` will not be a symlink?
Done
File src/os/stat_windows.go:
Patch Set #24, Line 105:
createFileAttrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINT
Is there anything remaining to be done for this? (If not, please mark the comment thread as resolved […]
Done
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.
We can't guarantee that this external link will always be valid or accessible. […]
Done
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Gerrit Bot uploaded patch set #26 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Alex Brainman, TryBot-Result+1 by Gopher Robot
os: add support for AppExecLinks
GitHub-Last-Rev: 1b731b3e5f1419657ef6b001873fbd077334172e
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/syscall_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/os/types_windows.go
4 files changed, 131 insertions(+), 0 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Patch set 26:Code-Review +1Hold +1
6 comments:
File src/os/os_windows_test.go:
Patch Set #26, Line 1295:
AppLinkSymlinks/AppLinkSymlink/AppExecLink/ ?
Patch Set #26, Line 1307:
indentifys/indentify/identify/
File src/os/stat_windows.go:
Patch Set #26, Line 72:
We must be dealing with APPEXECLINKHow do we know that only APPEXECLINK reparse points will trigger `ERROR_CANT_ACCESS_FILE`?
Can other kinds of reparse points also trigger it? If so, that seems fine, but the code comment would be misleading.
What about if `GetFileAttributeEx` returned a non-nil error? Then it seems like we don't even know whether the path refers to a reparse point...
How do we know that `fa` is valid here?
(What happens if `GetFileAttributeEx` returns a non-nil error other than `ERROR_SHARING_VIOLATION` and `CreateFile` returns `ERROR_CANT_ACCESS_FILE`?)
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,How do we know that only an `APPEXECLINK` can trigger this error?
- Might it also apply to other reparse points?
- If the call to `CreateFile` with `FILE_FLAG_OPEN_REPARSE_POINT` set also fails, should we return the error from the original `CreateFile` call, or the one with `FILE_FLAG_OPEN_REPARSE_POINT` set? (A code comment with a rationale would be helpful.)
Patch Set #26, Line 177:
attrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINTIt looks like `FILE_FLAG_OPEN_REPARSE_POINT` is already set if `fs.isSymlink()` is true — should we avoid repeating the call in that case?
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
6 comments:
File src/os/os_windows_test.go:
Patch Set #26, Line 1295:
AppLinkSymlinks/AppLinkSymlink/AppExecLink/ ?
Done
Patch Set #26, Line 1307:
indentifys/indentify/identify/
Done
File src/os/stat_windows.go:
Patch Set #26, Line 72:
We must be dealing with APPEXECLINK
How do we know that only APPEXECLINK reparse points will trigger `ERROR_CANT_ACCESS_FILE`? […]
Added that validation and created an explicit check
How do we know that `fa` is valid here? […]
Done
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
How do we know that only an `APPEXECLINK` can trigger this error? […]
This code was added by @alex.b...@gmail.com and as these do not identify as a symlink, we need to add that explicitly. `fs.isSymlink()` is false here.
Patch Set #26, Line 177:
attrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINT
It looks like `FILE_FLAG_OPEN_REPARSE_POINT` is already set if `fs. […]
This code was added by @alex.b...@gmail.com and as these do not identify as a symlink, we need to add that explicitly. `fs.isSymlink()` is false here.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Gerrit Bot uploaded patch set #27 to this change.
os: add support for AppExecLinks
GitHub-Last-Rev: f171935f26833aa6ba0ace048f29f53934546836
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/os/types_windows.go
6 files changed, 155 insertions(+), 6 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Gerrit Bot uploaded patch set #28 to this change.
GitHub-Last-Rev: a2c95ef9f681914bab54aaa22230f060961152ce
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/os/types_windows.go
6 files changed, 174 insertions(+), 6 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Gerrit Bot uploaded patch set #29 to this change.
GitHub-Last-Rev: f5543ee2d24ea02cb10d2bc7e8926ad1060119db
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/os/types_windows.go
6 files changed, 174 insertions(+), 6 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
Patch set 29:Run-TryBot +1Code-Review +2
7 comments:
Patchset:
Thank you everyone for pushing this along. See my comments below.
Jan, your new test fails on my laptop. So, please, fix it. And then I review your changes since Patch Set 25. I suggest you remove as much changes as you made since that Patch Set.
Thank you.
Alex
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()The test should be skipped if thid case is detected.
We already skip this test if os.Lstat("C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3.exe") fails. See line 1287.
I tested that skip when I had no python installed on my laptop. The skip is also tested by every Go Team builder run (I suspect there is no python appexeclink installed there).
How do we test scenario where you see error code of 90009? Is that the same as what I had before installing python?
File src/os/os_windows_test.go:
Patch Set #29, Line 1336:
func TestAppExecLinkReadlink(t *testing.T) {Is it possible to join this test with TestAppExecLinkStat ? So we have less tests to look at. And less code in the test.
Patch Set #29, Line 1346:
If it does, we should update our implementation.Line 1346 comment is confusing. Maybe just remove it.
Patch Set #29, Line 1347:
linkName, err := os.Readlink(pythonPath)This implementation is fine. I also OK if os.Readlink returns some error here.
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.
Done
Bryan asked you to copy some details from https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 and add them to this comment. But you removed the comment altogether.
And why did you add faErr?
And why did you add call to isAppExecLink? I think ERROR_CANT_ACCESS_FILE is good enough to identify APPLINK file.
I just run your tests on my laptop and they fail with:
```
=== RUN TestAppExecLinkStat
os_windows_test.go:1299: os.Lstat("C:\\Users\\user\\AppData\\Local\\Microsoft\\WindowsApps\\python3.exe") = &{name:python3.exe FileAttributes:1056 CreationTime:{LowDateTime:1510765494 HighDateTime:30857418} LastAccessTime:{LowDateTime:699192412 HighDateTime:30993647} LastWriteTime:{LowDateTime:699192412 HighDateTime:30993647} FileSizeHigh:0 FileSizeLow:0 Reserved0:2147483675 filetype:1 Mutex:{state:0 sema:0} path: vol:4030669601 idxhi:196608 idxlo:9214 appendNameToPath:false}
os_windows_test.go:1300: os.Stat("C:\\Users\\user\\AppData\\Local\\Microsoft\\WindowsApps\\python3.exe") = &{name:python3.exe FileAttributes:1056 CreationTime:{LowDateTime:1510765494 HighDateTime:30857418} LastAccessTime:{LowDateTime:699192412 HighDateTime:30993647} LastWriteTime:{LowDateTime:699192412 HighDateTime:30993647} FileSizeHigh:0 FileSizeLow:0 Reserved0:0 filetype:0 Mutex:{state:0 sema:0} path:C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3.exe vol:0 idxhi:0 idxlo:0 appendNameToPath:false}
os_windows_test.go:1301: files should be same
--- FAIL: TestAppExecLinkStat (0.59s)
=== RUN TestAppExecLinkReadlink
--- PASS: TestAppExecLinkReadlink (0.00s)
```
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
How do we know that only an APPEXECLINK can trigger this error?
Might it also apply to other reparse points?
I don't know that APPEXECLINK triggers ERROR_CANT_ACCESS_FILE.
Jan discovered that.
And I confirmed his discovery on my laptop.
And then I googled for it and I found https://github.com/dotnet/runtime/pull/58233/files (search this PR for ERROR_CANT_ACCESS_FILE).
And I also asked @quimm...@gmail.com to confirm or deny our discovery. But Quim did not comment on that yet.
If the call to CreateFile with FILE_FLAG_OPEN_REPARSE_POINT set also fails, should we return the error from the original CreateFile call, or the one with FILE_FLAG_OPEN_REPARSE_POINT set? (A code comment with a rationale would be helpful.)
I don't understand what you are referring to here. Please try again.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
5 comments:
Patchset:
Thank you everyone for pushing this along. See my comments below. […]
Why do I need to remove? I addressed @bcm...@google.com 's comments with these changes. I'll reply inline.
File src/os/os_windows_test.go:
Patch Set #29, Line 1336:
func TestAppExecLinkReadlink(t *testing.T) {
Is it possible to join this test with TestAppExecLinkStat ? So we have less tests to look at. […]
Sure
Patch Set #29, Line 1346:
If it does, we should update our implementation.Line 1346 comment is confusing. Maybe just remove it.
Ack
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.
you removed the comment
I did not remove that, I moved it to `readlink`.
And why did you add faErr?
Because of @bcm...@google.com 's remark about it being possible that `fa` is not valid so we need to double check that error as well before we deal with `fa` blindly.
I think ERROR_CANT_ACCESS_FILE is good enough to identify APPLINK file.
There @bcm...@google.com seemed to disagree, this way at least it will always be an actual AppExecLink
If this isn't OK can you two please agree offline first, then provide feedback as I'm going in circles here. It's really demotivating to address comments then a week later having to come back to this.
I just run your tests on my laptop and they fail
This is your test, I did not change this one 😊
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
@bcm...@google.com […]
That's why I added `faErr`
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
Gerrit Bot uploaded patch set #30 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Alex Brainman, TryBot-Result-1 by Gopher Robot
os: add support for AppExecLinks
This change adds support for AppExecLinks files, like
C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3.exe
The python3.exe can be installed by following these
https://www.microsoft.com/store/productId/9PJPW5LDXLZ5
instructions. The executable is added to your PATH and can be called
from command line, like `python3 --version`.
Calling GetFileAttributesEx on python3.exe returns FileAttributes with
FILE_ATTRIBUTE_REPARSE_POINT set. And os.Stat attempts to follow the
link. But Microsoft does not provide any link target for AppExecLinks
files (see
https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 ),
and Go should treat AppExecLinks as normal files and not symlinks.
This CL adjusts os.Stat implementation to return normal file
os.FileInfo
for AppExecLinks files instead of symlinks. The AppExecLinks files are
recognised as they return ERROR_CANT_ACCESS_FILE from CreateFile call.
The trick is not documented anywhere. Jan De Dobbeleer discovered the
trick. Also https://github.com/dotnet/runtime/pull/58233 appears to
also
use ERROR_CANT_ACCESS_FILE to distinguish AppExecLinks files.
The CL also adds new tests.
The CL is an extended copy of the Jan De Dobbeleer
https://go-review.googlesource.com/c/go/+/384160 CL.
Fixes #42919
Change-Id: I5a0a72eaba2a5b9c242b7ac32dd343fcc01e09e6
GitHub-Last-Rev: 1fee03151d26b9e9682053bfaa7088f0ffaa6e09
GitHub-Pull-Request: golang/go#51081
---
M src/internal/syscall/windows/reparse_windows.go
M src/internal/syscall/windows/syscall_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/os/types_windows.go
6 files changed, 162 insertions(+), 6 deletions(-)
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
Patch set 30:Code-Review +2
4 comments:
Patchset:
Why do I need to remove? I addressed @bcm...@google.com 's comments with these changes. […]
I asked you to make sure new test PASSes on you laptop. And mine.
File src/os/os_windows_test.go:
Patch Set #29, Line 1298:
if !os.SameFile(lfi, sfi) {Why did you removed os.SameFile test? Please revert the change. We want os.SameFile working properly. No?
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.you removed the comment
I did not remove that, I moved it to `readlink`.
And why did you add faErr?
Because of @bcm...@google.com 's remark about it being possible that `fa` is not valid so we need to double check that error as well before we deal with `fa` blindly.
Are you referring to https://go-review.googlesource.com/c/go/+/384160/23..30/src/os/stat_windows.go#b98 ? If yes, then that question was asked back against Patch Set 23. I completely rewrote whole code in Patch Set 25. Bryan gave +1 to Patch Set 25.
I think ERROR_CANT_ACCESS_FILE is good enough to identify APPLINK file.
There @bcm...@google.com seemed to disagree, this way at least it will always be an actual AppExecLink.
We don't actually know which code is correct, because Microsoft did not document anything. So I prefer less code. As long as our test PASSes I am good.
If this isn't OK can you two please agree offline first, then provide feedback as I'm going in circles here. It's really demotivating to address comments then a week later having to come back to this.
I am happy to wait for Bryan to confirm if he wants more changes on top of +1 he gave at Patch Set 25.
I agree that this CL is too long. But we did not know how to fix this problem at the start. We still can change if we discover more details here.
I warned you that this change is large. And we don't want to break any of existing code. And I expect you will need to rebase this code before it gets merged.
And reviewers cannot agree offline. We don't discuss this CL offline. This CL is where we all discuss and make decisions.
And I only have so much time to spend on your CL. I do this for free on top of everything I have to do in my life. I need to share my time before this CL and many other CLs and issues.
But if other reviewers give you +2, then you don't need to wait for my review. The only problem for them is that they don't really run the test.
> I just run your tests on my laptop and they failThis is your test, I did not change this one 😊
We wrote the test together. You started and I added more tests to it.
But I expect you to run that test on your laptop before you post any changes here. That is the only way to verify your changes.
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
That's why I added `faErr`
Jan. Let's wait for Bryan to reply before continuing this thread.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
3 comments:
Patchset:
This sentence confused me I guess:
I suggest you remove as much changes as you made since that Patch Set.
File src/os/os_windows_test.go:
Patch Set #29, Line 1298:
if !os.SameFile(lfi, sfi) {
Why did you removed os.SameFile test? Please revert the change. We want os. […]
That's the one which caused things to fail, but I did not toch that part. It still returned the "same" file buit some properties were different for some reason.
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.> > you removed the comment […]
I'll try to understand why that test now fails, BTW I always rebase before I push so that should be good.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Jan De Dobbeleer (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Russ Cox, Sachin Joseph.
1 comment:
File src/os/os_windows_test.go:
Patch Set #29, Line 1298:
if !os.SameFile(lfi, sfi) {
That's the one which caused things to fail, but I did not toch that part. […]
@alex.b...@gmail.com I don't understand how this test succeeded on your end to begin with. It fails because `Reserved0` and other elements which weren't ever set in Lstat are different. This wasn't touched by me unless some recent changes in master are causing this.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
2 comments:
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()
We already skip this test if os.Lstat("C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3. […]
Hmmm, that lstat does not skip the test on my computer. I'm afk for a week, will check what the lstat returns when I go back to work.
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
Jan. Let's wait for Bryan to reply before continuing this thread.
Will investigate this next week.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Quim Muntal, Russ Cox, Sachin Joseph.
Patch set 30:-Code-Review
5 comments:
Patchset:
This sentence confused me I guess: […]
You copied my changes as Patch Set 25. And new test PASSed back then. And now test is broken. That is why I suggested that you remove the changes you made since Patch Set 25. To make test work again. I hope my suggestion is clear this time.
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()
Hmmm, that lstat does not skip the test on my computer. […]
Thanks for checking. No worries we will wait until you are back to work. We need to wait for Go tree to be opened anyway.
File src/os/os_windows_test.go:
Patch Set #29, Line 1298:
if !os.SameFile(lfi, sfi) {
@alex.b...@gmail.com I don't understand how this test succeeded on your end to begin with. […]
Try running new test against https://go-review.googlesource.com/c/go/+/452375 . The test PASSed for me. Hopefully it PASSes for you too.
File src/os/stat_windows.go:
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.
I'll try to understand why that test now fails, BTW I always rebase before I push so that should be […]
Don't waste time rebasing your changes. You are making it harder for yourself and everyone else to develop this CL. Let's make the CL work and approved on that particular commit, and then we can rebase it before submitting.
I know that there are approved changes in that code already. These changes will be merged before your changes. So you would have to rebase regardless of what you do now. So delay that rebase until everyone is happy.
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
Will investigate this next week.
Quim sounds good.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Russ Cox, Sachin Joseph.
4 comments:
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()
Thanks for checking. No worries we will wait until you are back to work. […]
I'm getting the error code 90009 when `C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3.exe` reparse point exists (as a 0-length file) but python3 is still not installed from the Windows Store, so running `python3.exe` launches the Windows Store instead of running the python interpreter.
File src/os/stat_windows.go:
Patch Set #30, Line 71:
if faErr == nil && err == windows.ERROR_CANT_ACCESS_FILE {This condition can be tightened by checking that the target file is a reparse point and that we did not call `CreateFile` with `FILE_FLAG_OPEN_REPARSE_POINT`, i.e.:
- fa.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT != 0
- createFileAttrs&syscall.FILE_FLAG_OPEN_REPARSE_POINT == 0
`isAppExecLink` is not cheap, so we better reduce the false positive rate.
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
Quim sounds good.
I've added a couple of comments about `ERROR_CANT_ACCESS_FILE` next to the relevant code sections.
File src/os/types_windows.go:
Patch Set #30, Line 173:
if err == windows.ERROR_CANT_ACCESS_FILE {`CreateFile` will not fail with `ERROR_CANT_ACCESS_FILE` if attrs contains `FILE_FLAG_OPEN_REPARSE_POINT`, so the `isAppExecLink` can be avoided by checking if `fs.Reserved0 == windows.IO_REPARSE_TAG_APPEXECLINK` in addition to `fs.isSymlink()` in the previous if-case. @bcm...@google.com @alex.b...@gmail.com I wonder if we should update `isSymlink` to account for this additional case.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Quim Muntal, Russ Cox, Sachin Joseph.
Patch set 30:Code-Review +1Hold +1
9 comments:
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()
I'm getting the error code 90009 when `C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3. […]
The error reported in the issue most likely comes from `os/exec.findExecutable`. If that is the case, it should suffice to check that `exec.LookPath` resolves it to itself — we shouldn't need to actually run it.
File src/os/stat_windows.go:
Patch Set #25, Line 71:
if err == windows.ERROR_CANT_ACCESS_FILE {This seems backwards: we are calling `CreateFile` without `FILE_FLAG_OPEN_REPARSE_POINT` as if resolving a symlink target, but at this point we don't even know that the file is a symlink. (All that we know about it is that it is a reparse point.)
So we should start by obtaining the reparse tag (using either `FindFirstFile`, or `CreateFile` with `FILE_FLAG_OPEN_REPARSE_POINT` set), and only then try `CreateFile` without the flag set.
The first `CreateFile` call will not fail with `ERROR_CANT_ACCESS_FILE`, and will report an `APPEXECLINK` as a non-symlink (because its reparse tag does not indicate a symlink). Then we don't need any special cases for `APPEXECLINK` — the correct behavior follows from the general case instead.
Patch Set #25, Line 75:
// See https://github.com/dotnet/runtime/pull/58233#issuecomment-911971904 for reasoning.
We don't actually know which code is correct, because Microsoft did not document anything.
FWIW, I believe the relevant documentation is here:
So I prefer less code. As long as our test PASSes I am good.
All else equal, I too prefer less code over more. But I also have a very hard time believing that an error code as old and general as `ERROR_CANT_ACCESS_FILE` is specific to a feature as new as `IO_REPARSE_TAG_APPEXECLINK`.
(The TDD approach of “write the minimal code that passes tests” relies on having tests that cover all of the behaviors that users realistically require, but the rate of Windows issues we get in the Go issue tracker suggests to me that our tests do not yet meet that very high bar.)
File src/os/stat_windows.go:
Patch Set #30, Line 71:
if faErr == nil && err == windows.ERROR_CANT_ACCESS_FILE {
This condition can be tightened by checking that the target file is a reparse point and that we did […]
`stat` has only two call sites, and they both pass hard-coded attrs for the `createFileAttrs` argument.
- The call in `statNoLog` passes `FILE_FLAG_BACKUP_SEMANTICS`.
- The call in `lstatNoLog` passes `FILE_FLAG_BACKUP_SEMANTICS|FILE_FLAG_OPEN_REPARSE_POINT`.
I suggest that we simplify this function by changing the `createFileAttrs` argument to `followSymlinks bool`, which will make it easier to reason locally about this function:
```
createFileAttrs := uint32(syscall.FILE_FLAG_BACKUP_SEMANTICS)
if !followSymlinks {
// Open the symlink itself, not its target.
createFileAttrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINT
}
```
File src/os/types_windows.go:
if fs.isSymlink() {
// Use FILE_FLAG_OPEN_REPARSE_POINT, otherwise CreateFile will follow symlink.
// See https://docs.microsoft.com/en-us/windows/desktop/FileIO/symbolic-link-effects-on-file-systems-functions#createfile-and-createfiletransacted
attrs |= syscall.FILE_FLAG_OPEN_REPARSE_POINT
}As far as I can tell we should be setting `FILE_FLAG_OPEN_REPARSE_POINT` unconditionally.
If the file is a symlink, we never reach this point: `fs.path` is only set to a non-empty string in the `saveInfoFromPath` method, and that method is only called in two places:
1. On the non-`REPARSE_POINT` path, which implies a non-symlink.
2. On the `ERROR_SHARING_VIOLATION` path, which seems to imply a system file, and which I've been unable to reproduce for symlinks (see https://go-review.git.corp.google.com/c/go/+/460595/1#message-dc70bdea9dd2ba1217844b7f895dc663c7617ca4).
So reaching this point in the code implies a non-symlink, and for a non-symlink we should report information about the reparse point itself, not the link target — which means that we should follow the advice in https://learn.microsoft.com/en-us/windows/win32/fileio/reparse-points-and-file-operations and set `FILE_FLAG_OPEN_REPARSE_POINT`.
File src/os/types_windows.go:
Patch Set #26, Line 174:
We must be dealing with APPEXECLINK,
The documentation in https://learn.microsoft.com/en-us/windows/win32/fileio/reparse-points-and-file-operations says:
Applications that use the `CreateFile` function should specify the `FILE_FLAG_OPEN_REPARSE_POINT` flag when opening the file if it is a reparse point.
And https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew says:
[`FILE_FLAG_OPEN_REPARSE_POINT`] cannot be used with the `CREATE_ALWAYS` flag.
If the file is not a reparse point, then this flag is ignored.If we follow that advice, that suggests to me a different (and simpler?) approach: perhaps when were are calling `CreateFile` in a `stat`-like setting we should default to setting `FILE_FLAG_OPEN_REPARSE_POINT`, instead of defaulting to leaving it unset.
I see [five existing non-test calls](https://cs.opensource.google/search?q=%5CbCreateFile%5Cb%20filepath:src%2F%20-filepath:_test.go%20-filepath:src%2Fcmd&ss=go%2Fgo) to `CreateFile` in the standard library.
- The call in [`os.openSymlink`](https://cs.opensource.google/go/go/+/master:src/os/file_windows.go;l=400-402;drc=0f0aa5d8a6a0253627d58b3aa083b24a1091933f) already sets `FILE_FLAG_OPEN_REPARSE_POINT` unconditionally.
- The two calls in [`syscall.Open`](https://cs.opensource.google/go/go/+/master:src/syscall/syscall_windows.go;l=351-378;drc=0f0aa5d8a6a0253627d58b3aa083b24a1091933f) are intended to open the data from the underlying file, not the metadata stored in the reparse point.
- The calls in [`(*os.fileStat).loadFileId`](https://cs.opensource.google/go/go/+/master:src/os/types_windows.go;l=167-171;drc=0f0aa5d8a6a0253627d58b3aa083b24a1091933f) and [`os.stat`](https://cs.opensource.google/go/go/+/master:src/os/stat_windows.go;l=67-69;drc=0f0aa5d8a6a0253627d58b3aa083b24a1091933f) are both already being updated in this CL. I will leave more detailed comments on those specific files.
I have drafted CL 460595 with that approach.
File src/os/types_windows.go:
Patch Set #30, Line 93:
Reserved0: d.Reserved0,Per https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-win32_find_dataw:
If the `dwFileAttributes` member includes the `FILE_ATTRIBUTE_REPARSE_POINT` attribute, this member specifies the reparse point tag.
> Otherwise, this value is undefined and should not be used.So probably we need an extra condition here somewhere:
```
fs := &fileStat{
FileAttributes: d.FileAttributes,
CreationTime: d.CreationTime,
LastAccessTime: d.LastAccessTime,
LastWriteTime: d.LastWriteTime,
FileSizeHigh: d.FileSizeHigh,
FileSizeLow: d.FileSizeLow,
Reserved0: d.Reserved0,
}
```Patch Set #30, Line 99:
// https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/(FYI) The link in this comment is dead. I believe it is now https://devblogs.microsoft.com/oldnewthing/20100212-00/?p=14963.
Patch Set #30, Line 152:
if fs.path == "" {Doing some code archaeology:
`fileStat.path` is set [only in `(*fileStat).saveInfoFromPath`](https://cs.opensource.google/search?q=filepath:src%2Fos%20filepath:_windows.go%20%5C.path%5Cb%5C%20%3D%20case:yes&ss=go%2Fgo:src%2Fos%2F).
`saveInfoFromPath` is itself called in only [two places]( https://cs.opensource.google/search?q=%5C.saveInfoFromPath&ss=go%2Fgo:src%2Fos%2F&ssfr=1), both within the `stat` function.
- Notably, `newFileStatFromGetFileInformationByHandle` leaves `path` empty, because it fills in `vol`, `idxhi`, and `idxlo` itself.[One of the calls](https://cs.opensource.google/go/go/+/master:src/os/stat_windows.go;l=46;drc=0f0aa5d8a6a0253627d58b3aa083b24a1091933f) to `saveInfoFromPath` is guarded by `fa.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT == 0`, in which case `FILE_FLAG_OPEN_REPARSE_POINT` would have no effect either way.
[The other call](https://cs.opensource.google/go/go/+/master:src/os/stat_windows.go;l=61;drc=0f0aa5d8a6a0253627d58b3aa083b24a1091933f) is guarded by `err == windows.ERROR_SHARING_VIOLATION`, and creates the `fileStat` using `newFileStatFromWin32finddata` without checking whether it is a reparse point, but in gomote testing I am unable to provo
The latter appears to be the only way we could possibly get past this `fs.path == ""` check with a `fileStat` that represents a reparse point, but in testing with a gomote I don't think that's even possible.
―
That is to say: with a bit more care in the early returns from `stat`, I think we can have the invariant that
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Jan De Dobbeleer, Quim Muntal, Russ Cox, Sachin Joseph.
2 comments:
File src/os/stat_windows.go:
Patch Set #30, Line 71:
if faErr == nil && err == windows.ERROR_CANT_ACCESS_FILE {
`stat` has only two call sites, and they both pass hard-coded attrs for the `createFileAttrs` argume […]
The `followSymlinks` behavior turns out to be not quite as simple as setting the `createFileAttrs` bit, but still IMO much simpler than passing in `createFileAttrs` as an argument.
(See CL 460595.)
File src/os/types_windows.go:
Patch Set #30, Line 99:
// https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/(FYI) The link in this comment is dead. I believe it is now https://devblogs.microsoft. […]
Ack
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Bryan Mills, Ian Lance Taylor, Jan De Dobbeleer, Quim Muntal, Russ Cox, Sachin Joseph.
2 comments:
Patchset:
I think that Bryan come up with much better solution for issue #42919 in CL 460595. We should proceed with CL 460595 instead of this CL.
Alex
File src/os/os_windows_test.go:
Patch Set #25, Line 1322:
output, err := cmd.CombinedOutput()
I'm getting the error code 90009 when C:\Users\user\AppData\Local\Microsoft\WindowsApps\python3.exe reparse point exists (as a 0-length file) but python3 is still not installed from the Windows Store ...
Is there a way for me reproduce error with code 90009 here?
I can find another computer without python installed on it from Windows Store. What would I need to do?
Just wondering if we can extend our test with error code 90009.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor abandoned this change.
To view, visit change 384160. To unsubscribe, or for help writing mail filters, visit settings.