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

Issue 1263 new dev paths options #1270

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

Conversation

frankchen211
Copy link

According to @nerdvegas's suggest, added add_pre_paths and add_post_paths options to rez-env cli.

Related issue:
#1263

Let's say we have a package `helloworld` in D:\dev\helloworld(a pure python package, and no version specified) want to test, but don't want to move it to any package repository or package filters, instead call this package like this:
`rez-env helloworld pkg1 pkg2 ... -d D:\dev`, helloworld will be resolved to D:\dev\helloworld, but pkg1 and pkg2 resolved to a repository path in rez config. or if is also in D:\dev folder.
@sonarcloud
Copy link

sonarcloud bot commented Apr 3, 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 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@frankchen211
Copy link
Author

frankchen211 commented Apr 3, 2022

But this still can't ensure the resloved packages are always picked from the paths added, for testing purpose, we want to pick the packages from we specifed path instead of the highest best resovled versions.

For non version packge, have to do like this or specify a version in that path, otherwise rez will pick the highest version from other repo.

rez-env foo== --add-pre-paths E:\dev

@ColinKennedy
Copy link

ColinKennedy commented Apr 3, 2022

The proper fix for this imo is - implement a new "stacked" package orderer, that can stack the result of
multiple other orderers; - use that to combine existing orderer with a favour-local orderer, if
necessary; - package that into a convenient --favour-local cli option / api func arg
I think the most elegant way to implement the --favour-local
functionality you describe is probably with a new package orderer.

As mentioned in the thread, --add-pre/post-paths isn't a solution because it doesn't give you much functionality that setting REZ_PACKAGES_PATH can't. Maybe there's a case for adding these CLI arguments for other purposes but I don't think it addresses #1263, as you've also said.

@frankchen211
Copy link
Author

As I can see, to solve the problem mentioned in #1263, to allow requiring a package directly from a path foo@/path/to/dev/foo perhaps is the simplest method.

The proper fix for this imo is - implement a new "stacked" package orderer, that can stack the result of
multiple other orderers; - use that to combine existing orderer with a favour-local orderer, if
necessary; - package that into a convenient --favour-local cli option / api func arg
I think the most elegant way to implement the --favour-local
functionality you describe is probably with a new package orderer.

As mentioned in the thread, --add-pre/post-paths isn't a solution because it doesn't give you much functionality that setting REZ_PACKAGES_PATH can't. Maybe there's a case for adding these CLI arguments for other purposes but I don't think it addresses #1263, as you've also said.

@ColinKennedy
Copy link

ColinKennedy commented Apr 4, 2022

As I can see, to solve the problem mentioned in #1263, to allow requiring a package directly from a path foo@/path/to/dev/foo perhaps is the simplest method.

The suggested PR change adds 2 new CLI arguments, --add-pre/post-paths. Under this PR, functionally, this:

rez-env blah --add-pre-paths /before --paths /foo --add-post-paths /after

Is the same as this:

rez-env blah --paths /before:/foo:/after

I don't want to speak for Allan so I'll let him weigh in, but it sounds like he wants to implement a stacked package orderer and wrap that into a CLI argument.

At present, I don't see this PR providing functionality that we can't already get with --paths, alone. IMO it's better to use --paths rather than having several parameters for the same thing.

As it is, this PR still has the same problems as described in #1263 (comment)

@frankchen211
Copy link
Author

Yes, this only makes it a bit more user friendly instead of passing all paths by --paths, since not all TDs/developers/testers know where all rez repo paths are, and some of the paths are computed in production in our pipeline env, that's why added these new two opts, ppl don't need to append a long list of paths for testing.

Since foo@/path/to/dev/foo sounds like not a good idea for Allan, so I revert this part of code.

@nerdvegas
Copy link
Contributor

In the spirit of development by committee (which makes sense wrt the move to aswf) I'm gonna put this one to a vote. Personally I'm tending to not be in favour. As Colin points out you can just use --path instead, it's only slightly more convenient to have --add-pre-paths. The additional args don't break anything either though.

Please cast your vote, I'll check in again in a while.

@ColinKennedy
Copy link

I sympathize with the issues pointed out in #1263 so I would like that workflow to be improved, however I don't think this PR fully solves the problem, I'd rather we wait for a package orderer to see if it covers all of our use-cases.

e.g. I'm imagining something like

rez-env --favor-paths /foo:/bar/thing --paths /foo:/other/path:/bar/thing.

--favor-paths can provide a list of paths to prefer or, if no explicit path(s) are provided but --favor-paths is explicitly included, a default list containing just local_packages_path is passed, instead.

Once we have that functionality in, I suspect this PR won't be needed as the orderer will handle all of our use-cases. My vote is to wait or close this PR and, if the orderer doesn't do what we need, we can always re-open the PR later.

@maxnbk maxnbk requested a review from a team as a code owner November 15, 2022 23:58
@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.

3 participants