strongloop / strong-build

Build node packages into deployable applications
Other
47 stars 14 forks source link

find workaround for warnings about CRLF to LF conversions #7

Open sam-github opened 10 years ago

sam-github commented 10 years ago

http://stackoverflow.com/questions/1967370/git-replacing-lf-with-crlf

core.autocrlf set to false guarantees what gets checked out is what got checked in, this is appropriate for our use case

as-is, since npm (bless it) uses UNIX conventions for most of the files it writes, including the ones from most package.tgz files from npmjs.org, the "warning: LF will be replaced by CRLF in ...." will be spewed endlessly to the screen.... for files that the user has no control over.

We have to make sure its only temporarily false, though, and not mess with the user's defaults.

sam-github commented 10 years ago

(updated, I mistyped input when I meant false)

sam-github commented 10 years ago

I think we can do this with git -c core.autocrlf=false add ...

sam-github commented 10 years ago

I take it back, the problem is that if a file is text, some amount of normalization will occur, and its not reversible. I think we need to disable the text attribute (temporarily) on all files we add, see http://git-scm.com/docs/gitattributes. I'm not sure how to do this in a temporary way.

npm seems to heavily favor unix line endings... but programmer tools on windows often default to native. Not our fault, but the pages of warnings streaming by detract from the user experience.

sam-github commented 10 years ago

I think we should temporarily modify the config settings, either through command line, or via GIT_CONFIG being set to an internal file (so only effects slc build). Whatever we have to do. I don't think any git settings on line endings will work, because there is a mix of newline styles (in the npm packfiles) and whatever the user's conventions are. One option is a temporary .gitattributes, that forces node_modules, or perhaps everything, to be added as _non-text_ files, to avoid EOL conversions and the warnings associated with them.

Its a real problem, and no amount of simple "set your EOL up for windows" advice can really help, as far as I can tell.

sam-github commented 9 years ago

This has now become a linux problem, not just an OS X problem.

I suspect this started when js2xmlparser was added by @rmg recently to something.

sam@samtu:~/w/arc/my-app (master *%) % slc build
Running `git branch "deploy"`
Not merging HEAD into `deploy`, already up to date.
Running `npm install --ignore-scripts`
Running `npm prune --production`
Running `git add --force --all .`
  => warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/.gitattributes.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/CHANGES.md.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/LICENSE.md.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/README.md.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/example/example.js.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/lib/js2xmlparser.js.
The file will have its original line endings in your working directory.
warning: CRLF will be replaced by LF in node_modules/loopback/node_modules/strong-remoting/node_modules/js2xmlparser/test/test.js.
The file will have its original line endings in your working directory.
Running `git write-tree`
  => f1001d81a693f5a68540d4fb3b7fd5ccf511f025
Running `git commit-tree -p "refs/heads/deploy" -m "Commit build products" f1001d81a693f5a68540d4fb3b7fd5ccf511f025`
Running `git update-ref "refs/heads/deploy" 022c02462bc71c7239c246fcf76f2697859a0662`
Committed build products onto `deploy`

@chandadharap we need to prioritize fixing this.

sam-github commented 9 years ago

maybe started with this: https://github.com/strongloop/strong-remoting/pull/111

maybe the jsxml dev uses windows?

rmg commented 9 years ago

If we go low level enough we could do: https://gist.github.com/rmg/c0d542cc3a0338874d7e#file-git-snapshot-sh-L34

rmg commented 9 years ago

Submitted an upstream PR to address the js2xmlparser specific problem in the comment: michaelkourlas/node-js2xmlparser#22

Still need to look into what we can do about controlling, for the build steps only, how to mark new files as binary when adding them to the deploy commit.

rmg commented 9 years ago

After some digging, I am now further from a solution.

TL;DR

The biggest obstacle to fixing this is that whatever solution is used to fix the commit side must be mirrored on the checkout side, which we have no control over (even in the case of strong-pm, there's no way to know the required information until after we need it).

My recommendation is to put this on the back burner until someone can come up with a workable solution.

Long Version

The warnings in the js2xml example can be squelched with -c core.safecrlf=false, but that could hide early warning of file corruption caused by line-ending conversion (such as if a binary file was misidentified as a text file and all the CRs were removed, irreversibly corrupting the file).

If we instead disable autocrlf, it will be applied to all files being checked in, not just the new files, and the files will potentially be corrupted by being checked out later without that autocrlf override.

I'm currently seeing only two options, and both of them suck:

  1. create a .gitattributes file as part of the production branch that lists every new file and specifies it as -text. This will be clunky and cause problem with later branch merges. There's also the chicken/egg problem of the file needing to be checked out before the rest of the branch, which means it will probably not work at all.
  2. use -c core.safecrlf=false and suppress the warnings at the risk of corrupting any binary artifacts that git thinks are text. Since the purpose of the feature is specifically for committing generated/compiled artifacts, corrupted pseudo-binaries seems likely.
sam-github commented 9 years ago

I'm thinking we should set:

I'll have to try on Windows to see if this makes sense, it basically means that we recommend Windows developers use LF as EOL, and claim that is the node convention.