(0040878)
Brad King (manager)
2016-04-18 08:25

> Product Version: CMake 3.6

Is this new since 3.5?

(0040881)
Nils Gladitz (developer)
2016-04-18 08:38

Probably not. I just didn't look at older sources.
I think it might actually always have been there ... I've seen odd extra newlines on CDash in the past as well but I haven't looked if the issue is still present / related.

(0040884)
Nils Gladitz (developer)
2016-04-19 02:32

Would it make sense to perform the "\r\n" -> "\n" conversion in kwsys directly? Perhaps right after ReadFile?

Could it be done unconditionally or are there contexts where newlines have to be preserved?

(0040886)
Brad King (manager)
2016-04-19 11:21

Re 0016070:0040884: I'd like to preserve test output and such, so the input may not be the proper place to convert. Another approach is to make sure all the output is done in binary mode rather than text mode. Either way it would help to have an understanding of the various paths that text is taking to end up this way.

(0040887)
Nils Gladitz (developer)
2016-04-19 11:36

I think in context of ctest tests "\r" characters are also already handled:
https://cmake.org/gitweb?p=cmake.git;a=blob;f=Source/CTest/cmProcess.cxx;h=6441886486d4bc4c01202162598634673de2e571;hb=refs/heads/master#l82 [^]

I presume that at least within CMake all places where handling is missing are erroneous and that the places where newlines are explicitly handled should be consolidated.

I know nothing of use cases outside of CMake though.

(0040888)
Brad King (manager)
2016-04-19 11:45

I think KWSys.Process is too low level to do this filtering, especially because that affects non-CMake projects too. It doesn't have any notion of line-wise output, and cannot even assume that the data coming from the child is text.

(0040889)
Nils Gladitz (developer)
2016-04-19 11:57

Maybe one of:
  - make it opt-in through kwsysProcess_SetOption
  - a wrapper for kwsysProcess_WaitForData e.g. kwsysProcess_WaitForText
  - an utility function in cmake (outside of kwys) to be called after kwsysProcess_WaitForData

I think the last one might be the least invasive but might also require additional external state for the process (e.g. in case the last read data ends on \r (in the middle of a newline sequence)?).

(0040891)
Brad King (manager)
2016-04-19 12:42

Re 0016070:0040889: I'd prefer not to add more infrastructure to KWSys.Process because in the long run it should be replaced with libuv or another separate library. Having custom functionality will make that harder.

(0040899)
Ben Boeckel (developer)
2016-04-20 10:04

This was fixed in CTest by hooking up pipes in binary mode. Previously, it had read in binary and written in text which caused the \n to get expanded to \r\n even if it already had a \r in front of it. I don't see anything obvious in the execute_process implementation from a glance though.

(0040900)
Brad King (manager)
2016-04-20 10:19

For reference, the fix mentioned in 0016070:0040899 is:

ctest --launch: write to cout and cerr in binary
https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=29b0c8c3 [^]

(0040901)
Nils Gladitz (developer)
2016-04-20 10:34

As-is there seem to be plenty of individual workarounds and fixes in some but not all cases where kwsys's process code is used.

E.g. that ctest fix looks like it applies to ctest launchers but not e.g. the main ctest process running in dashboard mode with verbose output (where I see these malformed newline sequences currently).

For now I locally worked around it by piping ctest's output though a second process that filters out these sequences.

(0042985)
Kitware Robot (administrator)
2016-06-10 14:29

Resolving issue as `moved`.

This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.

Notes
(0040878) Brad King (manager) 2016-04-18 08:25	> Product Version: CMake 3.6 Is this new since 3.5?

(0040881) Nils Gladitz (developer) 2016-04-18 08:38	Probably not. I just didn't look at older sources. I think it might actually always have been there ... I've seen odd extra newlines on CDash in the past as well but I haven't looked if the issue is still present / related.

(0040884) Nils Gladitz (developer) 2016-04-19 02:32	Would it make sense to perform the "\r\n" -> "\n" conversion in kwsys directly? Perhaps right after ReadFile? Could it be done unconditionally or are there contexts where newlines have to be preserved?

