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.

Sign up for GitHub

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

Jump to bottom

Parse subdirs in CLI match specs #2798

Closed
wants to merge 1 commit into from
Closed

Parse subdirs in CLI match specs #2798

wants to merge 1 commit into from

Conversation

jonashaag
Copy link
Collaborator

Fixes #2792

For example, the channel in install conda-forge/noarch::tzdata would previously be parsed as conda-forge/noarch while it should be parsed as conda-forge with noarch subdir.

@jonashaag
Parse subdirs in CLI match specs
8f52bc5 
Fixes mamba-org#2792

For example, the channel in `install conda-forge/noarch::tzdata` would
previously be parsed as `conda-forge/noarch` while it should be parsed
as `conda-forge` with `noarch` subdir.
jonashaag
jonashaag commented Aug 29, 2023
View reviewed changes
libmamba/include/mamba/util/url_manip.hpp
std::string& cleaned_url,
std::string& platform
);
std::pair<std::string, std::optional<std::string>>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to return a tuple instead of doing pointer passing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand this makes the client code easier to write, I find this API more complicated to read and less expressive than the first one. Maybe a type alias for the return type could help, and/or a doc comment, but this will break the API and will probably need further update in micromamba.

Also I think this function should keep known_platforms and context_platform arguments instead of merging them, for different reasons that I will detail below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Break what API? I changed all code that uses this. Is this an interface used by external users?

The previous interface had an implicit flag to signal if it found a platform: The output platform variable is set to "" or not. I find that much less obvious than using an optional.

I agree with restoring context_platform.

Copy link
Member

@JohanMabille JohanMabille Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed that's an internal API, my bad (and it will probably stayed an internal cone, contrary to other headers in core).

I agree that it's better to use an optional than an implicit flag for the platform output variable, my point was that keeping the output names was more expressive than returning a complex composed type. But as I said, a typedef on the return type can help reading it.

EDIT: sorry for the duplicated answers, I don't know what happened when I submitted the review Oo

libmamba/src/core/channel.cpp
platform
);
if (!platform.empty())
auto all_platforms = KNOWN_PLATFORMS;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this creates a copy of KNOWN_PLATFORMS with the context channel added. Is that correct? I don't want to modify KNOWN_PLATFORMS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this creates a copy. If you keep all_platforms and context_platform argument in the split_platform API, you don't need to do this.

JohanMabille
JohanMabille requested changes Aug 29, 2023
View reviewed changes
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep "Fixing issues" PRs as minimal as possible, and avoid API change as much as we can.

libmamba/include/mamba/util/url_manip.hpp
std::string& cleaned_url,
std::string& platform
);
std::pair<std::string, std::optional<std::string>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand this makes the client code easier to write, I find this API more complicated to read and less expressive than the first one. Maybe a type alias for the return type could help, and/or a doc comment, but this will break the API and will probably need further update in micromamba.

Also I think this function should keep known_platforms and context_platform arguments instead of merging them, for different reasons that I will detail below.

libmamba/src/core/channel.cpp
platform
);
if (!platform.empty())
auto all_platforms = KNOWN_PLATFORMS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this creates a copy. If you keep all_platforms and context_platform argument in the split_platform API, you don't need to do this.

libmamba/include/mamba/core/channel.hpp
@@ -18,6 +18,14 @@

namespace mamba
{
// ATTENTION names with substrings need to go longer -> smalle
// otherwise linux-ppc64 matches for linux-ppc64le etc!
const std::vector<std::string> KNOWN_PLATFORMS = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to keep the definition of this variable in the cpp file, and provide a function to get it in the header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! I don't understand what difference that makes but not a C++ expert

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in the future we want to update the KNOWN_PLATFORMS variable:

  • if KNOWN_PLATFORMS is defined in the header, any binary using it will have to recompile when updating, meaning we break the ABI compatibility.
  • if it is defined in a source file, the client can simply upgrade the dependency with no further action required on its side.

I know this header is not really a "public" API yet (in that it's not in include/api), but it will likely become one after the ongoing refactoring. We want libmamba to be usable as a framework and not just be a geared towards the CLI.

libmamba/src/util/url_manip.cpp
platform = context_platform;
}
else
std::size_t pos;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging the known_platforms and context_platform arguments potentially leads to a different behavior here. The user has to know the detail of the implementation if he wants to be sure that the context_platform take priority over known_platforms. Beside, if the implementation changes in the future for any reason and we decide to parse known_platform in a different order, we may silently break existing code.

@jonashaag jonashaag mentioned this pull request Aug 29, 2023
Merged
@jonashaag
Copy link
Collaborator Author

Superseded by #2799

@jonashaag jonashaag closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohanMabille JohanMabille JohanMabille requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subdir in match spec does not take multichannel into account
2 participants
@jonashaag @JohanMabille