[FIX] Make package resolve request respect case sensitivity -- Windows #689
Conversation
There was a problem hiding this comment.
Unfortunately it can't be the done the way it has been here.
The package repository acts as an abstraction - it hides away the details on how packages are represented and stored. This code is assuming that packages are stored on a filesystem. To that end, this fix must be implemented within the filesystem package repository class.
Further to this - the fix in filesystem repo should only employ use of os.listdir where necessary, and that means 2 things:
- Only check pkg family name for case sensitive (mistmatched case on the root repo path is fine)
- Only use listdir if necessary (ie windows) - we should assume it will be more expensive than the associated os.path.exists call.
|
Hello Allan, Thank you for your feedback. OK got it, I will look into it and make the necessary changes as suggested above. |
|
One more subtlety that should be fixed at the same time: Since paths on windows are case-insensitive, the filesystem package repo filepath should be In other words, currently /sw/packages and /sw/Packages would be getting treated as separate package repos, whereas on Windows they're actually the same. So, there's another bug currently present, it just hasn't caused any problems so far (that I'm aware of!). To keep this clean and explicit, I'd recommend adding a function like |
|
Indeed, I had in mind using |
|
Yeah I meant specifically only up to the repo location, to be clear. Package names must be case sensitive regardless of filesystem. |
|
Hello Allan, with regards to this
If we went ahead and simply lowered the whole path wouldn't that cause other indirect issues? For example by default the packages path is ~/packages and as you mention ~/Packages would be treated the same on Windows but what if we do have a ~/Packages path defined on another platform and then try it to use it on Windows via the same rezconfig or via REZ_PACKAGES_PATH? We should possibly have a warning or something that Windows is case insensitive therefore all package paths will be lowered. Also what if a totally different path using mixed case is used for example /home/Test/myPackages etc? I mean on Windows it will end up working since it ignores case anyway but should we assume the user is aware of this? The issue becomes more complicated since we are now checking the whole path rather than just Packages vs packages or int vs Int meaning that on Windows there will always be a warning since home dir normally uses mixed case... This means on Windows check if the package location path is not lowered (contains uppercase) and if not give a warning that on Windows paths with different case are the same. Unless we take for granted that this is common knowledge. |
|
Package repositories have a `_uid` method they need to implement, which
uniquely identifies the repo. It would probably be enough to lower
self.location on windows in this function. Just doing this will ensure that
~/packages and ~/Packages map to the same repo instance on Windows.
Hth
A
…On Wed, Aug 14, 2019 at 12:32 PM Ira ***@***.***> wrote:
Hello Allan, with regards to this
In other words, currently /sw/packages and /sw/Packages would be getting
treated as separate package repos, whereas on Windows they're actually the
same. So, there's another bug currently present, it just hasn't caused any
problems so far (that I'm aware of!).
To keep this clean and explicit, I'd recommend adding a function like
get_case_canonical_path (or similar) in utils.filesystem, which would do
the lower() in windows but would return path unchanged on other platforms.
If we went ahead and simply lowered the whole path wouldn't that cause
other indirect issues? For example by default the packages path is
~/packages and as you mention ~/Packages would be treated the same on
Windows but what if we do have a ~/Packages path defined on another
platform and then try it to use it on Windows via the same rezconfig or via
REZ_PACKAGES_PATH? We should possibly have a warning or something that
Windows is case insensitive therefore all package paths will be lowered.
This means on Windows check if the package location path is not lowered
(contains uppercase) and if not give a warning that on Windows paths with
different case are the same. Unless we take for granted that this is common
knowledge.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#689>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSV5GR4AXBOABO4U7LTQENVFHANCNFSM4IKGBF4A>
.
|
|
Sounds good, I will remove the warning then, everything else is implemented as discussed above. |
| @@ -784,7 +786,16 @@ def _get_families(self): | |||
|
|
|||
| def _get_family(self, name): | |||
| is_valid_package_name(name, raise_error=True) | |||
| if os.path.isdir(os.path.join(self.location, name)): | |||
| if "Windows" in platform.system(): | |||
There was a problem hiding this comment.
I would simplify:
if os.path.isdir(os.path.join(self.location, name)):
# force case-sensitive match on pkg family, on case-insensitive platforms
if "Windows" in platform.system() and name not in os.listdir(self.location):
return None
|
Please note my changes. I realised there's another case we didn't consider (unversioned packages); it also made sense to abstract case sensitivity into Ps - Please test! |
Description
[Relates to issue #685]
Windows file systems are not case sensitive
Unlike Linux and Unix file systems, Windows paths are not case sensitive this means that requests to resolve packages via
rez-envusing the wrong case will go through and resolve successfully. This can cause potential issues on other platforms where using the same name with a different case for a package is valid.This is not a Python issue as the path check to the package directory returns True since Windows ignores case sensitivity in path names.
Issue
The root of the issue occurs at
src/rezplugins/package_repository/filesystem.pywhere the following check takes place
with location being the packages directories and name the requested package - this on Windows regardless of the case the name is using will return True if the names match.
Considered equal
The issue goes through multiple channels and can be intercepted in a few places for example
src/rez/resolved_context.pywhere the request takes placeWindows - Case insensitive file system
Linux - Case sensitive file system
Solution
This PR solves the issue by ensuring that the requested package exists as a directory in the
any of the packages directories. This is achieved through the use of
os.listdirwhich does give you the directory contents with their actual cases.In an attempt to get the same error message as case-sensitive systems, the fix has been added to src/rez/packages.py file in the get (package) families function. Failure to detect the package during to wrong case will return zero entries resulting in the expected flow.See #689 (review)
Breakdown
Type of change
How Has This Been Tested?
I have tried resolving a series of packages using different case. The results (before, after) are outlined below:
Before
After
Test Configuration:
2.38.2 ([Regression - Version >= 2.39.0] ConfigurationError: Error in Rez configuration under plugins.shell #688), pip 19.2.1Changelog
Point release
Source | Diff
Notes
Add case sensitivity check to prevent issues during package resolve for OSes with case insensitive file systems.
Merged pull requests:
Closed issues: