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

.gitattributes: turn off CRLF for Makefile.am #1344

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

MarcelRaad
Copy link
Member

Otherwise, buildconf in a Windows checkout fails with:

.ibtoolize: error: AC_CONFIG_MACRO_DIRS([m4]) conflicts with ACLOCAL_AMFLAGS=-I m4

@mention-bot
Copy link

@MarcelRaad, thanks for your PR! By analyzing the history of the files in this pull request, we identified @billhoffman and @gknauf to be potential reviewers.

@jay
Copy link
Member

jay commented Mar 21, 2017

? I use windows checkouts all the time with CRLF translation and I've never seen that

@MarcelRaad
Copy link
Member Author

It happened for me on two machines where I had cloned the git repository in Windows and then ran buildconf from the Ubuntu bash that you can get with Windows 10.

@MarcelRaad
Copy link
Member Author

The problem is briefly explained here:
http://pete.akeo.ie/2010/12/that-darn-libtoolize-acconfigmacrodirm4.html

@jay
Copy link
Member

jay commented Mar 22, 2017

hm. Well I'd think if you're using Ubuntu to work on a local curl repo that's using CRLF translation you're going to run into problems. This probably isn't what you want to hear but maybe you should turn it off, reset the repo to just LF?

git config --local core.autocrlf input
git config --local core.eol lf
git rm -f --cached -r .
git clean -xdf
git reset --hard

I think turning it off for just this one file you'll run into more problems. But let's say we shut CRLF translation off for all *.am, is there any negative downside to that other than some editors won't work? Does any autotools require CRLF line endings that you know of?

With just this file changed are you able to build curl and then run the tests successfully?

@MarcelRaad
Copy link
Member Author

MarcelRaad commented Mar 22, 2017

This probably isn't what you want to hear but maybe you should turn it off, reset the repo to just LF?

Thanks, I didn't know this was possible for an existing checkout. Although it might be a bit inconvenient as I'm developing on Windows and running the tests on Linux, so I'd have to switch between line endings regularly.

I think turning it off for just this one file you'll run into more problems. But let's say we shut CRLF translation off for all *.am, is there any negative downside to that other than some editors won't work? Does any autotools require CRLF line endings that you know of?

With just this file changed are you able to build curl and then run the tests successfully?

It doesn't make a difference if Makefile.am or *.am is used. Both ways, I'm able to build and run the tests fine with a 98% success rate. But you're right, *.am would be better as it's consistent with the other entries in .gitattributes, so I've just changed to using that.

@MarcelRaad
Copy link
Member Author

MarcelRaad commented Mar 25, 2017

@jay

But let's say we shut CRLF translation off for all *.am, is there any negative downside to that other than some editors won't work? Does any autotools require CRLF line endings that you know of?

With just this file changed are you able to build curl and then run the tests successfully?

I haven't found any downsides to forcing LF for *.am files and everything works for me on Linux as well as Windows (MSYS2) with CRLF translation.
There are 5 tests failing with CRLF translation that don't fail when using only LF: 1022 1023 1135 1221 1222.
1135 fails also under Windows, the others only under Linux.
If there are no objections, I'll push this commit and look at those 5 tests later.

If Makefile.am uses CRLF, buildconf in a Windows checkout fails with:
".ibtoolize: error: AC_CONFIG_MACRO_DIRS([m4]) conflicts with
ACLOCAL_AMFLAGS=-I m4"
@MarcelRaad MarcelRaad merged commit 2c3af5b into curl:master Mar 27, 2017
@MarcelRaad MarcelRaad deleted the makefile_lf branch March 29, 2017 18:22
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 15, 2017
Bash on Linux errors out on CR characters.
This makes tests 1221 and 1222 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 15, 2017
Ignore everything after the version numbers in LIBCURL_VERSION and
LIBCURL_VERSION_NUM to ged rid of the extra CR character.
This makes tests 1022 and 1023 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 15, 2017
Bash on Linux errors out on CR characters.
This makes tests 1221 and 1222 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 15, 2017
Ignore everything after the version numbers in LIBCURL_VERSION and
LIBCURL_VERSION_NUM to ged rid of the extra CR character.
This makes tests 1022 and 1023 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 16, 2017
Bash on Linux errors out on CR characters.
This makes tests 1221 and 1222 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 16, 2017
Ignore everything after the version numbers in LIBCURL_VERSION and
LIBCURL_VERSION_NUM to ged rid of the extra CR character.
This makes tests 1022 and 1023 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 16, 2017
This makes test 1135 pass with CRLF checkouts.

Ref: curl#1344 (comment)
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 17, 2017
Bash on Linux errors out on CR characters.
This makes tests 1221 and 1222 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
Closes curl#1422
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 17, 2017
Ignore everything after the version numbers in LIBCURL_VERSION and
LIBCURL_VERSION_NUM to ged rid of the extra CR character.
This makes tests 1022 and 1023 pass on Linux with a CRLF checkout.

Ref: curl#1344 (comment)
Closes curl#1422
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Apr 17, 2017
This makes test 1135 pass with CRLF checkouts.

Ref: curl#1344 (comment)
Closes curl#1422
@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants