rez icon indicating copy to clipboard operation
rez copied to clipboard

Issue 1263 new dev paths options

Open frankchen211 opened this issue 2 years ago • 8 comments

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

Related issue: #1263

frankchen211 avatar Apr 03 '22 08:04 frankchen211

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

sonarqubecloud[bot] avatar Apr 03 '22 08:04 sonarqubecloud[bot]

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

frankchen211 avatar Apr 03 '22 09:04 frankchen211

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 https://github.com/nerdvegas/rez/issues/1263, as you've also said.

ColinKennedy avatar Apr 03 '22 16:04 ColinKennedy

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.

frankchen211 avatar Apr 04 '22 03:04 frankchen211

As I can see, to solve the problem mentioned in https://github.com/nerdvegas/rez/issues/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 https://github.com/nerdvegas/rez/issues/1263#issuecomment-1081422278

ColinKennedy avatar Apr 04 '22 03:04 ColinKennedy

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.

frankchen211 avatar Apr 04 '22 07:04 frankchen211

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.

nerdvegas avatar Apr 19 '22 11:04 nerdvegas

I sympathize with the issues pointed out in https://github.com/nerdvegas/rez/issues/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.

ColinKennedy avatar Apr 19 '22 16:04 ColinKennedy