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

Trim BOM from config file for windows support #6900

Merged
merged 1 commit into from
Jun 24, 2016
Merged

Trim BOM from config file for windows support #6900

merged 1 commit into from
Jun 24, 2016

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Jun 23, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

Default windows text editor (Notepad) adds a BOM to the beginning of the
file. This needs to be trimmed otherwise we will get an "invalid toml"
error.

see influxdata/telegraf#1378
and http://utf8everywhere.org/#faq.boms

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @gunnaraasen and @e-dard to be potential reviewers

@sparrc
Copy link
Contributor Author

sparrc commented Jun 23, 2016

without this change we get an error when parsing a file with a BOM:

% influxd -config ~/db/ws/influxd.conf.bom

 8888888           .d888 888                   8888888b.  888888b.
   888            d88P"  888                   888  "Y88b 888  "88b
   888            888    888                   888    888 888  .88P
   888   88888b.  888888 888 888  888 888  888 888    888 8888888K.
   888   888 "88b 888    888 888  888  Y8bd8P' 888    888 888  "Y88b
   888   888  888 888    888 888  888   X88K   888    888 888    888
   888   888  888 888    888 Y88b 888 .d8""8b. 888  .d88P 888   d88P
 8888888 888  888 888    888  "Y88888 888  888 8888888P"  8888888P"

[run] 2016/06/23 09:24:27 InfluxDB starting, version unknown, branch unknown, commit unknown
[run] 2016/06/23 09:24:27 Go version go1.6.2, GOMAXPROCS set to 4
[run] 2016/06/23 09:24:27 Using configuration at: /Users/sparrc/db/ws/influxd.conf.bom
run: parse config: Near line 0 (last key parsed ''): Bare keys cannot contain '\ufeff'.

@sparrc
Copy link
Contributor Author

sparrc commented Jun 23, 2016

corresponding PR in telegraf: influxdata/telegraf#1404

@sparrc
Copy link
Contributor Author

sparrc commented Jun 23, 2016

cc @nathanielc in case you want to make this change for Kapacitor as well

@e-dard
Copy link
Contributor

e-dard commented Jun 23, 2016

@sparrc I think it would be nice to keep these sort of OS-specific actions in the the OS binaries or runtimes. So I would prefer something in trimBOM like:

if runtime.GOOS != "windows" {
    return fileBytes
}

An alternative approach would to use build flags to compile out the trimBOM function, but that seems a bit overkill in this case. Maybe @jwilder has some thoughts on this?

@sparrc
Copy link
Contributor Author

sparrc commented Jun 23, 2016

That seems reasonable. Although one counterpoint is that the BOM is not necessarily only on Windows. Any editor/OS can add that prefix, but as far as I know Notepad is the only one that actually does.

Also a file that Notepad saves could be transferred to any other machine.

Vim, for example, deals with the BOM on any system. You can validly open a file with a BOM on linux/osx so we may want to do the same.

@e-dard
Copy link
Contributor

e-dard commented Jun 23, 2016

Ah right OK. If it's not windows-specific then it's cool as it is. LGTM 👍

@jsternberg
Copy link
Contributor

Update the changelog and I think this is good. I agree with @sparrc that we shouldn't limit this code to only Windows. It's possible for someone to edit a file in Notepad on Windows and then copy it to a Linux machine by using something like Chef or Ansible so I think it's worth always trimming the byte mark since it doesn't really harm the Linux version.

@sparrc sparrc force-pushed the trim-bom branch 3 times, most recently from 7fe2e99 to c7141b5 Compare June 23, 2016 16:01
// this is for Windows compatability only.
// see https://github.com/influxdata/telegraf/issues/1378
func trimBOM(f []byte) []byte {
return bytes.Trim(f, "\xef\xbb\xbf")
Copy link
Contributor

@joelegasse joelegasse Jun 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely be bytes.TrimPrefix... bytes.Trim will remove the code points from the beginning and the end. If this is only going to appear at the beginning, we should only check there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll change it

Default windows text editor (Notepad) adds a BOM to the beginning of the
file. This needs to be trimmed otherwise we will get an "invalid toml"
error.

see influxdata/telegraf#1378
and http://utf8everywhere.org/#faq.boms
@sparrc sparrc merged commit 98360c5 into master Jun 24, 2016
@mark-rushakoff mark-rushakoff deleted the trim-bom branch July 6, 2016 17:07
@timhallinflux timhallinflux added this to the 1.0.0 milestone Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants