AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
938 stars 332 forks source link

rez-test --interactive flag #1224

Open ColinKennedy opened 2 years ago

ColinKennedy commented 2 years ago

I've been using a custom script to get "a Rez environment of a rez-test" for a while but it has flaws. Looking through Rez's list of issues and PRs, it looks like this has been on the menu for a while (https://github.com/nerdvegas/rez/issues/665).

I'd like to implement this work. Full disclosure, I've already written a mostly-working version of this, including unittests (in this forked branch). But before putting in a PR, I wanted to discuss it a bit more. Reading Fede's PR on the topic (https://github.com/nerdvegas/rez/pull/759) was very insightful. However I discovered a some sharp edges related to tests "requires", "on_variants" and the UX of providing an explicit --variant (or not). I think I got a list of expected behavior that is consistent and makes sense though. I'll outline it below.

Given some_package:

name = "some_package"

# ... more package definition here ...

variants = [["python-2", "backports.functools-lru-cache-1"], ["python-3"]]

tests = {
    "foo": {
        "command": "echo foo",
        "requires": ["python-2", "six"],
    },
    "fizz": {
        "command": "echo another",
        "requires": ["python-2", "extra_package-2"],
    },
    "bar": {
        "command": "echo foo",
        "requires": ["python-3"],
    },
    "buzz": {
        "command": "echo buzz",
        "requires": ["python-3", "thing-1"],
    },
    "lastly": {
        "command": "echo lastly",
        "requires": ["python-3"],
        "run_on": "explicit",
    }
}

Here's how I'm thinking that the CLI would behave:

# Gets the combination of "foo & fizz"
rez-test some_package foo fizz --interactive

# Resolves the "bar & buzz" package. "lastly" is ignored because it is "explicit"
rez-test some_package --variant 1 --interactive

# Resolves with "lastly", explicitly
rez-test some_package lastly --interactive

# Fails to resolve because --variant 0 does not match the requires of "lastly"
rez-test some_package lastly --variant 0 --interactive

# Fails to resolve because "foo / fizz" conflict with "bar", which are all default tests
rez-test some_package --interactive

# Fail to resolve because python versions conflict
rez-test some_package foo bar --interactive

# Resolves if all default tests are compatible
rez-test another_package --interactive

To summarize the behavior, I tried to make --interactive work like the actual rez-env behavior.

And the flipside also being true

I'm considering also adding behavior such as "if no test names are given, not all of the default tests are guaranteed to be part of the resolve". Because it could be that not all tests are resolvable and maybe variant "0" resolves 2/4 tests but variant 1 resolves 3/4 tests. In either case, you're missing at least one test but at least you get a resolve back. The logic does not do this currently, but that may be a consideration. Maybe people will think that's a good idea.

Special consideration is given for "on_variants". For example if 2+ tests have a common variant that resolves and the user doesn't specify an explicit --variant, a matching resolve is returned. However if they do specify --variant and it is invalid, the resolve conflicts and fails.

What do you think of this behavior? I can make a PR with the changes as-is but I figured it'd be better to discuss the exact behavior in a discussion thread prior to opening a PR, in case others have suggestions.

nerdvegas commented 2 years ago

Generally I think this is fine, but there's a caveat:

You can't explicitly select variants. Even if you specifically know the variant index you want, rez just doesn't work that way (yet). You can only influence the variant it selects by changing the request. rez-test already has an --extra-packages arg, which you can use to do this. So, drop all the variant-specific stuff from this. Explicit variant selection is a much bigger topic than rez-test itself, we definitely don't want to be bringing that into the mix here.

ColinKennedy commented 2 years ago

Edit: Re-read your post and now I get what you were asking for. I'll use --extra-packages in place of --variant and keep all the other functionality as planned. Thank you!