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

file.line on windows not treating unix line endings correctly. #48110

Closed
whytewolf opened this Issue Jun 13, 2018 · 12 comments

Comments

Projects
None yet
5 participants
@whytewolf
Contributor

whytewolf commented Jun 13, 2018

Description of Issue/Question

On windows when using file.line state module. If the file being changed has unix style line endings instead of dos line endings. The entire file can be changed.

Setup

A simple file to change

test1
test2
test3

a simple state.

remove_test2:
 file.line:
  - name: 'c:\test\test.txt'
  - mode: 'delete'
  - match: 'test2'

Steps to Reproduce Issue

(Include debug logs if possible and relevant.)
if the file c:\test\test.txt uses dos line endings it only removes test2 from the file.
however if the c:\test\test.txt files is using Unix line endings the entire file is reduced to a blank file.

Unix line ending version

winion-0:
----------
          ID: remove_conf_line
    Function: file.line
        Name: C:/test/test.txt
      Result: True
     Comment: Changes were made
     Started: 20:02:27.416000
    Duration: 15.0 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -1,3 +0,0 @@
                  -test1
                  -test2
                  -test3

Summary for winion-0
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  15.000 ms

Dos line endings in c:\test\test.txt

winion-0:
----------
          ID: remove_conf_line
    Function: file.line
        Name: C:/test/test.txt
      Result: True
     Comment: Changes were made
     Started: 20:04:08.523000
    Duration: 21.0 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -1,3 +1,2 @@
                   test1
                  -test2
                   test3

Summary for winion-0
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  21.000 ms

This only affects windows minions. Linux minions can handle the file with either line ending.

Versions Report

Salt Version:
           Salt: 2018.3.0

Dependency Versions:
           cffi: 1.10.0
       cherrypy: 10.2.1
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: 1.6.1
       M2Crypto: Not Installed
           Mako: 1.0.7
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: 2.18
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:25:58) [MSC v.1500 64 bit (AMD64)]
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.3
           RAET: Not Installed
          smmap: 2.0.3
        timelib: 0.2.4
        Tornado: 4.5.2
            ZMQ: 4.1.6

System Versions:
           dist:
         locale: cp1252
        machine: AMD64
        release: 2016Server
         system: Windows
        version: 2016Server 10.0.14393  Multiprocessor Free

@whytewolf whytewolf added the ZD label Jun 13, 2018

@whytewolf

This comment has been minimized.

Contributor

whytewolf commented Jun 13, 2018

ZD-2564

@Ch3LL

This comment has been minimized.

Contributor

Ch3LL commented Jun 14, 2018

@whytewolf is this a regression?

also ping @saltstack/team-windows any ideas here

@Ch3LL Ch3LL added the Info Needed label Jun 14, 2018

@Ch3LL Ch3LL added this to the Blocked milestone Jun 14, 2018

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 14, 2018

from https://docs.python.org/3/tutorial/inputoutput.html

text mode, the default when reading is to convert platform-specific line endings (\n on Unix, \r\n on Windows) to just \n.

I would assume about would add platform-specific line endings

def _get_eol(line):
    match = re.search('((?<!\r)\n|\r(?!\n)|\r\n)$', line)
    return match and match.group() or ''

_get_eol will never see the line endings unless the file is open in binary mode. From what I can tell its open in text mode on Unix and Windows PY2 Binary, Windows PY3 Text.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 14, 2018

I have made a few comments about Text Process in a few PR/Issues. Can we start a Discuss PR about what the requirements are and the assumptions. For example py3 introduced a newline option, which is also in PY2 io.open

newline controls how universal newlines works (it only applies to text mode). It can be None, '', '\n', '\r', and '\r\n'. It works as follows:

https://docs.python.org/release/3.2/library/functions.html#open

On input, if newline is None, universal newlines mode is enabled. Lines in the input can end in '\n', '\r', or '\r\n', and these are translated into '\n' before being returned to the caller. If it is '', universal newline mode is enabled, but line endings are returned to the caller untranslated. If it has any of the other legal values, input lines are only terminated by the given string, and the line ending is returned to the caller untranslated.
On output, if newline is None, any '\n' characters written are translated to the system default line separator, os.linesep. If newline is '', no translation takes place. If newline is any of the other legal values, any '\n' characters written are translated to the given string.

Also atomic_open is used which calls
ntf = tempfile.NamedTemporaryFile
Which convert \n if open in text mode to platform default line ending. So original file \n out \r\n. And also does not support the newline option in io.open which is available in PY2 and PY3.

https://docs.python.org/2/library/io.html?highlight=newline%20controls#io.open

@Ch3LL

This comment has been minimized.

Contributor

Ch3LL commented Jun 21, 2018

ping @terminalmage do you have any ideas around the proper way to handle this?

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 21, 2018

See also #46291 (comment)

@whytewolf

This comment has been minimized.

Contributor

whytewolf commented Jun 21, 2018

After much testing, I have not been able to get this to work on any version of salt under windows. As @damon-atkins says this could be a python issue at the core. Is there any way within python standards to handle this on windows as it is handled in Linux?

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 2, 2018

@whytewolf Does the fix in #48380 fix this for you?

@whytewolf

This comment has been minimized.

Contributor

whytewolf commented Jul 3, 2018

Sorry having trouble testing this fix. Apparently, I can't just drop it into salt://_modules since an import was changed. And I'm having trouble getting windows to recognize the updated copy in the directory.

Will try again. just thought I would note to let you know this is on my radar.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 6, 2018

Looks like we might be trying a different fix anyway. This was fixed in develop recently, and the fix is different from the one posted above.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 10, 2018

Ok, there was a larger fix in develop that we are backporting to 2018.3. That is the fix to test @whytewolf, which is PR #48503. Thank you!

@whytewolf

This comment has been minimized.

Contributor

whytewolf commented Jul 12, 2018

That patch looks to fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment