normalize paths in CMAKE_PREFIX_PATH for proper comparison

[kejxu]

CMAKE_PREFIX_PATH uses forward slash on all platforms, but os.path.dirname() is platform dependent. this problem is not noticeable on Linux/Ubuntu since os.path.sep is also /, yet os.path.dirname on Windows would contain \ in the returned result; leading to mismatch during comparison. Normalizing paths in CMAKE_PREFIX_PATH would get rid of this issue by using the same separator in both path strings

meantime, \ in base_path needs to be converted to / to match other paths already in CMAKE_PREFIX_PATH

[dirk-thomas]

I am a bit concerned about the use of os.path.normpath here since it is not safe to be used in the potential presence of symlinks. How about converting base_path immediately from os.path.sep to / and then keepig the simple check if it is already in CMAKE_PREFIX_PATH?

[kejxu]

thanks for raising this concern! I will first try base_path.replace(os.path.sep, '/') and see if it's going to cause any problem.
Meantime, there are a few reasons (in my opinion) why os.path.normpath would be okay:

• it would help collapse redundant separators. since os.path.dirname() should return normalized path, this might help prevent duplicated items if CMAKE_PREFIX_PATH was changed manually (so it contains redundant separators)
• for cases where symlinks are involved, this would become comparison between the actual path and base_path (am I right?). maybe this would actually help prevent duplicated paths

[dirk-thomas]

it would help collapse redundant separators.

The problem is that normpath only performs a fairly simple string manipulation. In case where the path contains .. the result might actually be wrong. See https://docs.python.org/2/library/os.path.html#os.path.normpath

I would also argue that having redundant separators in the CMAKE_PREFIX_PATH is wrong in the first place and should be avoided at the root (where they are added when that is actually the case). Not every place which uses it should try to work around such data.

maybe this would actually help prevent duplicated paths

Are you actually seeing such duplicates or is this purely hypothetical?

[kejxu]

I cannot recall seeing such duplicates recently, so this is majorly just hypothetical. Will remove the use of os.path.normpath from this change, and open another ticket if there actually is a repro of duplicated paths in CMAKE_PREFIX_PATH.

should we still keep the use of os.path.normcase in this change? based on the python document, with the help of this we don't have to do the os.path.sep replacement

os.path.normcase(path)
Normalize the case of a pathname. On Unix and Mac OS X, this returns the path unchanged; on case-insensitive filesystems, it converts the path to lowercase. On Windows, it also converts forward slashes to backward slashes.

[dirk-thomas]

I would rather not use the normcase function for the purpose of making the path lowercase.

The separator conversion I would do differently. Currently the logic is:

• base_path set with platform separator
• while iterating CMAKE_PREFIX_PATH convert each item to platform separator to compare with base_path
• convert base_path to forward slashes and add to CMAKE_PREFIX_PATH

I would suggest changing it to the following which is more efficient and less "backwards" with the logic:

• base_path set with platform separator
• convert base_path to forward slashes
• compare base_path to each item in CMAKE_PREFIX_PATH - without the need for conversion
• add base_path to CMAKE_PREFIX_PATH - again without the need for conversion

[kejxu]

thanks! will change to the suggested logic

[dirk-thomas]

Thanks for the patch and for iterating on it.