Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --favor-paths rez-env flag #1305

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ColinKennedy
Copy link

@ColinKennedy ColinKennedy commented May 7, 2022

Closes: #1263

Added rez-env cli option, --favor-paths

  • If not provided, does nothing
  • If provided and 1+ path specified, --favor-paths /foo:/bar
    • All packages found within /foo are preferred over packages found in /bar
    • All packages found in /bar are preferred over packages found within packages_path
  • If provided but no paths specified, --favor-paths, [config.local_packages_path] is used. All other logic applies.
  • In all cases, the usual package path rules still apply. e.g. any path provided in --favor-paths must be in packages_path or it won't be used during ordering

Miscellaneous

Introduced SplitPaths to show an alternative way we can be parsing our arguments. If you like that approach, I can add it to --paths as well, which is currently being described in setup_parser but then parsed again, in env.py command.

Question To Reviewers

I was thinking of also including a new rez.config.Config option for favor paths, so that people can set it via environment variable as well. I can think of some situations where this would be really desirable. Would that be okay to add?

What's not in this PR, currently

And now I recall - the problem we ran into when discussing this ages ago, is that orderers can't be stacked. So that could interfere with orderers already in place.

In the original ticket thread, #1263 (comment), @nerdvegas mentions that we may need a kind of "stacked orderer". However I didn't encounter any limitations in practice. ResolvedContext takes a list of package orderers so it should be fine. However please let me know if not and I'll make necessary adjustments.

@sonarcloud
Copy link

sonarcloud bot commented May 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@diegogarciahuerta
Copy link

diegogarciahuerta commented Jan 9, 2023

Hi @ColinKennedy , thanks for this PR, super useful!

We have been testing it in Windows and found one issue that I think should be easy to resolve.

It looks like Rez implements a canonical_path function, that amongst other things, converts the package locations to lowercase for case insensitive filesystems:

location = canonical_path(location, platform_)

canonical_path definition:

if not platform.has_case_sensitive_filesystem:

In FavorPathsOrder you are checking if the package location is in the list of favor paths:
https://github.com/AcademySoftwareFoundation/rez/pull/1305/files#diff-772d1d5249a49e66b7497839f3f80ddb367c98ee0c880a8a3b2edf421ae817feR174

this comparison should be done with canonical_path in mind otherwise the check most likely always fail in Windows. ie. windows drive letters tend to be uppercase.

To solve it there are a couple of obvious options,

  • either in the __init__ of the package ordered,
from rez.utils.filesystem import canonical_path

...

self._paths = [canonical_path(path) for path in paths]

or later on in the compoarison:

from rez.utils.filesystem import canonical_path

...
    def reorder(self, iterable, key=None):
...
        buckets = collections.OrderedDict([(canonical_path(path), []) for path in self._paths])

Hope this helps, I've tested both with our Windows setup and they work as expected.

@JeanChristopheMorinPerso JeanChristopheMorinPerso requested a review from a team as a code owner April 4, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Another way to test packages in dev stage
2 participants