Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Normalize line endings. #78

Closed
wants to merge 62 commits into from
Closed

Conversation

DanielFabian
Copy link

After setting text=auto in #75, it would make sense to once normalize all files that are not normalized yet. Thereafter all new files will always have the same line endings which will help the diff and is also good for consistency between platforms.

funnelweb and others added 30 commits October 30, 2012 10:11
switch to strong names for compiler binaries and update the bootstrap
compiler with those names. The strong names are based on
src\fsharp\test.snk

this means the strong name for FSharp.Compiler.dll changes, but no one
should be depending on that and the new names are useful as the won't
collide with the visual studio versions of these DLLs on windows. The
monodevelop binding is neutral to the strong names used.

we still compile FSharp.Core as
delay-signed-with-the-microsoft-key-and-then-mono-key-signed which Mono
recognizes as strong named but i think windows does not. We have to use
this identity for FSHarp.Core to keep binary compatibility with
Windows-compiled thing like type provider DLLs.

We may have to use the Microsoft FSHarp.Core.dll on Windows, i think it
gets picked up automatically from the GAC but need to check tomorrow.

also fixes bugs in ilsupp.fs in strong nae signing.

tested by bootstrapping, and manually examining the names on the dlls.
The FSharp.Core in lib/bootstrap/4.0 needs to be properly strong name
signed if we'r going to run on Windows without VS installed and without
FSharp.Core in the GAC
See fsharp#50

The comments in FSharpBuild/Fsc.fs explain a bit more. It is realy hard
to work out how to make this perfectly consistent. Essentially, be
careful using resources in folders if you want your projects to be
portable across VisualStudio/MSBuild and mono/MonoDevelop/XBuild

We hit this bug in fsharp/fsharpbinding, but we've moved that to no
longer put resources in folders for now.
Mono version 2.11.5 was never released and was actually renamed to 3.0
This gives documentation for FSharp.Core.dll in MonoDevelop
This includes adding dependencies for the Mono profile 2.1 binaries we
need to reference
@DanielFabian
Copy link
Author

the trouble with not normalizing we see occasionally on files just randomly differ on every single line. It's not a huge problem, more like a caveat.

@DanielFabian
Copy link
Author

I did the
git filter-branch -f --prune-empty --tree-filter 'git ls-files -z | xargs -0 dos2unix' -- --all

and it made the history really pretty, check it out in my repo

@knocte
Copy link
Contributor

knocte commented Dec 2, 2012

@DanielFabian : agreed, but what I have a problem is what to normalize to? IMHO it should be Windows line-endings, otherwise you would translate the same problem to the other way around: when you change something in an msbuild file (.sln, .csproj, .fsproj) via MonoDevelop, you will see the whole file changing.

Cannot you use text=auto but using Windows line endings?

@DanielFabian
Copy link
Author

well, no. What it auto does locally what's for you and in the repo it does LF. You do can set CRLF explicitly, but then it just is CRLF on all systems, including Unix. Or you can set it to LF for all systems. But you can't set CRLF in the repo and auto elsewhere. (It would have no effect, though, because you'd still get your line endings locally)

@DanielFabian
Copy link
Author

Do play a bit with my branch, maybe https://github.com/DanielFabian/fsharp/commits/master it really did clean up the history. Maybe play a bit with git blame and a bit with git diff etc...

@ghost
Copy link

ghost commented Dec 6, 2012

What's the status here? Do people think this should be merged?

I think its good to normalize line the endings in GitHub - is this commit the best way to do it?

@rneatherway
Copy link
Member

I agree that the line endings should be normalized. I think the only concern is what they will be when checked out. This will default, as @DanielFabian says, to the line ending of your platform, but can be changed if desired in your .gitconfig. The only reason I can think of for doing this would be to do a diff with Codeplex for example, and this isn't too bad for a one-off event. Rebasing cleans up the history, so this really solves the problem for good.

@knocte: Monodevelop could only be a problem if it reads a .fsproj with unix line endings and then saves it with windows line endings. Surely this isn't the behaviour?

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

@rneatherway : that is the behaviour, it's what I've tried to explain

@cdrnet
Copy link
Contributor

cdrnet commented Dec 6, 2012

*.fsproj eol=crlf in .gitattributes will make sure it is still normalized in the repo but always in CRLF in the working directory/checkout, even on Linux. Hence MonoDevelop should work fine, unless I'm missing something? Or does it save all files in CRLF, not just the project file?

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

You would need that for SLN files as well.

@rneatherway
Copy link
Member

Right, that wasn't clear before. So Monodevelop does this for all files including *.cs, *.fs? It would seem pretty strange behaviour for it to only change *.{fsproj,csproj,sln}.

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

It only changes msbuild files because that's what it needs to do to be fully interoperable with VS. VS saves all msbuild files with CRLF, but it doesn't do it with the rest unless you tell it to.

@rneatherway
Copy link
Member

Thanks, I see. So two options then:

  1. Normalize the repository, but set eol=crlf for all msbuild files.
  2. Have all files be CRLF everywhere (no normalization) and reject any pull requests that violate this policy.

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

There can be a thrid option derived from the 2nd that is not so harsh:
3. Have all files be CRLF everywhere (no normalization), and include a git pre-commit hook to unix2dos all files so we don't get any pull requests that violate this policy.

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

And when I said "all files" I meant "all files modified in the commit". Just in case someone is thinking this solution hurts performance...

@rneatherway
Copy link
Member

This seems like a reasonable solution.

@ghost
Copy link

ghost commented Dec 6, 2012

@knocte I don't yet repro this behaviour where MD saves all .fsproj files as CRLF. Maybe I'm doing something wrong, but I tried this on OSX with 3.0.5:

  • ran dos2unix on an .fsproj (not under a git repo) and checked its length
  • opened it in MD
  • renamed a file in solution pad but kept the name the same length
  • closed MD

The file stayed the same "unix" length after the edit. I then did the same on the "dos" file and it seemed to convert to "unix" endings (the file became shorter)

In any case, I don't think this repo should use CRLF line endings just because of the behaviour of one particular development tool (MD), which to be honest isn't even used very much AFAIK when editing the compiler.

To me, "auto" line endings look like the simplest and best choice to keep things sane on the different systems.

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

@dsyme : TBH I haven't tried with *.fsproj files specifically. I just know this is the behaviour of MD with msbuild files in general (which maybe only applies to *.csproj files, and the FSharpBinding decided not to mimic this).

In regards to the argument of MD not being used much in fsharp compiler: fair enough. But maybe this argument doesn't apply so much to other modules such as FSharpBinding?

@knocte
Copy link
Contributor

knocte commented Dec 6, 2012

BTW, if FSharpBinding indeed has this behaviour, I would consider it a bug (otherwise it would generate lot of noise when working with F# projects which were originally created in Windows).

@cdrnet
Copy link
Contributor

cdrnet commented Dec 6, 2012

Throwing in some technical aspects of Git itself.

(I personally prefer 1) but that doesn't matter here)

Note that the normalization of the complete git history as discussed above and tried by @DanielFabian causes some havoc among all forks as well, although that's just once, then it is done and fine. Forks essentially need to reset to the new master. It is still possible to cherry-pick from the old history afterwards (e.g. for pending pull requests), but we should never merge between them.

  1. Enable repository-wide normalization by git:
  • Trivial to implement by adding the following lines to .gitattributes:
*.sln text eol=crlf merge=union
* text=auto

A complete .gitattributes could then look like:

*.cs text diff=csharp
*.sln text eol=crlf merge=union
*.fsproj text merge=union
*.csproj text merge=union
* text=auto
  • Automatically enforced (auto-fixing if needed, with warning)
  • Well supported by git and git tools
  • Supports custom working copy formats depending on client config out of the box (e.g. on Windows, text files would be checked out as CRLF by default, but this is locally configurable).
  • Repository format does not depend on how git was installed or configured locally
  1. Enable clone-wide normalization by git (status quo):
  • Trivial to implement: do nothing, i.e. do not specify any text handling in .gitattributes
  • Normalization depending on local core.autocrlf settings, not enforceable repository wide
  • Works quite well in Windows-only projects, provided all users have installed and configured git the same way
  • If core.autocrlf is enabled, can cause a lot of files to be marked as changed right on a clean checkout if others have not enabled it or configured differently. Merging can quickly become a nightmare.
  • Deprecated in git (exactly because of above points) and replaced with .gitattributes text handling some years ago.
  1. Disable any text normalization by git, accept random line endings
  • Trivial to implement by adding the following line to .gitattributes:
* -text
  • Works quite well in Windows-only projects as the randomness is zero :) but this is not the case here
  • Diff and Merge can get messy once files start being converted between the two randomly by the editors; gitk allows to "not show" such changes, but otherwise git tools are not very good at ignoring white space and line endings in my experience. But at least it doesn't flag files as changed right after checkout. But it might do so after a file has been opened and saved in your favorite editor even if no change was made.
  • Some primitive tools or editors might complain (or show redundant characters) when confronted with uncommon line endings for that platform
  1. Try to normalize to CRLF by other means
  • Not standard behavior, so it is unclear how it will be supported by tools
  • Hooks are not enforceable unless we can set up server-side hooks (client-side hooks are neither automatically distributed nor enforced by git)
  • Server-side hooks cannot fix anything, just block the operation until it is fixed manually by the contributor, which can become tedious
  • clean/smudge filters could work and can be configured by .gitattributes, but it is unclear to me what the advantage would be over using the standard text normalization behavior as in 1).

@funnelweb
Copy link
Contributor

Then let's just go for (1). That means merging this pull request and adjsuting the .gitattribute, right?

@DanielFabian - do you know why this pull request is labelled "cannot be automatically merged"?

@DanielFabian
Copy link
Author

I'm not 100% sure, but most likely, because I ran the script I mentioned
earlier and it rewrote every commit from the beginning of the git history,
thus also rewriting the parent of the branch.

Maybe just run my script locally and make a 'push -f' to force an overwrite
(took about 4 minutes of processing locally in my case).
EDIT: the script mentioned is:
git filter-branch -f --prune-empty --tree-filter 'git ls-files -z | xargs -0 dos2unix' -- --all

@funnelweb
Copy link
Contributor

The repository has now been normalized to Unix and "text auto", and a .gitattributes like put in place like the one described by @cdrnet. Thanks everyone and lets see how this goes.

@funnelweb
Copy link
Contributor

closing this since it has been don by direct commit

@funnelweb funnelweb closed this Dec 9, 2012
fsgit pushed a commit that referenced this pull request Jul 2, 2014
…ifiers.

Related to #78 - without this restriction, the typechecker can't distinguish between valid pipes separating cases and invalid pipes that are embedded in a case identifier.  The result is that nonsense like this compiles, when it should not:

let (|``Foo|Bar``|) x =
    match x with
    | 0 -> Foo
    | _ -> Bar

match 42 with
| Foo -> "foo"
| Bar -> "bar" (changeset 1282321)
fsgit pushed a commit that referenced this pull request Jul 2, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.