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/compile: 1.5 beta 2 can't handle CRLF terminated go system source #11771

Closed
pbennettinin opened this issue Jul 18, 2015 · 9 comments
Closed

Comments

@pbennettinin
Copy link

The easiest way to reproduce is to change all the .go files in go/src/ (just go/src/runtime is fine) to be CRLF terminated instead of LF terminated. Since we check our build tools into source control, this happens as soon as a developer on a Windows machine fetches the tools.
Upon building something that makes go install want to rebuild system libs, you'll get errors like this:

# runtime
/usr/local/go/src/runtime/cgocall.go:144: args escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/chan.go:91: t escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/chan.go:290: c escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/chan.go:577: t escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/chan.go:627: t escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/env_posix.go:34: arg escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/env_posix.go:45: arg escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/extern.go:119: rpc escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/hashmap.go:272: t escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/hashmap.go:320: t escapes to heap, not allowed in runtime.
/usr/local/go/src/runtime/hashmap.go:320: too many errors

This looks like the same issue that was supposedly already fixed in #9281

This is not an issue in 1.4.

@bradfitz bradfitz added this to the Go1.5Maybe milestone Jul 18, 2015
@bradfitz bradfitz changed the title 1.5 beta 2 can't handle CRLF terminated go system source. cmd/compile: 1.5 beta 2 can't handle CRLF terminated go system source Jul 18, 2015
@ianlancetaylor
Copy link
Contributor

The upshot of issue #9281 was that the Go repo itself can only be built if the file use Unix-style line endings. Anything else got too complex, because of changes to test files. So, in the general case, what you are doing is not expected to work.

However, it does seem to me that the compiler should support the specific case of magic comments in files with CRLF line endings. The Go spec says that carriage returns are ignored. The Go spec does not discuss the special case of magic comments, but it seems to me that for them too carriage returns should be ignored.

@gopherbot
Copy link

CL https://golang.org/cl/12379 mentions this issue.

@pbennettinin
Copy link
Author

Any ‘text’ file WILL have different line endings depending on the platform. This is not a new phenomenon. So, any test case requiring a specific line ending in a text file (which I would argue is itself broken), should ensure that file is in the format it needs one way or the other. Whether that means it just reads an embedded string constant with escape sequences, writes that out to a file first (if using file IO is required in the test), or even translates after reading it in.

One of the tests mentioned in #9281 for eg. is just reading an 11 byte text file from disk and expecting an exact BYTE size - why? There’s no reason for this at all. Read it as a text file, correctly (Scanner and Reader.ReadLine already handle this properly) and it’s a non-issue. Worst case, it could have those 11 bytes as a local constant.

Ian, it looks like you’ve already got a diff up fixing the Go compiler itself being broken by EOL differences which is great, but technically, test cases failing because of ‘text’ file differences is broken as well.
For my immediate needs the former is really all I require but the latter staying broken would be unfortunate I think.

Thanks for all the great work in 1.5 guys…

@ianlancetaylor
Copy link
Contributor

To fix the test cases, we simply told git that every file in the Go source tree is a binary file, and that none should be treated as a text file. That makes the right thing happen for the Go source tree. Anybody who wants to copy the Go source tree out of git needs to do the same thing. That is, perhaps, slightly onerous, but it's simple.

ianlancetaylor added a commit that referenced this issue Jul 18, 2015
Update #11771.

Change-Id: I3bb3262619765d3ca79652817e17e8f260f41907
Reviewed-on: https://go-review.googlesource.com/12379
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@ianlancetaylor
Copy link
Contributor

I'm going to mark this as fixed. We aren't going to change the Go sources to support CRLF endings in the test files. Mark the test files as binary.

@pbennett
Copy link

Thanks for the fix. As for the test cases, will ALL test files be in testdata/ directories? If so, it's easy to just set up a rule for that.

@bradfitz
Copy link
Contributor

No promises. But probably. We may use a *.go file as a test data file in the future. You've chosen to do something unique, so now you get to deal with the consequences. You're using the Go tree in an unsupported (by us) way. I recommend that when you vendor the Go sources into your version control system, you mark the whole subtree as binary.

@pbennett
Copy link

What is it about what I'm asking or doing that you think is unique?
Clearly, 'text' files having different line endings isn't something unique to me. Even ignoring the historical reasons for CRLF, LFCR, LF, or CR endings, most text based internet protocols in fact dictate CRLF. Besides the expected CRLF, HTTP even has acceptance of both bare LF and CR as required. Source files consumed by compilers have historically never cared either.

So if you have test code that explicitly requires 'text' files they work on to remain in a very specific format then those files should be named in such a way as to make this clear or they should write those files themselves (or work in memory). Ideally, they would be written properly and not care. If you guys want to kick the can and mark your entire source tree as 'binary,' that's certainly your right, but to imply that me having an expectation of properly handling text files in development, runtime, and in source code management is somehow unique to me is really strange.

So either that, or you're saying me 'vendoring' Go itself is unique? If so, that's just the nature of our business. If we have to support and patch a product that's ten years old then we need all the tools used to be versioned alongside everything else so it can still be built. Developers don't install much of anything if we can avoid it. It's all checked in and versioned appropriately and developers fetch the code to build as well as the code that builds it all together. Recent Microsoft tools (C++ from v8 on) being a perfect example of garbage that can't simply be 'checked in.' :\

@bradfitz
Copy link
Contributor

We also vendor Go and patches. That's fine.

Nothing in Go the language or resulting binaries cares about line endings. The only places that do are some of our standard library tests, just for convenience. It's easier for us to treat everything as binary. Nobody using Go has to write their tests this way, but we do. If you vendor Go's tests (which is fine), just be aware that we assume the world is binary in our tests. If you don't want to assume that, it's on you to also carry patches in your vendored tree to "fix" the tests for the updated world's assumptions. (your world with CRLF)

@golang golang locked and limited conversation to collaborators Jul 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants