Updates to support arbitrary channel platforms.#934
Conversation
|
Hey, thanks for this PR! Sounds awesome. Do you think it'll be hard to fix the failing tests? |
|
Well, you might notice I had to remove a couple of tests that involved testing with non-existent channel locations. The "breaking" change here is that you do always need to use channels that actually exist, since it doesn't rely on the predefined set of platform names to use as separators when parsing channels. I had the cmake tests working, I guess I missed some of the other test suites :) I can take a look. |
|
I think maybe a few of the micromamba tests are failing because there's no network connection? This brings up a good point, albeit not much of an issue: if you run with |
| # - reproc-static | ||
| # - yaml-cpp-static | ||
| # - xz-static | ||
| # - zstd-static |
There was a problem hiding this comment.
These were all the packages required to build a static micromamba. There were also a few other tweaks I had to do to get it to build. What is your process of building a static micromamba?
There was a problem hiding this comment.
Also, these could probably be removed or moved to a different environment file.
There was a problem hiding this comment.
Hey! Have a look at recently added https://github.com/mamba-org/mamba/blob/master/environment-static-dev.yml
|
@wolfv Do the umamba tests not run with network access? If you look at the new code where
is emitted, that should only happen if https://conda.anaconda.org/conda-forge/channeldata.json can't be fetched. |
|
@afranchuk the test that is currently failing is a rather basic test for installing from a yaml file ... there should be internet connection. Could it be an issue with loading the files from cache? I can re-run the tests to make sure that it's not related to some anaconda.org outage. |
|
It shouldn't be a caching issue, the cache certainly isn't required. Odd, something else must be going on... |
|
It looks like the error is that the SSL certificate is failing verification (curl error 60). |
|
All right, so one thing is that I added a static method to |
|
I guess I can change it to a GET, but that is kind of wasteful... |
|
I changed the code to try a GET if the HEAD fails. This should be as fast as we can go, and it will be cached so realistically it will be a one-time cost per (misbehaving) url. |
|
Hi @afranchuk, I had some time to dig deeper into this PR and the functionality. In fact, I'd like to refactor the entire DownloadTarget thing a bit in the future anyways. I have some thoughts: I think with this setup here, we still don't have the ability to "layer" platforms, right? One could argue that e.g. "linux-64" and "noarch" are two layered platforms (similar to how channels would be different layers).
and that would fetch those three platforms. On the contrary we might want to explicitly disable noarch. We could either require it with the syntax above (so that I also wanted to make you aware that with micromamba we already support a We could modify the I also want to note that Works: https://conda.anaconda.org/robostack/osx-arm64/ros-noetic-laser-geometry-1.6.7-py38h0184f97_0.tar.bz2 Thanks for the heads-up on the failing HEAD requests on repo.mamba.pm. I think the problem is that for these files we redirect with a 301 to an authenticated S3 file location. We can look into that. However, given that channeldata.json doesn't even fully work on anaconda.org maybe we should require explicitly specifying platforms? Would you be interested in discussing this PR over video? It might be faster to discuss this by voice? Sorry for taking so much time to write up these thoughts. |
|
Thanks for the feedback. As far as layering goes, I was just imagining that as I'm okay with a different syntax for providing platforms, I was just trying to keep it roughly backward-compatible with prior usage. I think one (small) issue with the syntax you suggested is that (at least as you literally suggested) it is not shell-friendly (without quotes). But naturally excluding the spaces makes it a bit nicer to work with (e.g. The lack of consistency around Unrelated, I didn't realize that a libcurl GET by default writes to stdout (though in retrospect that makes sense), which is what caused the tests to fail again. Looks like it did correctly identify the repositories this time around though, the extra output just confused the final test checks. I pushed a fix to silence that. |
|
Yeah, discussing directly may be a bit faster. But given the inconsistency of server behavior, it does seem like explicitly providing the platforms with additional syntax ("breaking" command-line API for the old |
|
@wolfv looks like it's working now, but to work better we have two options:
|
a url resource exists.
|
Hi @afranchuk to be honest, I lean towards the explicit option. Would you be OK with that change? I think it would be a very awesome feature! |
|
Yeah, I'm working on it right now. |
|
awesome! |
This closes #794. It removes the list of special/supported platforms, and instead queries the repository with HEAD requests to find the channel root url to determine if a platform is present in the channel url or not. This is cached so it only occurs once per channel root url, assuming that urls won't change from being a channel root to being a platform-specific directory (incredibly unlikely).