Better and platform agnostic path handling in commands

[bfloch]

I think we have two goals:

1. Express and append paths in a native form

• Common case to express paths properly depending on platform
• Should use the native os.pathsep for appending

2. Convert/Enforce to a non-native form after expansion

• Case where env variable is either not representing a PATH or has non-native expectations
• This may mean the need to control the os.pathsep (which we have already)

For better illustration here samples of non-handled cases and their intentions:

def commands():
    # Case 1.
    env.PATH.append("{root}/bin")     # This really should be "{root}\\bin" on some platforms

   # Case 2.
   env.LANGUAGE_COMMENTS.append(r"//")    # This does not represent a path, should be literal
   env.CMAKE_MODULE_PATH.append("{root}/cmake")    # Tricky: This path needs to use forward slashes and ; regardless of platform.

Solution A

One way to deal with 1. is to make the native path conversion implicit.

So imagine that we assumed any environment variable is by default a path that should be native.
Our sample would need to be explicit in the 2. cases:

# We got this already. Necessary in rezconfig / package for case 2. later on
with scope("config") as c:
    c.env_var_separators = {
        "CMAKE_MODULE_PATH": ";",
    }

def commands():
    # Case 1.
    env.PATH.append("{root}/bin")     # Converts to native path with os.pathsep

   # Case 2.
    env.LANGUAGE_COMMENTS.append(literal(r"//"))    # Literal with os.pathsep
    # This needs special treatment.
    # Can't use str.replace() since it must happen after expansion(?)
    env.CMAKE_MODULE_PATH.append(unixpath("{root}/cmake")    
    #                                   ^ we don't have this

Solution B

The alternative solution would be to always be explicit, and assume literal otherwise:

# Necessary in rezconfig / package for case 2. later on
with scope("config") as c:
    c.env_var_separators = {
        "CMAKE_MODULE_PATH": ";",
    }

def commands():
    # Case 1.
    env.PATH.append(nativepath("{root}/bin"))     # Can't use os.path.join

   # Case 2.
    env.LANGUAGE_COMMENTS.append(r"//")    # No special treatment

    # Still need to handle this case
    env.CMAKE_MODULE_PATH.append(unixpath("{root}/cmake")    
    #                                   ^ we don't have this

Solution C

This is an extension of Solution A, but we give the user control over the behavior in rezconfig so we could even act as Solution B.

Like before we implement three additional bindings:

• nativepath -> Converts to native path
• unixpath -> Converts to forward slashes
• windowspath -> Converts to backwards slashes

But we add a definition in config that handles paths.
Similar to the env_var_separators environment variables do have special types and I wouldn't mind this information to be hidden away in config, especially since we can always override it per package.

# rezconfig.py

env_var_separators = {
        "CMAKE_MODULE_PATH": ";",
}

# Tries functions, else uses default
env_var_default_functions = ["nativepath"]    
env_var_functions = {
    "CMAKE_MODULE_PATH": ["expandvars", "unixpath"],    # Accepts list for chaining
    "LANGUAGE_COMMENTS": ["literal"],
}

Our packages become idiomatic:

def commands():
    # Case 1.
    env.PATH.append("{root}/bin")     # Assumes native path

   # Case 2.
   env.LANGUAGE_COMMENTS.append(r"//")    # Handled by rezconfig
   env.CMAKE_MODULE_PATH.append("{root}/cmake")    # Handled by rezconfig

And we should be able to express any other case in rezconfig or the package directly.

[bfloch]

EDIT:
nativepath is wrong as default. I probably mean ["expandable", "nativepath"].

Hm, I guess I am still a little confused about the default behavior so take the proposal with a grain of salt.

[nerdvegas]

I agree with the general direction of C, but I'd be up for simplifying it a little, and generalising the settings (which will make more sense below). RE env_var_functions: I don't think we should have this 'chaining' - expandvars should always be implied (knowing that expansion won't happen if literal() is used). Exposing 'expandvars' to the user like this is confusing IMO. So instead I would just have a single function name for each entry - "nativepath", "unixpath" etc ("posixpath" instead?) - implying that these all do variable expansion anyway. RE generalization. So there is a need to give more control over env-vars in general. Right now for eg, append/prepend always works in set-if-first-then-append mode - ie, PYTHONPATH gets set the first time it's appended/prepended, then appended/prepended on all subsequent ops. We should have a way of refining this behaviour - for eg, a useful default would be, "append/prepend, but retain the original, pre-rez-env value as well, and keep it appended to the result". You could have other modes as well, like "retain" (which would do what the "parent_variables" setting does currently). In any case, without going too much into those details, I think the settings need to be structured in a way that promotes this idea of more env var control, and we could start doing this now. What I mean is, instead of this: env_var_separators = { "CMAKE_MODULE_PATH": ";", } env_var_default_functions = ["nativepath"] env_var_functions = { "CMAKE_MODULE_PATH": ["expandvars", "unixpath"], # Accepts list for chaining "LANGUAGE_COMMENTS": ["literal"], } We would have: env_var_settings = { "*": { "path_modify_op": "nativepath" # ie append, prepend }, "*_PATH": { "set_op": "nativepath" # eg, so "env.MAYA_PLUGIN_PATH = ..." uses nativepath }, "CMAKE_MODULE_PATH": { "pathsep": ";", "path_modify_op": "unixpath" } } Note that the globbing would allow for less verbose settings, by sharing settings across vars (eg, "CMAKE_MODULE_PATH" would inherit settings from "*", then "*_PATH", in that order. Which actually isn't useful in this specific example, but you get the idea). Lastly - I don't know if you already implied this or not, but nativepath() should be able to take any sensible path syntax (posix or windows) and convert it to the native equivalent. That way, studios can just default use whichever they are comfortable with. We should check for ambiguous cases (like "/hey/this\\is\\weird"), and raise an error for those. Do we also need to deal with drives in Windows (eg C://), is that even still a thing? I have no idea.

…

On Sat, Aug 31, 2019 at 12:48 AM Blazej Floch ***@***.***> wrote: Literal is wrong as default. I probably mean expandvars. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#707>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSVSMCMLKL5MNDEKQ5TQHEXLLANCNFSM4ISODKGQ> .

[bfloch]

I agree. Thanks for taking the proposal further.

On Wildcards and inheritance

I wonder how the order would work - I can imagine this brings up some challenges, like:

{
  "*": { ... },        # A
  "*_PATH": { ... },   # B
  "HELLO_*": { ... },  # C
}

In this case what would the the right initial setting given HELLO_PATH and the fact that dicts are unordered?

Maybe a better approach would be to have ordered settings with explicit inheritance (although not as nicely looking):

OrderedDict([
  ("CMAKE_MODULE_PATH", {...} ),  # D
  ("*_PATH", {...} ),             # B
  ("HELLO_*", {                   # C
      "inherits": "*",
      ...
  })
  ("*", {...} ),                  # A, usually at the end.
])

Then the rule is:

• If it has the explicit key use it (e.g. env_var_settings["CMAKE_MODULE_PATH"])
• Else iterate the keys and cancel if a match is found (can skip non wildcards)
• If there is inheritance, then follow it and apply in reverse order.

The benefit is now we can express both inheritance and explicit rules that don't want to inherit.

Looking at this I wonder how strong the relationship between variables is. Maybe we don't need inheritance at all, due to the complexity involved.

It shouldn't be too much work to have separate settings for each special case. Since it is python you could also define you little helpers variables.
But I would be fine with having inheritance if there are strong opinions about it.

I'll put some more thought into the ops and try to come up with all the crazy cases that we might want to handle across platforms.

[nerdvegas]

I would avoid the ordered inheritance for two reasons - 1) it just looks odd as you mention, but 2) it forces you to put things in order that are not related (FOO and BAH for eg). In terms of inheritance - we at least need to specify global defaults, and perhaps that, combined with specific env-var overrides, is enough. What we could do is use the schema I've already suggested, but support only full env-var names as keys, or "*" (which is the global defaults). Doing it this way also means that we don't break compatibility if we decide later that we do indeed want more fine-grained inheritance as I described. If we did want inheritance, then a fairly simple set of rules can be used to determine order: * Globbed before non-globbed * shorter before longer * lastly, alphabetical Thus "*" would be used, then "PATH_*" to override, then "FOO_PATH" to override again. Ambiguous cases like "PATH_*" and "*_HELLO" would just fall back to alphabetical (ie arbitrary but deterministic). In summary, I'd be happy to use suggested schema but support only '*' and full env-var names, leaving the option for further inheritance open in case we decide we want it. Thx A

…

On Thu, Sep 5, 2019 at 5:13 AM Blazej Floch ***@***.***> wrote: I agree. Thanks for taking the proposal further. On Wildcards and inheritance I wonder how the order would work - I can imagine this brings up some challenges, like: { "*": { ... }, # A "*_PATH": { ... }, # B "HELLO_*": { ... }, # C } In this case what would the the right initial setting given HELLO_PATH and the fact that dicts are unordered? Maybe a better approach would be to have ordered settings with explicit inheritance (although not as nicely looking): OrderedDict([ ("CMAKE_MODULE_PATH", {...} ), # D ("*_PATH", {...} ), # B ("HELLO_*", { # C "inherits": "*", ... }) ("*", {...} ), # A, usually at the end. ]) Then the rule is: - If it has the explicit key use it (e.g. env_var_settings["CMAKE_MODULE_PATH"]) - Else iterate the keys and cancel if a match is found (can skip non wildcards) - If there is inheritance, then follow it and apply in reverse order. The benefit is now we can express both inheritance and explicit rules that don't want to inherit. Looking at this I wonder how strong the relationship between variables is. Maybe we don't need inheritance at all, due to the complexity involved. It shouldn't be too much work to have separate settings for each special case. Since it is python you could also define you little helpers variables. But I would be fine with having inheritance if there are strong opinions about it. I'll put some more thought into the ops and try to come up with all the crazy cases that we might want to handle across platforms. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#707>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSRGMQTWFKY4EUHLQDDQIACEZANCNFSM4ISODKGQ> .

[bfloch]

@nerdvegas - I agree with everything you said.
I'd prefer the first pass implementation (* and specific vars inheriting it) for now and then we can expand on it like you proposed. Maybe it is not that much work to begin with doing the full thing. We'll see.

Let's open the next and bigger can of worms:

Settings and Ops

This is a rather complex topic since we want to support so many use cases so I'll build up to the suggestion based on every possible use case imaginable. It also gives us opportunity to migrate existing settings in a generic configuration.

I don't want to jump into implementation details yet (hope this is doable at all :) ), but let's see if we
can come up with use cases best demonstrated via an sample.

References

#703
#698
#661
(more ?)

Use cases

(cases handled by the sample are crossed)

• Path operations:
  • Handle paths in platform independent fashion
  • Handle paths in explicit platform dependant fashion
  • Handle differences in Drive handling (C:\ -> C:/ vs /C/) (Needed?)
  • Handle platform dependent path prefix (C:\path -> /mnt/c/path) (Needed?)
• Define platform dependent settings
• Migrate all_*_variables
• Define path separators (like ; and :)
• Stable de-duplication of separated values (Needed?)
• Implicit variables, system variables and there placement with context variables

Sample

(Note: This is a completely artificial example to showcase many possible use cases)

env_var_settings = {

    # -------------------------------------------------------------------------
    # Global control inherited(!) by all other variables.

    "*": {

        # Migration of all_parent_variables
        #
        # Full control over variables is achieved given the following parent_variables & actions:
        #
        # clear (No parent variables)
        #  [prepend_all] [prepend_parent->FAILS] [append_parent->FAILS] [$REZ_VARS] [append_all]
        #
        # prepend (parent before rez variables)
        #  [prepend_all] [prepend_parent] [$PARENT_VARS] [append_parent] [$REZ_VARS] [append_all]
        #
        # append (parent after rez variables)
        #  [prepend_all] [$REZ_VARS] [prepend_parent] [$PARENT_VARS] [append_parent] [append_all]
        #
        "parent_variables": "clear"

        # Migration of all_resetting_variables
        # Note: Actually had a problem with understanding the naming, but kept for consistency
        "resetting_variables": False

        # Any variable (!!!) on this level may be "its expected type" OR dict of 
        # ("platform": "its expected type").
        # Note we are only handling platform.system differences.
        "pathsep": {
            "windows": ";",
            "linux":   ":",
            # Any other platform, except specific one …
            "*":       "🍎",
        },
        # Default append, prepend and set operation
        "modify_op": "nativepath",
        # Interestingly this could now also be implemented as followed :-)
        # but we probably still need nativepath on commands() side.
        "modify_op": {
            "windows": "windowspath",
            "*": "posixpath",
        },
    },

    # -------------------------------------------------------------------------
    # Creation of variables without having to rely on bootstrap file or package
    # (Studio must make decision of when to use implicit packages vs. rezconfig)

    # Set custom variables without package.

    "STUDIO_REZCONFIG_VERSION": {
        "parent_variables": "clear",
        "prepend_all": "2019.08.23.2",
    },

    "STUDIO_STORAGE": {
        "parent_variables": "clear",
        "prepend_all": {
            "windows": "P:\\",
            "*":  "/mnt/production",
        }
    },

    # Sample where a user might run a local STUDIO_CACHING_SERVER
    # [Parent] [This] [Rez]
    "STUDIO_CACHING_SERVERS": {
        "parent_variables": "prepend",
        "append_parent": ["192.168.2.2", "cache.studio.com"], # Note multiple things as list
    },

    # Sample where user might override for debugging, rez is default authority but
    # ultimately use sensible default
    # [Parent] [Rez] [This]
    "STUDIO_LOCATION": {
        "parent_variables": "prepend",
        "append_all": "UNKNOWN",
    },

    # Artificial case where REZ packages always have priority, but we guarantee a fallback
    # coming from Parent Environment or This config
    # [Rez] [Parent] [This]
    "STUDIO_USER_GROUPS": {
        "parent_variables": "append",
        "append_parent": {
            "windows": ["UPDATING"],
            "linux": ["WORKING"],
            "osx": ["BROKE"],
        }
    },

    # -------------------------------------------------------------------------
    # Common use cases
    # We now can control the "always-on" tools and their order very precisely

    "PATH": {
        # In this sample: Rez first
        "parent_variables": "append",

        "prepend_all": {
            "windows": "C:\\ImportantTools",
            "*": "/opt/important_tools"
        },

        # Here after the system paths
        "append_all": {
            "windows": ["C:\\WindowsFallbackTools"],
        },
    },

    "PYTHONPATH": {
        "parent_variables": "clear",
    },

    "CMAKE_MODULE_PATH": {
        "parent_variables": "clear",

        # Nice.
        "pathsep": ";",
        "modify_op": "posixpath",
    },

    "PATHEXT": {
        "pathsep": ";",
        "parent_variables": "prepend",
        "append_parent": {
            # Does not pollute other platforms
            "windows": [".PY", ".PS1"],
        }
    }
}

Naming, features and details are like always up for discussion. Let me know if I missed anything.

[bfloch]

I'll close this in favor for #737 which has a broader context.