View Issue Details Jump to Notes ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003957CMakeCMakepublic2006-10-20 08:092012-05-09 15:26
ReporterPeter Kuemmel 
Assigned ToBrad King 
PrioritylowSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionCMake 2.8.7Fixed in VersionCMake 2.8.7 
Summary0003957: FILE(WRITE) produce wrong line endings
DescriptionQt4 uic.exe needed in the patch.

move all files into c:\cc and call

cmake.exe -DKDE_UIC_EXECUTABLE=uic.exe -DKDE_UIC_FILE:FILEPATH=c:/cc/ui_file.ui -DKDE_UIC_H_FILE:FILEPATH=c:/cc/file.h
 -P c:/cc/cm_uic.cmake

The generated file.h has mac line endings!

uic ui_file.ui -o file.h

does not produce mac line endings,
thus it is a cmake bug.

Visual Expres couldn't headle such headers.
TagsNo tags attached.
Attached Fileszip file icon cc.zip [^] (1,059 bytes) 1969-12-31 19:00
patch file icon 0001-Add-NEWLINE_STYLE-option-to-configure_file.patch [^] (16,162 bytes) 2011-03-26 13:09 [Show Content]
patch file icon 0001-add-NEWLINE_STYLE-to-configure_file.patch [^] (9,940 bytes) 2011-03-28 15:12 [Show Content]

 Relationships
related to 0005533closedBrad King CONFIGURE_FILE should keep the line-ending convention of the input file 

  Notes
(0005598)
Brad King (manager)
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 (manager)
2006-10-30 10:45

Oops, I meant FILE(WRITE), not CONFIGURE_FILE.
(0013910)
David Cole (manager)
2008-10-20 17:12

Brad King / David Cole discussed via email on October 10, 2008
(0018859)
Brad King (manager)
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 (reporter)
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 (manager)
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 (developer)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (developer)
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 (developer)
2011-03-28 15:13

I've uploaded the much simpler patch 0001-add-NEWLINE_STYLE-to-configure_file.patch
(0025993)
Brad King (manager)
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 (developer)
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 (developer)
2011-11-20 08:08

I forgot to return in case of wrong NEWLINE_STYLE argument. So please pull
the newline-style branch from gitgub:

https://github.com/syntheticpp/CMake/tree/newline-style [^]
(0027835)
Brad King (manager)
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 (developer)
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 (manager)
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 (developer)
2011-11-21 16:24

OK, created a new branch, with one commit:
   
   https://github.com/syntheticpp/CMake/tree/newline-style-reviewed [^]

And a git question: Is it somehow possible to use the old branch 'newline-style' for the final merge request which only contains the rebased commit?
(0027841)
Brad King (manager)
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 (manager)
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 (developer)
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 (developer)
2011-11-21 17:16

https://github.com/syntheticpp/CMake/tree/newline-style [^]

Is now the most recent one with 79 columns limit.
(0027860)
Brad King (manager)
2011-11-28 13:51

Thanks Peter. I applied the patch mentioned in 0003957:0027844 with some minor tweaks:

  http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=a0874906 [^]

I expect that "ios_base::openmode" will not work on several of the older platforms we support. I'll fix that up as necessary after our nightly builds come in.
(0029442)
David Cole (manager)
2012-05-09 15:26

Closing resolved issues that have not been updated in more than 4 months.

 Issue History
Date Modified Username Field Change
2008-10-10 18:11 David Cole Relationship added related to 0005533
2008-10-20 17:11 David Cole Assigned To Brad King => David Cole
2008-10-20 17:12 David Cole Note Added: 0013910
2009-01-22 14:03 David Cole Priority normal => low
2009-12-10 14:09 David Cole Assigned To David Cole => Brad King
2009-12-14 12:05 Brad King Note Added: 0018859
2011-03-25 16:44 Torsten Rohlfing Note Added: 0025907
2011-03-25 16:53 Brad King Note Added: 0025908
2011-03-25 16:53 Brad King Assigned To Brad King =>
2011-03-25 16:53 Brad King Status assigned => backlog
2011-03-26 13:09 Peter Kuemmel File Added: 0001-Add-NEWLINE_STYLE-option-to-configure_file.patch
2011-03-26 13:12 Peter Kuemmel Note Added: 0025913
2011-03-26 13:16 Peter Kuemmel Note Added: 0025914
2011-03-28 08:56 Brad King Note Added: 0025920
2011-03-28 14:21 Peter Kuemmel Note Added: 0025941
2011-03-28 14:23 Brad King Note Added: 0025942
2011-03-28 14:36 Peter Kuemmel Note Added: 0025943
2011-03-28 14:52 Peter Kuemmel Note Added: 0025944
2011-03-28 15:12 Peter Kuemmel File Added: 0001-add-NEWLINE_STYLE-to-configure_file.patch
2011-03-28 15:13 Peter Kuemmel Note Added: 0025945
2011-03-31 15:06 Brad King Note Added: 0025993
2011-11-20 07:43 Peter Kuemmel Note Added: 0027833
2011-11-20 08:08 Peter Kuemmel Note Added: 0027834
2011-11-21 10:26 Brad King Note Added: 0027835
2011-11-21 15:41 Peter Kuemmel Note Added: 0027838
2011-11-21 15:45 Brad King Note Added: 0027839
2011-11-21 16:24 Peter Kuemmel Note Added: 0027840
2011-11-21 16:32 Brad King Note Added: 0027841
2011-11-21 16:36 Brad King Note Added: 0027842
2011-11-21 16:57 Peter Kuemmel Note Added: 0027843
2011-11-21 17:16 Peter Kuemmel Note Added: 0027844
2011-11-28 13:51 Brad King Note Added: 0027860
2011-12-16 17:12 David Cole Assigned To => Brad King
2011-12-16 17:12 David Cole Status backlog => assigned
2011-12-16 17:13 David Cole Status assigned => resolved
2011-12-16 17:13 David Cole Fixed in Version => CMake 2.8.7
2011-12-16 17:13 David Cole Resolution open => fixed
2011-12-16 17:13 David Cole Target Version => CMake 2.8.7
2012-05-09 15:26 David Cole Note Added: 0029442
2012-05-09 15:26 David Cole Status resolved => closed


Copyright © 2000 - 2018 MantisBT Team