Adds PlatformDependent factory.

[bfloch]

EDIT: See last comments for updated version for review.

Allow to specify any value in rezconfig based on platform. Tests and documentation included.

Samples from documentation:

    from rez.utils.config import PlatformDependent
    default_shell = PlatformDependent({
        "linux": "bash",
        "windows": "powershell",
        "*": "zsh",
    })

* can be used as fallback key. By default it matches the platform_.name. This can be changed by specifying attr as any other platform_ attribute:

    from rez.utils.config import PlatformDependent
    default_shell = PlatformDependent({
        "AMD64": "bash",
        "x86": "powershell",
        "*": "zsh",
    },
    attr="arch")

I haven't tried but this could be even chained for more complex rules.

Backwards compatibility

• This is just a factory. All schema validation still works as proven by the unit tests
• No breaks in compatibility. This is just an option, it is not used by default.

[nerdvegas]

Hey Blazej,

Fantastic idea, I like it. I can see how this would simplify setting definitions with nested dicts. I have some suggestions though:

1. I think it's a little confusing to use PlatformDependent, but then specify that it's actually (eg) arch dependent by passing attr='arch'. Also, having to know the available attributes on platform_ is a little too low level. This is just the config, it needs to be pretty basic to use. To that end, I think it'd be good to have have multiple classes instead - PlatformDependent, ArchDependent etc.

2. You should not have to import the class in rezconfig to use it. We should make it available already, as is done with ModifyList. See https://github.com/nerdvegas/rez/blob/master/src/rez/config.py#L814

3. I think it'd be better to pass the default value as an arg, rather than as '*' in the map arg.

4. Lastly, I think we're missing a trick here, this could be more useful as a more general solution! For example:

# in a rezconfig.py
default_shell = Conditional(
    value=os.getenv("STUDIO_LOCATION"),
    default="bash",
    options={
      "LA": "bash",
      "Paris": "zsh"
    }
)

We would still have PlatformDependent etc, but that would just be a specific case of Conditional.

[bfloch]

Very glad you like it. I also had a good feeling about it. All your points are valid. Will look into it tomorrow.

[bfloch]

0. First commit is a bugfix in the current override that I found along the way. Let me know if you prefer a separate PR.

1. & 4. Implemented a Conditional generic version and the derived PlatformDependent, OsDependent and ArchDependent forms. The latter two do not respect platform_map as this would mean a recursion. But I guess we can live with that.

2. These guys are built-ins for rezconfig now.

3. "*" is now a default argument. I had to actually default it to the exception type to raise so that it works with None. This also means that you can control the exception in case this factory is used in other contexts.

+Added tests for using in a config file. This exposed the recursive import mentioned above.

[bfloch]

I was thinking about what would be necessary to make OsDependent and ArchDependent respect platform_map. It would seem that the platform_map would need to load a config in a special mode where only the platform_map key is being extracted, without the implicit conditionals being bound. Not sure if this is doable without making a mess but I'll take a look at it.

[bfloch]

This is doing what one would expect.

However due to the nature of platform_map I need to use inspect. It is still possible (and necessary in the case of config.override) to specify the platform_map explicitly for OsDependent and ArchDependent, but it was important to me that it would just work within a regular config and still use the configs current platform_map (if there is any).

All the different types are visible in the unit tests.

Also removed the platform_mapped decorator which make the code easier.

[bfloch]

Last thing I might do is use the inspect only in the bound class. This way there shouldn't be any wrong results.

[bfloch]

Ok this is much better. Ignore the past discussion. Here an updated descriptions:

• Implemented a generic Conditional version and the derived PlatformDependent, OsDependent and ArchDependent forms.

• The conditionals are built-ins for rezconfig. In order to respect the current config's platform_map, a special InConfig*-variants of the Conditional is used (but exposed under the same name, so their interface matches). This means that the conditionals will respect the platform_map in a case like this:

# This is a rezconfig.py

platform_map = {
    "arch": {
        ".*": "IMPOSSIBLE_ARCH",
    },
}

prune_failed_graph = ArchDependent({
    "IMPOSSIBLE_ARCH": True,
})

• In the case of an config.override you need to make sure to pass the correct platform_map instead (if you require it). The example above would be something like this:

