os.path.dirname not doing what you think in file.makedirs_ if the path excludes a trailing slash

[mikejford]

I just ran a simple test of what's going on in makedirs_ because I couldn't understand why a directory wasn't being created on the minion. The sample below shows a directory I know to exist on the host, and the command that is being used in makedirs_ to assign the path to dirname. Note that I did not include the trailing slash on the path.

Python 2.6.6 (r266:84292, Jan 22 2014, 09:42:36)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.normpath(os.path.dirname('/etc/salt'))
'/etc'
>>>

The message to the user is wrong, since dirname was the value that was tested by os.path.isdir but path is the value sent back to the user in the return message.

dirname = os.path.normpath(os.path.dirname(path))

    if os.path.isdir(dirname):
        # There's nothing for us to do
        return 'Directory {0!r} already exists'.format(path)

I'm not sure what would be preferable here as a solution:

1. Change documentation to reflect that path must include the trailing slash?
2. Append a trailing slash to path and expect os.path.normpath to fix any double slashes?
3. Change the return message formatting to report on the actual value tested?

[basepi]

Ooh, good catch. At the very least we should do 3, but we should probably take additional steps as well. Can you clarify, just for completeness, which version of salt you're looking at, and where those lines of code come from? Thanks!

[mikejford]

We're on 2014.1.5 on the master and minion in question. Those lines are from modules/file.py in the makedirs_ function (lines 2818-2827 in the version in the copy of the source from EPEL).

[basepi]

Thanks. I'm going to run some tests and look at this later.

[nmadhok]

@basepi We can check if the path that the user enters ends with a trailing slash or not. If it does then we have no problem but if it doesn't then we can append a trailing slash to it and that's all.

Python 2.7.7 (default, Jun 14 2014, 23:12:13) 
[GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.normpath(os.path.dirname('/etc/salt'))
'/etc'
>>> os.path.normpath(os.path.dirname('/etc/salt/'))
'/etc/salt'
>>> 

Do you want me to work on this?

[mikejford]

@nmadhok Try the case where the directory doesn't exist and you omit the trailing slash i.e. /etc/foo

It will report that Directory '/etc/foo' already exists.

[nmadhok]

@mikejford Yeah i just figured. Working on this. Thanks for finding the bug!

[basepi]

Thanks for tackling this, @nmadhok !

[nmadhok]

Here try this @mikejford!
@basepi No problem. Ready to be merged ;)

Fix:

Python 2.7.7 (default, Jun 14 2014, 23:12:13) 
[GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.normpath(os.path.dirname('/etc/salt'))
'/etc'
>>> os.path.normpath(os.path.dirname('/etc/salt/'))
'/etc/salt'
>>> os.path.normpath(os.path.dirname(os.path.join('/etc/salt', '')))
'/etc/salt'
>>> os.path.normpath(os.path.dirname(os.path.join('/etc/salt/', '')))
'/etc/salt'

Note that the following works too:

Python 2.7.7 (default, Jun 14 2014, 23:12:13) 
[GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.normpath(os.path.dirname('/etc/salt'))
'/etc'
>>> os.path.normpath('/etc/salt')
'/etc/salt'
>>> os.path.normpath('/etc/salt/')
'/etc/salt'

Both fixes won't get the directory path in case the user gives the path as /etc/salt/foo.txt

The fix to that is to change the order:

Python 2.7.7 (default, Jun 14 2014, 23:12:13) 
[GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.normpath(os.path.dirname('/etc/salt'))
'/etc'
>>> os.path.normpath(os.path.dirname('/etc/salt/'))
'/etc/salt'
>>> os.path.normpath(os.path.dirname(os.path.join('/etc/salt', '')))
'/etc/salt'
>>> os.path.normpath(os.path.dirname(os.path.join('/etc/salt/', '')))
'/etc/salt'
>>> os.path.normpath(os.path.join(os.path.dirname('/etc/salt/foo.txt'), ''))
'/etc/salt'

[mikejford]

@nmadhok I think you got something out of order in your sample above. There's three closing parens in the samples that show the dirname, and two closing parens in the sample with the path with a file.

When you run the last with a dirname as a path, it cuts the last value from the string still.

Python 2.6.6 (r266:84292, Jul 10 2013, 22:48:45)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.normpath(os.path.join(os.path.dirname('/opt/code'), ''))
'/opt'
>>>

[mikejford]

@basepi I think there's a fundamental question about what the action of makedirs should be. If it's supposed to replicate the functionality of mkdir -p from unix, then the name argument should simply be considered a full directory path that is desired. There shouldn't be a consideration of filenames at all except for the case that the node already exists as a file type.

[nmadhok]

@mikejford You're right. I didn't test the directories with os.path.normpath(os.path.join(os.path.dirname('/etc/salt/foo.txt'), '')) but instead tested them with os.path.normpath(os.path.dirname(os.path.join('/etc/salt/', '')))

The correct way to go i feel should be using os.path.normpath(os.path.dirname(os.path.join('/etc/salt', '')))

The user shouldn't specify path to a file but the directory path itself.

[mikejford]

@nmadhok The merged code doesn't use the version you believe is the correct form. That's where I'm still taking issue.

[nmadhok]

@mikejford Changing it right away. Thanks for pointing it out.

[nmadhok]

Changed the docs. #14488

[nmadhok]

@basepi This issue can be closed!