Fix channel and base_url in list cmd
#3488
Conversation
Hind-M
force-pushed
the
umamba_list_json
branch
from
8651f0f to
1221fed
Compare
Hind-M
force-pushed
the
umamba_list_json
branch
from
00059be to
105e18e
Compare
| "https://pkg-containers.githubusercontent.com/ghcr1/blobs/pandoc" | ||
| ) | ||
| assert pkg["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" | ||
| assert pkg["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" |
There was a problem hiding this comment.
We could set this to something else like oci or ghcr?
But that could be done in another PR once it's all sorted out.
There was a problem hiding this comment.
Yeah let's fix the bugs for now;, and figure out what to do for OCI in a dedicated PR.
| // This is more or less an implementation of `util::rstrip` specific to this use case | ||
| // (for printing purposes), but using `std::string` instead of `std::string_view` | ||
| // `util::rstrip` is not used here because it leads to an UB, | ||
| // `using non owned/tracked strings from Channel (& co) and PackageInfo |
There was a problem hiding this comment.
Not sure to understand why util::rstrip leads to UB. Is it still the case if you assign its result to a std::string?
Anyway, if we keep this implementation, we should open an issue to try to fix the UB and use a single implementation of rstrip.
There was a problem hiding this comment.
Well, since we use it recursively, it will imply doing a lot std::string conversions?
Not sure if doing this is cleaner than just having a string version for rstrip?
Anyway, I agree that we should definitely reevaluate.
There was a problem hiding this comment.
I meant using the funciton degined in util::string and assign the final result of the recursive call to a string. The function returns temporary string_view passed by copies, so there hsould not be any dangling temporary issues.
There was a problem hiding this comment.
Well doing:
std::string res = std::string(util::rstrip(util::rstrip(util::rstrip(util::rstrip(full_str, filename), "/"), platform), "/"));
instead of using the reimplemented rstrip in strip_from_filename_and_platform definitely leads to UB, if that's what you meant.
There was a problem hiding this comment.
yep that's what I meant :( OK let's merge as is and open an issue to fix that UB later
| "https://pkg-containers.githubusercontent.com/ghcr1/blobs/pandoc" | ||
| ) | ||
| assert pkg["base_url"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" | ||
| assert pkg["channel"] == "https://pkg-containers.githubusercontent.com/ghcr1/blobs" |
There was a problem hiding this comment.
Yeah let's fix the bugs for now;, and figure out what to do for OCI in a dedicated PR.
|
I agree that we may reintroduce the canonical_name in a future PR. |
Fix #3480
The channel name used in
1.xwas dropped during the "Great refactoring" (under the assumption, IIUC, that it's the same ascanonical_name, which was not always true...) and now that info is stored/computed nowhere.Since this is only used in the
listcommand, it's handled this way for now...