Allow local channel to have the appropriate priority in test environments #3049
Conversation
Bezier89
changed the title
conda_build/build.py
Outdated
| @@ -1735,7 +1735,7 @@ def _construct_metadata_for_test_from_package(package, config): | |||
| # channel_urls is an iterable, but we don't know if it's a tuple or list. Don't know | |||
| # how to add elements. | |||
| config.channel_urls = list(config.channel_urls) | |||
| config.channel_urls.insert(0, local_channel_url) | |||
There was a problem hiding this comment.
This is putting local always behind any configured channels, which is explicitly what @mingwandroid expressed concern about. We need the full logic that we have from the other PR here, as well.
There was a problem hiding this comment.
Per my comment, empirical testing with this PR produces the following behavior:
If a user specifies "conda build -c mychannel -c local":
conda will prioritize packages in mychannel over packages in the local conda-bld folder for the test environment
If a user specifies "conda build -c mychannel":
conda will prioritize packages in the local conda-bld folder over packages in mychannel for the test environment
If a user specifies "conda build -c local -c mychannel":
conda will prioritize packages in the local conda-bld folder over packages in mychannel for the test environment
If I instead use the logic from your other PR here, it does not work. Replacing "local" with local_channel_url causes all packages with local builds to be preferentially picked up in the test environment.
There was a problem hiding this comment.
There are two places that this must be changed. One was in #3038. You've found another place here. I'm asking that you make this place similar with the code from #3038.
I don't question your empirical testing, but I believe that the code you have here will impose undesirable behavior for @mingwandroid where his builds will prefer remote channels over local, and there will be no way to override that behavior. Importantly, "local" is not necessarily the same thing as "local_channel_url" and that may explain your empirical results. There is no code here, as there is in #3038, that translates "local" into what it should be (local_channel_url). There's nothing here, as there is in #3038, that attempts to maintain order - rather, there is only code here that forces a different order.
There was a problem hiding this comment.
My points are:
-
This PR exhibits the behavior @mingwandroid wants.
-
If I put logic similar to the logic in PR put local channel behind user-specified channels, for remote priority #3038 here, it does not work. Under no circumstances does "local" not have top priority, even when passing "-c mychannel -c local". Another way to phrase this is that if I print out the content of config.channel_urls just after this line, the following content produces the following behaviors:
["mychannel", "local", "file:///foo/bar"]
mychannel is preferred for test dependencies over local builds
["mychannel", "file:///foo/bar"]
local builds are preferred for test dependencies over mychannel
If we go with your proposal to put the logic from PR #3038 here, we end up with the second case, since "local" would be replaced with local_channel_url (e.g. "file:///foo/bar"). Truthfully, I don't understand the underlying code well enough to say why that is the case, but that's what it is.
Merge master
|
@msarahan I was able to spend some more time investigating this. The code you wrote in #3038 runs on top of (and after) the code in this PR was getting executed. That it why the odd behavior I mentioned earlier is exhibited, and why #3036 is not actually resolved. I believe the correct fix is to simply remove this code, as the logic from #3038 already comes in and appropriately adds the local channel. Hopefully the passing test coverage also shows that is the case :) This is important functionality for us, so I'd really appreciate it if you could take a look! |
|
Great, thanks for figuring that out. Always happy to see code disappear. Merging. |
|
Hi there, thank you for your contribution! This pull request has been automatically locked because it has not had recent activity after being closed. Please open a new issue or pull request if needed. Thanks! |
@msarahan #3038 does not resolve #3036. I've tested this change and it has the desired behavior:
Without this change, in all 3 cases conda will prioritize packages in the local conda-bld folder over packages in mychannel for the test environment
This change works because if 'local' is present (like in the first example), it stays in the order that it already is in the list, and if 'local' is not present (like in the second example) it gets add with top priority - as implemented by https://github.com/conda/conda-build/pull/3038/files. This is just the missing piece to have the expected behavior in the test phase in addition to the the build phase.