(0040886) Brad King (manager) 2016-04-19 11:21	Re 0016070:0040884: I'd like to preserve test output and such, so the input may not be the proper place to convert. Another approach is to make sure all the output is done in binary mode rather than text mode. Either way it would help to have an understanding of the various paths that text is taking to end up this way.

(0040887) Nils Gladitz (developer) 2016-04-19 11:36	I think in context of ctest tests "\r" characters are also already handled: https://cmake.org/gitweb?p=cmake.git;a=blob;f=Source/CTest/cmProcess.cxx;h=6441886486d4bc4c01202162598634673de2e571;hb=refs/heads/master#l82 [^] I presume that at least within CMake all places where handling is missing are erroneous and that the places where newlines are explicitly handled should be consolidated. I know nothing of use cases outside of CMake though.

(0040888) Brad King (manager) 2016-04-19 11:45	I think KWSys.Process is too low level to do this filtering, especially because that affects non-CMake projects too. It doesn't have any notion of line-wise output, and cannot even assume that the data coming from the child is text.

(0040889) Nils Gladitz (developer) 2016-04-19 11:57	Maybe one of: - make it opt-in through kwsysProcess_SetOption - a wrapper for kwsysProcess_WaitForData e.g. kwsysProcess_WaitForText - an utility function in cmake (outside of kwys) to be called after kwsysProcess_WaitForData I think the last one might be the least invasive but might also require additional external state for the process (e.g. in case the last read data ends on \r (in the middle of a newline sequence)?).

(0040891) Brad King (manager) 2016-04-19 12:42	Re 0016070:0040889: I'd prefer not to add more infrastructure to KWSys.Process because in the long run it should be replaced with libuv or another separate library. Having custom functionality will make that harder.

(0040899) Ben Boeckel (developer) 2016-04-20 10:04	This was fixed in CTest by hooking up pipes in binary mode. Previously, it had read in binary and written in text which caused the \n to get expanded to \r\n even if it already had a \r in front of it. I don't see anything obvious in the execute_process implementation from a glance though.

(0040900) Brad King (manager) 2016-04-20 10:19	For reference, the fix mentioned in 0016070:0040899 is: ctest --launch: write to cout and cerr in binary https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=29b0c8c3 [^]

(0040901) Nils Gladitz (developer) 2016-04-20 10:34	As-is there seem to be plenty of individual workarounds and fixes in some but not all cases where kwsys's process code is used. E.g. that ctest fix looks like it applies to ctest launchers but not e.g. the main ctest process running in dashboard mode with verbose output (where I see these malformed newline sequences currently). For now I locally worked around it by piping ctest's output though a second process that filters out these sequences.

(0042985) Kitware Robot (administrator) 2016-06-10 14:29	Resolving issue as `moved`. This issue tracker is no longer used. Further discussion of this issue may take place in the current CMake Issues page linked in the banner at the top of this page.

Issue History
Date Modified	Username	Field	Change
2016-04-17 17:52	Nils Gladitz	New Issue
2016-04-18 08:25	Brad King	Note Added: 0040878
2016-04-18 08:38	Nils Gladitz	Note Added: 0040881
2016-04-19 02:32	Nils Gladitz	Note Added: 0040884
2016-04-19 11:21	Brad King	Note Added: 0040886
2016-04-19 11:36	Nils Gladitz	Note Added: 0040887
2016-04-19 11:45	Brad King	Note Added: 0040888
2016-04-19 11:57	Nils Gladitz	Note Added: 0040889
2016-04-19 12:42	Brad King	Note Added: 0040891
2016-04-20 10:04	Ben Boeckel	Note Added: 0040899
2016-04-20 10:19	Brad King	Note Added: 0040900
2016-04-20 10:34	Nils Gladitz	Note Added: 0040901
2016-06-10 14:29	Kitware Robot	Note Added: 0042985
2016-06-10 14:29	Kitware Robot	Status	new => resolved
2016-06-10 14:29	Kitware Robot	Resolution	open => moved
2016-06-10 14:29	Kitware Robot	Assigned To	=> Kitware Robot
2016-06-10 14:31	Kitware Robot	Status	resolved => closed

Relationships