|
Notes |
|
|
(0005598)
|
|
Brad King
|
|
2006-10-30 10:43
|
|
The problem is actually with CONFIGURE_FILE. In order to deal with newlines in a cross-platform way we've introduced the following policy:
- Any input text is read in binary mode and converted to unix newlines (\\r\\n -> \\n)
- Any output text is written in text mode to produce native newlines automatically
This policy has not been fully swept through the code.
I think CONFIGURE_FILE is currently opening its output in binary mode.
|
|
|
|
(0005599)
|
|
Brad King
|
|
2006-10-30 10:45
|
|
Oops, I meant FILE(WRITE), not CONFIGURE_FILE.
|
|
|
|
(0013910)
|
|
David Cole
|
|
2008-10-20 17:12
|
|
|
Brad King / David Cole discussed via email on October 10, 2008 |
|
|
|
(0018859)
|
|
Brad King
|
|
2009-12-14 12:05
|
|
Changing this behavior will almost certainly break existing projects. Here is what I wrote to Dave Cole on 2008-10-10:
-------------------------------------------------------------------------
I think the only way to satisfy everyone is to add options to get each kind of output. For file(WRITE) it should simply be "LF" or "CRLF". For configure_file() it should be "LF", "CRLF", or "PRESERVE". I guess the APPEND option to the file(WRITE) command could be expected to maintain the convention already found in the file, but perhaps that is overkill. Let's use options like
NEWLINE_STYLE [LF|CRLF|PRESERVE]
or as single options
NEWLINE_STYLE_LF | NEWLINE_STYLE_CRLF | NEWLINE_STYLE_PRESERVE
The default would of course be whatever it does now.
------------------------------------------------------------------------- |
|
|
|
(0025907)
|
|
Torsten Rohlfing
|
|
2011-03-25 16:44
|
|
Has there been any development on this or is there a simple workaround?
I am running into this very problem trying to drive tests of a VisualStudio build using sh scripts executed via Cygwin's bash. The Windows cmake CONFIGURE_FILE creates a script with CRLF, but bash refuses to execute anything but LF-style scripts. |
|
|
|
(0025908)
|
|
Brad King
|
|
2011-03-25 16:53
|
|
Re 0003957:0025907: There has been no progress. As a workaround you can use Cygwin's dos2unix to convert the script after writing it. Alternatively you can write .cmake scripts and execute them with "cmake -P /path/to/script.cmake" to drive your tests. Use the execute_process command in the script when you need to run executables.
Sending to backlog as I have no time to spend on this now. If anyone wants to volunteer to fix this please step forward.
|
|
|
|
(0025913)
|
|
Peter Kuemmel
|
|
2011-03-26 13:12
|
|
I've uploaded a patch for configure_file:
NEWLINE_STYLE [UNIX|APPLE|DOS|WIN32|LF|CR|CRLF|PRESERVE]
I also added UNIX/APPLE/WIN32 because these strings are already set by cmake and
'DOS' because WIN32 is too specific. |
|
|
|
(0025914)
|
|
Peter Kuemmel
|
|
2011-03-26 13:16
|
|
|
The inital bug is fixed in cmake 2.8.4: no Mac line endings in the generated header on Windows. |
|
|
|
(0025920)
|
|
Brad King
|
|
2011-03-28 08:56
|
|
Re 0003957:0025913: Thanks for working on this. I like your proposed interface. Each project can use the terminology its authors prefer. I think the implementation can be much simpler though. I'd prefer that the newlines be written correctly up front rather than reconfiguring the file after the fact.
The COPY_ONLY case should not do any newline modification (and the documentation should say so). The true configure case already hard-codes the newline as "\n" in one place:
while( cmSystemTools::GetLineFromStream(fin, inLine) )
{
outLine = "";
this->ConfigureString(inLine, outLine, atOnly, escapeQuotes);
fout << outLine.c_str() << "\n";
}
The GetLineFromStream method never returns CR or multiple lines. The only reason CRLF ever gets written is because just above that block we open the stream with the default ostream behavior instead of in binary mode. |
|
|
|
(0025941)
|
|
Peter Kuemmel
|
|
2011-03-28 14:21
|
|
GetLineFromStream is buggy it returns " 1\r 2\r 3\r" as one line. Should I fix it?
(OMG backward bug-compatibility ;) ) |
|
|
|
(0025942)
|
|
Brad King
|
|
2011-03-28 14:23
|
|
Don't change that one.
Does anyone ever use pure Mac newlines (CR) anymore? I haven't seen a file like that even once since CMake started in 2000. |
|
|
|
(0025943)
|
|
Peter Kuemmel
|
|
2011-03-28 14:36
|
|
OK, I tought on the Mac each text file uses CR, isn't that the case any more?
Was CR dropped with the switch to OSX and Unix? |
|
|
|
(0025944)
|
|
Peter Kuemmel
|
|
2011-03-28 14:52
|
|
|
Because Apple is famous for simpliy dropping support for legacy stuff I prepare a patch for LF and CRLF support only. |
|
|
|
(0025945)
|
|
Peter Kuemmel
|
|
2011-03-28 15:13
|
|
|
I've uploaded the much simpler patch 0001-add-NEWLINE_STYLE-to-configure_file.patch |
|
|
|
(0025993)
|
|
Brad King
|
|
2011-03-31 15:06
|
|
Re 0003957:0025945: That looks much simpler, thaks. A few more comments:
- Please use our indentation style visible in the rest of the code for new files.
- I don't see how the PRESERVE option works except in the COPY_FILE case where it is ignored anyway. Let's just remove that option unless you want to rewrite the whole function to keep the input file newlines.
- Please make the command argument parsing more strict so that it is an error to have NEWLINE_STYLE followed by nothing or an invalid type. |
|
|
|
(0027833)
|
|
Peter Kuemmel
|
|
2011-11-20 07:43
|
|
A new patch is available at, which addresses all above points:
https://github.com/syntheticpp/CMake/commit/3a44924d3129f0a360208efd6bb5ce75bdd7f93f [^]
- Error when NEWLINE_STYLE is called with no or a wrong style
- int cmMakefile::ConfigureFile()
By default NewLineStyle has an invalid style.
In case of is an invalid style "\n" is used, so it is backward compatible.
- cmConfigureFileCommand
COPY_ONLY must not be called with NEWLINE_STYLE -> backward compatible. |
|
|
|
(0027834)
|
|
Peter Kuemmel
|
|
2011-11-20 08:08
|
|
|
|
|
(0027835)
|
|
Brad King
|
|
2011-11-21 10:26
|
|
Thanks for the new patch. Here are more comments.
- The patch to cmFileCommand is whitespace-only.
- This logic has an off-by-one error because the last index is always one less than the size:
+ if (args[i] == "NEWLINE_STYLE")
+ {
+ if (args.size() > i)
+ {
+ const std::string eol = args[i + 1];
- Since configure_file now opens the output stream in binary mode the default behavior may be different when no style is specified.
- This hunk does not appear releated to the change:
- outLine = "";
+ outLine.clear();
I think the assignment was preferred over the clear because the latter frees memory and causes more allocation later. |
|
|
|
(0027838)
|
|
Peter Kuemmel
|
|
2011-11-21 15:41
|
|
I've fixed all issues, and also added unit tests.
Do you wanna pull the branch as it is, or is there a way to merge the commits into one where the undo-commits will disappear? |
|
|
|
(0027839)
|
|
Brad King
|
|
2011-11-21 15:45
|
|
|
Thanks for those fixes. Please use "git rebase -i" or whatever tool you prefer to rewrite the topic into a clean final form. |
|
|
|
(0027840)
|
|
Peter Kuemmel
|
|
2011-11-21 16:24
|
|
|
|
|
(0027841)
|
|
Brad King
|
|
2011-11-21 16:32
|
|
|
Re 0003957:0027840: You can force push on top of the old branch. However, since the name of the topic branch only shows up in history as part of the merge commit message I can fix it up when I integrate it. |
|
|
|
(0027842)
|
|
Brad King
|
|
2011-11-21 16:36
|
|
|
Our style limits C++ source files to 79 columns. Please revise the new commit to avoid adding lines longer than that in Source/*.cxx. Sorry I forgot to mention that last time. |
|
|
|
(0027843)
|
|
Peter Kuemmel
|
|
2011-11-21 16:57
|
|
Seems you give me the chance to test the forced push.
But first I have to look for my 15' monitor to see the benefit of a 79 column limit ;) |
|
|
|
(0027844)
|
|
Peter Kuemmel
|
|
2011-11-21 17:16
|
|
|
|
|
(0027860)
|
|
Brad King
|
|
2011-11-28 13:51
|
|
|
|
|
(0029442)
|
|
David Cole
|
|
2012-05-09 15:26
|
|
|
Closing resolved issues that have not been updated in more than 4 months. |
|
cc.zip (1,059) 1969-12-31 19:00
0001-Add-NEWLINE_STYLE-option-to-configure_file.patch (16,162) 2011-03-26 13:09