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

cmake: Remove strange bytes used for converting Makefiles #1275

Closed
wants to merge 1 commit into from

Conversation

ligfx
Copy link
Contributor

@ligfx ligfx commented Feb 21, 2017

An alternative solution that came out of discussion on #1271. The strange bytes are used to mark escaped newlines so that they can be converted back after processing. However, text editors and other programs tend to silently change these characters in CMakeLists.txt or complain. This is a cosmetic concern for generated files (which I assume no-one reads), so let's remove it entirely.

@mention-bot
Copy link

@mention-bot mention-bot commented Feb 21, 2017

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

Copy link
Contributor

@webmaster128 webmaster128 left a comment

I tested this PR and support it (modulo the minor change request).

The resulting files are still safe for human consumption. The missing newlines only affect file lists and not the overall document or the license header. See both versions here: https://gist.github.com/webmaster128/6caba8a76052ea240a2e86077bf569bf

CMakeLists.txt Outdated
@@ -1043,9 +1043,8 @@ function(TRANSFORM_MAKEFILE_INC INPUT_FILE OUTPUT_FILE)
string(REPLACE "$(top_srcdir)" "\${CURL_SOURCE_DIR}" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT})
string(REPLACE "$(top_builddir)" "\${CURL_BINARY_DIR}" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT})

string(REGEX REPLACE "\\\\\n" "§!§" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT})
string(REGEX REPLACE "\\\\\n" "" MAKEFILE_INC_TEXT ${MAKEFILE_INC_TEXT})
Copy link
Contributor

@webmaster128 webmaster128 Feb 24, 2017

Choose a reason for hiding this comment

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

The replacement string should be " " instead of "" to be safe in cases where you have

XY_FILES = progress.h\
formdata.h

which would get converted to

XY_FILES = progress.hformdata.h

Even if this is not happening in current Makefile.inc files, an author of such a file expects that a whitespace is set when writing \↵.

Copy link
Contributor

@webmaster128 webmaster128 Feb 24, 2017

Choose a reason for hiding this comment

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

Sorry, I was wrong here. \↵ means breaking without adding whitespace. Confirm the Makefile

command2:
        ech\
o 123

where echo 123 is executed instead of ech o 123.

Copy link
Contributor

@webmaster128 webmaster128 Feb 24, 2017

Choose a reason for hiding this comment

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

Okay, now it is getting strange. The rule does not apply for variable definitions. The Makefile

AVAR = 12\
34

command2:
        echo "${AVAR}"
        echo -n "${AVAR}" | hexdump -C

gives me the newline converted to a space

$ make command2
echo "12 34"
12 34
echo -n "12 34" | hexdump -C
00000000  31 32 20 33 34                                    |12 34|
00000005

Copy link
Contributor Author

@ligfx ligfx Feb 24, 2017

Choose a reason for hiding this comment

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

Good catch! Fixed.

Copy link
Contributor

@webmaster128 webmaster128 left a comment

👍

A solution that came out of discussion on curl#1271. The strange bytes are
used to mark escaped newlines so that they can be converted back after
processing. However, text editors and other programs tend to silently
change these characters in CMakeLists.txt or complain. This is a
cosmetic concern for generated files (which I assume no-one reads), so
let's remove it entirely.
@ligfx ligfx force-pushed the cmake_remove_unicode_shenanigans branch from f5756a6 to 17db8e9 Compare Feb 24, 2017
@jay
Copy link
Member

@jay jay commented Feb 25, 2017

Thanks guys but I went with #1271 instead because the changes are minimal and it doesn't make the output difficult to read. Whether anyone is reading them I don't know.

@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants