Navigation Menu

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

Jump to bottom

x/tools/txtar: Parse does not cope with CR-LF line endings #59264

Open
rogpeppe opened this issue Mar 27, 2023 · 5 comments · May be fixed by golang/tools#435
Open

x/tools/txtar: Parse does not cope with CR-LF line endings #59264

rogpeppe opened this issue Mar 27, 2023 · 5 comments · May be fixed by golang/tools#435
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@rogpeppe
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version devel go1.21-a4d5fbc3a4 Thu Feb 9 21:14:00 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rogpeppe/.cache/go-build"
GOENV="/home/rogpeppe/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/rogpeppe/src/go/pkg/mod"
GONOPROXY="github.com/cue-unity"
GONOSUMDB="github.com/cue-unity"
GOOS="linux"
GOPATH="/home/rogpeppe/src/go"
GOPRIVATE="github.com/cue-unity"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/rogpeppe/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/rogpeppe/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.21-a4d5fbc3a4 Thu Feb 9 21:14:00 2023 +0000"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1790663412=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran this testscript test:

exec go mod tidy
exec go run main.go
cmp stdout expect-stdout
-- go.mod --
module m

go 1.20

require golang.org/x/tools v0.7.0
-- main.go --

package main

import (
	"fmt"

	"golang.org/x/tools/txtar"
)

func main() {
	ar := txtar.Parse([]byte("comment\r\n-- file --\r\ndata\r\n"))
	fmt.Printf("comment: %q\n", ar.Comment)
	for _, f := range ar.Files {
		fmt.Printf("file %q: %q\n", f.Name, f.Data)
	}
}
-- expect-stdout --
comment: "comment\r\n"
file: "file": "data\r\n"

What did you expect to see?

A passing test.

What did you see instead?

> exec go mod tidy
> exec go run main.go
[stdout]
comment: "comment\r\n-- file --\r\ndata\r\n"
> cmp stdout expect-stdout
--- stdout
+++ expect-stdout
@@ -1,1 +0,0 @@
-comment: "comment\r\n-- file --\r\ndata\r\n"
@@ -0,0 +1,2 @@
+comment: "comment\r\n"
+file: "file": "data\r\n"

FAIL: /tmp/testscript86563551/x.txtar/script.txtar:3: stdout and expect-stdout differ

The logic inside the txtar package does not properly recognise CR-LF files.
It's an open question whether the logic should normalize \r\n to \n.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 27, 2023
@gopherbot gopherbot added this to the Unreleased milestone Mar 27, 2023
rogpeppe added a commit to rogpeppe/go-internal that referenced this issue Mar 27, 2023
@rogpeppe
txtar: alias types to x/tools
84092a1 
Although we can't quite use the x/tools/txtar implementation in its
entirety (golang/go#59264 needs
to be fixed first, and we implement some other functions that
x/tools/txtar doesn't (yet?) have, we can at least alias its
types, which makes it possible to work with other packages
that use those types.
@rogpeppe rogpeppe mentioned this issue Mar 27, 2023
Merged
rogpeppe added a commit to rogpeppe/go-internal that referenced this issue Mar 27, 2023
@rogpeppe
txtar: alias types to x/tools
9a713f0 
Although we can't quite use the x/tools/txtar implementation in its
entirety (golang/go#59264 needs
to be fixed first, and we implement some other functions that
x/tools/txtar doesn't (yet?) have, we can at least alias its
types, which makes it possible to work with other packages
that use those types.
rogpeppe added a commit to rogpeppe/go-internal that referenced this issue Mar 27, 2023
@rogpeppe
txtar: alias types to x/tools (#209)
ca7ccbd 
Although we can't quite use the x/tools/txtar implementation in its
entirety (golang/go#59264 needs
to be fixed first, and we implement some other functions that
x/tools/txtar doesn't (yet?) have, we can at least alias its
types, which makes it possible to work with other packages
that use those types.
@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 27, 2023
@bcmills
Copy link
Contributor

bcmills commented Apr 3, 2023

The logic inside the txtar package does not properly recognise CR-LF files.

Agreed, that should probably be fixed.

It's an open question whether the logic should normalize \r\n to \n.

I would say that it should not.

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Apr 3, 2023
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 3, 2023
@bsponge
Copy link

bsponge commented Apr 6, 2023 •
edited

@bcmills I would like to get on with it

@bsponge
Copy link

bsponge commented Apr 6, 2023 •
edited

Should parsed structure be formated back to string using \n?
example

Archive{
                                Comment: []byte("comment1\r\ncomment2\r\n"),
                                Files: []File{
                                        {"file1", []byte("File 1 text.\r\n-- foo ---\r\nMore file 1 text.\r\n")},
                                        {"file 2", []byte("File 2 text.\r\n")},
                                        {"empty", []byte{}},
                                        {"noNL", []byte("hello world")},
                                },
}

should be formated to

"comment1\r\n" +
                                "comment2\r\n" +
                                "-- file1 --\n" +
                                "File 1 text.\r\n" +
                                "-- foo ---\r\n" +
                                "More file 1 text.\r\n" +
                                "-- file 2 --\n" +
                                "File 2 text.\r\n" +
                                "-- empty --\n" +
                                "-- noNL --\n" +
                                "hello world\n"

or

"comment1\n" +
                                "comment2\n" +
                                "-- file1 --\n" +
                                "File 1 text.\n" +
                                "-- foo ---\n" +
                                "More file 1 text.\n" +
                                "-- file 2 --\n" +
                                "File 2 text.\n" +
                                "-- empty --\n" +
                                "-- noNL --\n" +
                                "hello world\n"

or even version with only \r\n but that would require a prior scan of data stored in the structure

@bcmills
Copy link
Contributor

bcmills commented Apr 7, 2023

Good question. Perhaps add a UseCRLF field to the Archive struct, analogous to the same field on the csv.Writer struct?

@bsponge bsponge linked a pull request Apr 8, 2023 that will close this issue
Open
@gopherbot
Copy link

Change https://go.dev/cl/483335 mentions this issue: txtar: add CRLF handling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

txtar: add CRLF handling bsponge/tools
5 participants
@mdempsky @rogpeppe @bcmills @gopherbot @bsponge