config.override(
    "prune_failed_graph",
    ArchDependent(
        {
            "IMPOSSIBLE_ARCH": True,
        },
        platform_map=config.platform_map
    )
)

• You can pass a default. It can be an exception (which it defaults to) which allows None defaults to work. This also means that you can control the exception in case this factory is used in other contexts. 👍

• Last commit is a bugfix in the current override that I found along the way (plugins would be reset if a non-plugin override came after a plugin override). Let me know if you prefer a separate PR.

• Includes tests.

EDIT:

• Replaced platform_mapped decorator with a method.

[bfloch]

Tests passing

• [x] Windows 10.0.17134 - Python 2.7
  • [x] cmd*
  • [x] PowerShell 5.1.17134.165*
  • [x] pwsh 6.2.2*
  • *fails in test_build_cmake like in master.
• [x] Linux Ubuntu 18.04.2 LTS (WSL) - Python 2.7
  • [x] tcsh
  • [x] sh
  • [x] bash
  • [x] csh
  • [x] pwsh 6.2.1
  • [x] zsh

[maxnbk]

I have some failing tests on Windows in this branch (on py3.7), is this the right place for those? Granted, it's far fewer failing tests than is currently in master, so that doesn't have to be a blocker.

[nerdvegas]

So there's a problem I can see that I think has to be fixed.

While Os/ArchDependent does respect platform_map, it only does so if platform_map is defined in the same config. The problem with this is, it would not be at all unreasonable for a studio to define platform_map in some root rezconfig.py, and then have various overrides in a second rezconfig.py (you can specify multiple, and they get merged together). In that scenario, any OsDependent used in the second rezconfig would not work as expected.

I think I can see a reasonable fix though - it isn't perfect, but good enough I reckon. What we need to do is, in _load_config_from_filepaths, make the config data read so far, available in some reserved variable - the same way that's done in _load_config_py. Then, InspectedDependent would have access to both platform_map if defined in the current config, but also if defined in an earlier config.

I think this should be a fairly trivial fix. It's not perfect in the sense that OsDependent won't be aware of platform_map being redefined in a config later in the searchpath - but I think the chances of that being done are low, and it's not that unreasonable for OsDependent to only know about platform_map as it's been defined up to that point in the config searchpath.

ps - We could also provide a reserved get_config_value function, which does the inspecting of current config and/or previous merged configs, to get the value you're after. I think this would make sense because we could see this pattern again in a different case, and it'd keep all the messy details (frame introspection etc) in the one place.

[bfloch]

I have some failing tests on Windows in this branch (on py3.7), is this the right place for those? Granted, it's far fewer failing tests than is currently in master, so that doesn't have to be a blocker.

The only tests that are expected to fail are the cmake related ones. I will check Python 3.7. Thanks for the hint.

[maxnbk]

These are my other (non-py3-specific) test case failures at the moment. I apologize if any of these are caused by my specific Win10 home system. It doesn't get as much love as it should. ;)

======================================================================
ERROR: test_conditional_in_file (rez.tests.test_config.TestConfig)
Test package config overrides.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 628, in __new__
    return options[key]
KeyError: 'IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 846, in _load_config_py
    exec(code, g)
  File "c:\opt\rez\lib\site-packages\rez\tests\data\config\test_conditional.py", line 19, in <module>
    "IMPOSSIBLE_ARCH": True,
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 700, in __new__
    default)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 689, in __new__
    return base(options, default, platform_map=platform_map)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 659, in __new__
    default=default)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 631, in __new__
    raise default(*e.args)
rez.exceptions.ConditionalConfigurationError: 'IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH' not found in Conditional options

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\tests\test_config.py", line 137, in test_conditional_in_file
    self.assertListEqual(c.plugins.release_hook.emailer.recipients, ["[email protected]"])
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 535, in plugins
    plugin_data = self._data.get("plugins", {})
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 623, in _data
    data = copy.deepcopy(self._data_without_overrides)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 618, in _data_without_overrides
    data, self._sourced_filepaths = _load_config_from_filepaths(self.filepaths)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 893, in _load_config_from_filepaths
    data_ = loader(filepath_with_ext)
  File "c:\opt\rez\lib\site-packages\rez\backport\lru_cache.py", line 96, in wrapper
    result = user_function(*args, **kwds)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 849, in _load_config_py
    % (filepath, str(e)))
rez.exceptions.ConfigurationError: Error loading configuration from c:\opt\rez\lib\site-packages\rez\tests\data\config\test_conditional.py: 'IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH' not found in Conditional options

======================================================================
ERROR: test_conditional_overrides (rez.tests.test_config.TestConfig)
Test configuration with platform conditionals
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 53, in validate
    data = self.schema.validate(data)
  File "c:\opt\rez\lib\site-packages\rez\vendor\schema\schema.py", line 230, in validate
    raise SchemaError('%r should be instance of %r' % (data, s), e)
rez.vendor.schema.schema.SchemaError: 'Wrong Type' should be instance of <class 'bool'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\tests\test_config.py", line 94, in test_conditional_overrides
    platform_name: True,
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 486, in override
    self._uncache(key, keep_plugins=True)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 589, in _uncache
    if key and hasattr(self, key):
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 588, in getter
    return self._validate_key(key, attr, key_schema)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 614, in _validate_key
    return key_schema.validate(value)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 57, in validate
    % (self.key, str(e)))
rez.exceptions.ConfigurationError: Misconfigured setting 'debug_none': 'Wrong Type' should be instance of <class 'bool'>

======================================================================
FAIL: test_execute_command_environ (rez.tests.test_context.TestContext)
Test that execute_command properly sets environ dict.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\tests\test_context.py", line 82, in test_execute_command_environ
    self.assertEqual(parts, ["covfefe", "hello"])
AssertionError: Lists differ: [''] != ['covfefe', 'hello']

First differing element 0:
''
'covfefe'

Second list contains 1 additional elements.
First extra element 1:
'hello'

- ['']
+ ['covfefe', 'hello']

[bfloch]

These are my other (non-py3-specific) test case failures at the moment. I apologize if any of these are caused by my specific Win10 home system. It doesn't get as much love as it should. ;)

...

I think this one is not related to this PR but we can be sure by holding this one off until the Windows tests PR changes are in: #775 #781

I'll be looking at Allan's requests next.

[bfloch]

• Added the suggested Python3 change
• Fixed the bug of repeated keys in the platform map (IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH)
• @nerdvegas Gave your suggestion a try. One detail is I had to introduce a new hashable dict in order to keep the caches. If anyone knows a nicer way to deal with this, please let me know.

If you don't mind let's have the get_config_value as a separate refactoring PR, I haven't gotten my head around how this would work with the suggested Conditional classes.

I am still trying to figure what went wrong with Python 3.x ...

[bfloch]

Should be in a pretty good shape now.

[bfloch]

@maxnbk I fixed the issues you had (I hope). I'll open this up again for review.

[bfloch]

Moved the test to test_utils.

[nerdvegas]

Hey @bfloch, just getting back to this now, apologies for the delay.

There's only a couple of minor changes I would make. I don't mind doing them myself as part of the merge, as they're fairly trivial.

1. I think the fallback_platform_map stuff could be more general. Ie, the _load_config_py and _load_config_yaml files would take a merged_data arg instead, which would make data from previous configs in the config path available to the currently eval'd config (stored as reserved __merged_data for eg). This is less about generalising for use in other cases, and more about making the code easier to understand.

2. I would move the InspectedDependent and derived classes from data_utils.py into config.py, as I think they're too specific to the config to be elsewhere. These classes would also expect to see __merged_data in their current frame rather than __fallback_platform_map. Now I think this would be more obvious - these classes would be checking __merged_data.get("platform_map"), and we'd avoid the more special case fallback_platform_map in several places.

cheers A

[bfloch]

Just rebased and resolved conflict with current master. I'll be looking into your feedback shortly. Sounds minimal and it seems we might be wanting to use this shortly which will also give us some real world feedback.

P.S. Nice how far all of you pushed the GH actions!

[nerdvegas]

Hey cheers Blazej, I'll get back onto this soon.

On Wed, 2 Sep. 2020, 15:49 Blazej Floch, [email protected] wrote:

Just rebased and resolved conflict with current master. I'll be looking into your feedback shortly. Sounds minimal and it seems we might be wanting to use this shortly which will also give us some real world feedback.

P.S. Nice how far all of you pushed the GH actions!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/pull/739#issuecomment-685315987, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOUSWLGEZSDAAZLEACO2TSDXMHVANCNFSM4IU6L54Q .