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/gofmt: doesn't honor line endings of Windows \r\n terminated source files #16355

Closed
pbennett opened this issue Jul 13, 2016 · 24 comments
Closed

Comments

@pbennett
Copy link

@pbennett pbennett commented Jul 13, 2016

This is present in all current versions of gofmt.
Issue #2242 referenced it, but I was asked to create a new issue for it by Robert.
Robert, we spoke about this at Gophercon to help jog your memory.

Basically, with a \r\n terminated .go file, gofmt will rewrite the entire file to be \n terminated instead. Windows systems use this by default (so this isn't the place for a one-true EOL discussion please) and gofmt should honor the format of the source file. Changing the line endings has a huge impact in regards to source control systems let alone some native windows tools not handling it well.

To keep it simple, I would suggest just using the line terminator from the first line as the hint for the entire file. Determine the EOL chars from the first line - apply on all future writes.

@pbennett pbennett closed this Jul 13, 2016
@pbennett pbennett reopened this Jul 13, 2016
@pbennett
Copy link
Author

@pbennett pbennett commented Jul 13, 2016

@griesemer Mentioning you in comment to bring you in... I'm not sure how to assign to you directly.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 13, 2016

I am strongly opposed to changing this. The whole point of gofmt is to have ONE format. The choice of "\n" vs "\r\n" is no different from the choice between spaces and tabs. gofmt made the decision already.

@pbennett
Copy link
Author

@pbennett pbennett commented Jul 13, 2016

Line endings are a platform issue. If you want to support Windows, you need to accept the fact that it is different. Comparing line-endings to the tabs vs. space issues is incorrect. They aren't the same issue, at all. Some Windows tools don't properly handle LF terminated files. None care about tabs or spaces.
gofmt should not be the hammer used to drive home a personal 'one true EOL' battle.
Would I like everything to use the same line endings? Sure, but that's not the reality.
This is fundamentally a Windows platform support issue, and one where Go is not being a good citizen.

@dominikh
Copy link
Member

@dominikh dominikh commented Jul 13, 2016

huge impact in regards to source control systems

Can you expand on this?

Some Windows tools don't properly handle LF terminated files

Which relevant tools don't? Only notepad.exe comes to mind, which hardly counts as relevant.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 13, 2016

Breaking notepad.exe is unfortunate, but I think it's the lesser evil compared to having gofmt not have consistent output.

The only thing that's changed in the 5 years since we last had this conversation in #2242 and on the mailing list is that more Windows editors now support NL-only files.

@pbennett
Copy link
Author

@pbennett pbennett commented Jul 13, 2016

This is also an issue with source control systems. Line endings will be checked out based on the local system and expected to be in the local system format - possibly converting to a common format internally. Variations abound in how this is handled by Git, Perforce, etc. So having a file checked in as CR/LF and gofmt changing it to LF is a problem. It is changing the file in a way that can cause checkin problems.
Again, the standard Windows EOL terminator is \r\n - and gofmt should honor that when present.

@griesemer griesemer self-assigned this Jul 13, 2016
@griesemer griesemer added this to the Unplanned milestone Jul 13, 2016
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jul 13, 2016

I think @bradfitz makes a strong point - we really want to just have one format. The CR/LF vs LF only differences caused a lot of pain in the early days. The language spec was even adjusted to accommodate for that (e.g. CRs are stripped from raw strings https://golang.org/ref/spec#String_literals).

We also know that a huge number of Go developers work on Windows (the majority?), so I'd like to hear what other Windows users have to say here. Specifically, is the fact that gofmt strips CRs from .go files a problem when interacting with whatever version control system is used?

My inclination is that the right approach here might be for an organization that works exclusively in a CR/LF environment to make their own customized gofmt. It's trivial to to that (the cmd/gofmt command essentially uses the go/format package which produces a string containing the formatted source. It's a simple loop to introduce CR\LFs as necessary.)

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jul 14, 2016

I don't think I can be considered as average Windows developer. But I never had any problems with Go source files having LF instead of CR/LF.

All standard Go repositories use .gitattributes file (see #9281 (comment) for details) to make git treat all source files as binaries. This helps with "LF instead of CR/LF" problem. Is that correct approach? Is it suitable for others? I am not sure. But works fine for Go Team.

I suppose we need to understand what the real problem here is. Then we can decide what the solution is.

Alex

@bradfitz bradfitz added the OS-Windows label Nov 22, 2016
@bradfitz bradfitz changed the title gofmt doesn't honor line endings of Windows \r\n terminated source files cmd/gofmt: doesn't honor line endings of Windows \r\n terminated source files Nov 22, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 21, 2017

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please reopen if this is a mistake or you have the requested information.)

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

IMO, the tools should support two things:

  • Normalizing the file contents before comparison, so it doesn't treat EOL-only differences as something "wrong"
  • Writing the file contents back to disk with CRLF line endings if those were detected in the original source file.

This should make the tooling friendly to all dev environments, regardless of the source control tools / settings used (such as git's autocrlf.)

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

I opened a PR to do add both of those features to goimports (https://go-review.googlesource.com/c/tools/+/83535) , but was directed here for "policy discussion".

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2017

@dlwyatt, what breaks for you on Windows if you keep the files with only LF line endings?

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

Is that relevant? The argument isn't whether anyone should have CRLF endings, it's whether go's tooling should work well when people do. The compiler doesn't care about the CRLF, why should gofmt / goimports?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2017

I believe it is relevant. You've said there's a problem and are proposing a solution, so we want to understand what the problem is first, before evaluating the solution.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 12, 2017

@dlwyatt The compiler only consumes a text file; it doesn't produce one. But gofmt is in the business of standardizing textual output. So this comparison doesn't quite hold up.

The question is relevant because it's not obvious what to do with line endings if there are multiple options. What should be done if a file mixes CRLF and LF endings? What about CRLF's in comments? Note also that the compiler discards CRs in raw string literals (per the spec https://golang.org/ref/spec#String_literals).

It gets messy pretty quickly. If LF works, why make it more complicated?

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

If you're going to force LF-only via tooling, you may as well make the compiler puke if it comes across CRLF.

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

Excluding the argument about producing versus consuming for a moment: if you run them with the -l switch, they'll report files as wrong even if the line endings are the only difference. That's a consume-only bit of functionality that still doesn't work on a file that the compiler treats as legal.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 12, 2017

@dlwyatt We certainly don't want to complain if the compiler sees a CRLF; that would put an unnecessary burden on people to ensure their editors behave in just the one way.

But I hear your comment regarding the -l flag. Perhaps CRLF's and LF's should be considered equal for that purpose. Would that alleviate your concerns?

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

Mostly. At least then if a file were to be modified, it would have a valid content change (in addition to having its line endings wiped out and replaced.) It's still a bit annoying to have the git line ending warnings show up, but not the end of the world.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2017

It's still a bit annoying to have the git line ending warnings show up, but not the end of the world.

Where was that mentioned?

Again, please discuss problems. We're much more receptive to discussing problems than discussing solutions to unstated problems.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 12, 2017

@dlwyatt Since this is a closed issue, file a new issue with the concrete problem ("-l considers CRLF and LF line endings as different")?

@dlwyatt
Copy link

@dlwyatt dlwyatt commented Dec 12, 2017

Sorry, I thought I had posted a bit of console output, but that was over in ptimports (palantir/checks#59)

Looks like yes, goimports also inserts plain newlines:

C:\Users\dwyatt\Documents\Git\go\src\github.com\palantir\checks\ptimports [windows-support ≡]
λ  goimports -w .\ptimports.go
C:\Users\dwyatt\Documents\Git\go\src\github.com\palantir\checks\ptimports [windows-support ≡ +0 ~1 -0 !]
λ  git diff
warning: LF will be replaced by CRLF in ptimports/ptimports.go.
The file will have its original line endings in your working directory.
C:\Users\dwyatt\Documents\Git\go\src\github.com\palantir\checks\ptimports [windows-support ≡ +0 ~1 -0 !]

Never noticed that before because I use VSCode, and apparently it takes care of that already when it runs goimports.

@grokify
Copy link

@grokify grokify commented Jul 5, 2018

@MyYellowBlanket
Copy link

@MyYellowBlanket MyYellowBlanket commented Sep 27, 2018

if someone runs 'goreturns -d somefile.go' on windows (in a git-bash since no 'diff' command installed). He probably see the whole file has changed if goreturns doesn't keep the original line ending.

thxCode pushed a commit to thxCode/rancher that referenced this issue Apr 30, 2019
- Use dapper
- Patch Linux pipelines with OS info
- Add Windows 1809 Drone support
- Add .gitattributes file to fix
golang/go#16355
thxCode pushed a commit to thxCode/rancher that referenced this issue May 1, 2019
**Problem:**
- Cannot set up kubelet on Windows
- Sometimes, host kubelet cannot connect controlplanes via container nginx proxy, but we found that only the connection between kubelet and container nginx has been broken.
- Upgraded from previous existing cluster causes Windows agent failed
- Running a few days, Windows host will crash by OOM, as `docker create
-> docker start` leaks none-page memory
- The Windows code doesn't look very clear
- Need to support Flannel vxlan mode
- Need to support CI
- Need to support Docker manifest images
- Need to add unit test

**Solution:**
- Adjust kubelet options
- Replace container Nginx proxy by host Nginx
- Fix Windows error when upgrading rancher
    + Use `AllK8sWindowsVersions` to index system images
    + Specify the `NetworkMode` of Process to `none`
- For Windows, replace `docker create -> docker start` to `docker run`
- Refactor Windows agent
    + Separate logrus hook into a file
    + Create `winRunner` to execute main logic
    + Remove ugly break label
    + Separate connecting into a single method
    + Only treat Powershell active retry exit status
    + Adjust PowerShell script
        + Redirect log to c:\var\log
        + Reduce retry count
        + Change log.ps interval to 1s
        + Log to file if running background
- Support Flannel Vxlan Mode
    + Adjust PowerShell script via moving `net-conf.json` into `start-flanneld`
    + Support to configure Flanneld backend via `FlannelBackendConfig`
- Support CI
    + Use dapper
    + Patch Linux pipelines with OS info
    + Add Windows 1809 Drone support
    + Add .gitattributes file to fix golang/go#16355
- Support Docker manifest images
    + Remove the processing of Windows image suffix in everywhere
- Unit test
    + There is not syslog implemented on Windows, so separate by os
    + Add test logic into CI

**Issue:**
- rancher#17341
- rancher#16074
- rancher#19048
- rancher#17499
- rancher#17499
cjellick added a commit to rancher/rancher that referenced this issue May 1, 2019
**Problem:**
- Cannot set up kubelet on Windows
- Sometimes, host kubelet cannot connect controlplanes via container nginx proxy, but we found that only the connection between kubelet and container nginx has been broken.
- Upgraded from previous existing cluster causes Windows agent failed
- Running a few days, Windows host will crash by OOM, as `docker create
-> docker start` leaks none-page memory
- The Windows code doesn't look very clear
- Need to support Flannel vxlan mode
- Need to support CI
- Need to support Docker manifest images
- Need to add unit test

**Solution:**
- Adjust kubelet options
- Replace container Nginx proxy by host Nginx
- Fix Windows error when upgrading rancher
    + Use `AllK8sWindowsVersions` to index system images
    + Specify the `NetworkMode` of Process to `none`
- For Windows, replace `docker create -> docker start` to `docker run`
- Refactor Windows agent
    + Separate logrus hook into a file
    + Create `winRunner` to execute main logic
    + Remove ugly break label
    + Separate connecting into a single method
    + Only treat Powershell active retry exit status
    + Adjust PowerShell script
        + Redirect log to c:\var\log
        + Reduce retry count
        + Change log.ps interval to 1s
        + Log to file if running background
- Support Flannel Vxlan Mode
    + Adjust PowerShell script via moving `net-conf.json` into `start-flanneld`
    + Support to configure Flanneld backend via `FlannelBackendConfig`
- Support CI
    + Use dapper
    + Patch Linux pipelines with OS info
    + Add Windows 1809 Drone support
    + Add .gitattributes file to fix golang/go#16355
- Support Docker manifest images
    + Remove the processing of Windows image suffix in everywhere
- Unit test
    + There is not syslog implemented on Windows, so separate by os
    + Add test logic into CI

**Issue:**
- #17341
- #16074
- #19048
- #17499
- #17499
tnir added a commit to tnir/mattermost-plugin-rssfeed that referenced this issue Aug 22, 2019
cf.

- golang/go#16355
- golang/go#2242

Signed-off-by: Takuya Noguchi <takninnovationresearch@gmail.com>
tnir added a commit to tnir/mattermost-plugin-rssfeed that referenced this issue Aug 26, 2019
cf.

- golang/go#16355
- golang/go#2242

Signed-off-by: Takuya Noguchi <takninnovationresearch@gmail.com>
@golang golang locked and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.