Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: use '/' instead of backslash in go.work on Windows for consistency with Unix systems #64851

Open
Nasfame opened this issue Dec 23, 2023 · 19 comments · May be fixed by #64965
Open

cmd/go: use '/' instead of backslash in go.work on Windows for consistency with Unix systems #64851

Nasfame opened this issue Dec 23, 2023 · 19 comments · May be fixed by #64965
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Nasfame
Copy link
Contributor

Nasfame commented Dec 23, 2023

Go version

go 1.21.5

What operating system and processor architecture are you using (go env)?

set GO111MODULE=
set GOARCH=amd64
set GOBIN=N:\Peeves\go-bin
set GOCACHE=n:\peeves\go-cache
set GOENV=C:\Users\Hiro\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=N:\peeves\go-path\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=N:\peeves\go-path
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\Hiro\.go\current
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Users\Hiro\.go\current\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.21.5
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=N:\Peeves\Temp\go-build2388527693=/tmp/go-build -gno-record-gcc-switches

What did you do?

look at the terminal section:

image

ran

go work use ../tools/updater

What did you see instead?

go 1.21

toolchain go1.21.5

use (
	.
	..\tools\updater
)

What did you expect to see?

go 1.21

toolchain go1.21.5

use (
	.
	../tools/updater
)

Solution

Prefer / for relative paths across all platforms for portability while maintaining the respective platform specific path separators for absolute paths.

Additional Info

Related to https://youtrack.jetbrains.com/issue/GO-16069/LSP-false-positive-warning-for-go-work-paths

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 23, 2023
@gopherbot gopherbot added this to the Unreleased milestone Dec 23, 2023
@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 28, 2023
@dr2chase
Copy link
Contributor

I can't quite make sense of the title paired with the expected/received examples.

Is the desired outcome

  1. go.work always use "/" as a path separator ("for consistency with unix systems")
  2. go.work uses the platform path separator (under "what did you expect to see?")

I think you want 2, since that would be the one requiring a change.

@dr2chase
Copy link
Contributor

@golang/tools-team

@findleyr
Copy link
Contributor

I agree that this probably requesting a change to platform-dependent path formatting. @Nasfame can you explain why this is desirable? Shouldn't go work use prefer the platform-independent '/' style?

@Nasfame
Copy link
Contributor Author

Nasfame commented Dec 28, 2023

@findleyr sorry for the confusions. I expected to see '/' but saw backslash prompting me to raise : https://youtrack.jetbrains.com/issue/GO-16069/LSP-false-positive-warning-for-go-work-paths

I am extremely sorry, I have updated the bug report.

CC: @dr2chase

@Nasfame
Copy link
Contributor Author

Nasfame commented Dec 28, 2023

I can't quite make sense of the title paired with the expected/received examples.

Is the desired outcome

  1. go.work always use "/" as a path separator ("for consistency with unix systems")
  2. go.work uses the platform path separator (under "what did you expect to see?")

I think you want 2, since that would be the one requiring a change.

@dr2chase I think that go work tool should use / as path separator for both unix and windows systems in support for cross-compatibility.

@matloob
Copy link
Contributor

matloob commented Jan 2, 2024

So I talked to @bcmills about this. One alternative is to detect the separator passed in and use that (but not change the behavior for for instance go work use -r .).

@Nasfame
Copy link
Contributor Author

Nasfame commented Jan 2, 2024

@matloob go.work is just a manifest. Why can't it be platform agnostic? i.e what's the harm in the standardizing the path separators in go.work with /.

@matloob
Copy link
Contributor

matloob commented Jan 3, 2024

@Nasfame We'll still need backslashes in absolute paths, for instance C:\foo\bar, so we can't always use /. The behavior change I proposed should fix the use case you reported, so that users running go work use on different platforms get the same result.

@Nasfame
Copy link
Contributor Author

Nasfame commented Jan 3, 2024

Well I m using windows 11, and it's working absolutely fine with forward slashes. Also I am not suggesting the go work should use forward slashes internally in it's logic.

@matloob
Copy link
Contributor

matloob commented Jan 4, 2024

@Nasfame I don't understand. Are you using forward slashes in absolute paths?

@bcmills
Copy link
Member

bcmills commented Jan 4, 2024

Forward slashes in absolute paths do work on modern Windows, but they're not canonical — and for absolute paths (unlike for relative paths), there is no compelling reason to prefer the (non-canonical) slashes over the canonical Windows backslashes.

(A slash-separated relative path is portable across platforms. A slash-separated absolute path is not.)

@Nasfame
Copy link
Contributor Author

Nasfame commented Jan 4, 2024

Thanks @bcmills for the clarification. I agree.

/ separated relative path should do

It doesn't make sense to use / for absolute paths for windows for the sake of portability!!!

Nasfame added a commit to golangFame/go that referenced this issue Jan 4, 2024
@gopherbot
Copy link

Change https://go.dev/cl/553936 mentions this issue: gowork: use forward slash for relative paths across all platforms

@matloob
Copy link
Contributor

matloob commented Jan 8, 2024

I still think that we should go with the option of using forward or back slashes depending on whether the arguments to go work use have forward or back slashes.

@Nasfame
Copy link
Contributor Author

Nasfame commented Jan 8, 2024

@matloob, I'm confident that the majority of developers aren't using the ..\..\ approach. Instead, the / slash-formatted relative paths are more widely adopted for their portability. I believe this serves as a strong argument in favor of using forward slashes over backslashes.

If a developer insists on employing \ formatted relative paths, they can manually update it in the go.work file. However, it's crucial to note that the Go work tool is designed to generate portable workspaces, emphasizing the importance of maintaining consistency with forward slashes for broader compatibility and ease of use.

@bcmills
Copy link
Member

bcmills commented Jan 9, 2024

@Nasfame, we want to avoid gratuitous churn in the actions of go commands such as go work use. If the user provides an explicit form of slash in the argument to the command (as in go work use -r ./), it seems fine to use that explicit form as a signal as to which form they want to use. However, without an explicit signal we should bias in the direction of preserving existing behaviors.

@Nasfame
Copy link
Contributor Author

Nasfame commented Jan 10, 2024

@bcmills When I run the command go work use -r .\, I expected to get go.work formatted with backslash...

image

PS N:\goferHiro\gotcha> go env GOROOT
C:/Users/Hiro/.go/go1.21.5
PS N:\goferHiro\gotcha> go work use -r .\
PS N:\goferHiro\gotcha> cat go.work
go 1.21.5

use (
        .
        ./examples/basic
)

@bcmills But same is not true with ..\,where the bias is towards the platform-dependent slash.

image

@Nasfame
Copy link
Contributor Author

Nasfame commented Jan 10, 2024

@Nasfame, we want to avoid gratuitous churn in the actions of go commands such as go work use. If the user provides an explicit form of slash in the argument to the command (as in go work use -r ./), it seems fine to use that explicit form as a signal as to which form they want to use.

Well what if a project involves devs working in windows and linux? The windows dev could use .\ formatted rel paths while the unix devs would use ./ formatted rel paths in the go work use.

The portability of the workspace is compromised!!!. Which is why I emphasize that the go.work manifest should use / as the standard for relative paths.

However, without an explicit signal we should bias in the direction of preserving existing behaviors.

@bcmills Its not very clear to me what the existing behaviors are. Ref: #64851 (comment)

@bcmills bcmills changed the title x/tools/gowork: use '/' instead of backslash in windows for consistency with unix systems cmd/go: use '/' instead of backslash in go.work on Windows for consistency with Unix systems Jan 26, 2024
@bcmills bcmills added GoCommand cmd/go and removed Tools This label describes issues relating to any tools in the x/tools repository. labels Jan 26, 2024
@bcmills
Copy link
Member

bcmills commented Jan 26, 2024

@bcmills Its not very clear to me what the existing behaviors are. Ref: #64851 (comment)

That's why it's usually a good idea to start with a regression test. 🙃

The go command's regression tests are found in src/cmd/go/testdata/script; see the README file there for script syntax. This search can point you toward some of the existing tests of go work use as examples:
https://cs.opensource.google/search?q=file:testdata%2Fscript%20%22go%20work%20use%22&sq=&ss=go%2Fgo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants