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.

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

Wrap libcurl calls #164

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Wrap libcurl calls #164

wants to merge 9 commits into from

Conversation

Hind-M
Copy link
Member

@Hind-M Hind-M commented Feb 14, 2023

Fix #159
It seems like we are using libcurl functions quite everywhere in the code base, so I don't think we can drop libcurl dependency.
I'm opening the PR as a draft to have first feedbacks and discuss if this is how we want to do stuff.

TODO:

  • Handle ssl_backend_t enum conversion to use outside powerloader.

@@ -15,6 +15,22 @@ namespace powerloader
// Want: S3, OCI
};

enum CompressionType
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the enums here so that we could have them in the same file. But maybe we don't want that...?

Copy link
Member

Choose a reason for hiding this comment

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

If powerloader user dont need them, then maybe moving them into src/curl_internal.hpp might work better?


namespace powerloader
{
class CURLInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

I put this class here, but maybe having its own file is more appropriate so that we don't include CURLSetup where it is not needed.

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's fine here (I think about it like a "module"), although not sure the impact of includes.
I'm fine with whatever you end up prefering.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, if the goal is just to have a name prefix, I think you can remove the class and replace it by a namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm actually the CURLInterface functions have access to CURLHandle private curl stuff because they are friends. If we just use a namespace, is there a way to have access as well? (without including headers)

Copy link
Member

Choose a reason for hiding this comment

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

Weird. Well the equivalent would be to make the functions as friend too. But then would check if it's really necessary to have these friends... or if maybe hiding a data structure in CURLHandle through a pointer which is known internally but not publicly wouldnt be a better solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that we need to indeed hide a data structure like but we still need it to be accessible in many other places where it is used outside the CURLHandle (I guess that was the main reason why many of these curl functions were public in CURLHandle).

Copy link
Member

Choose a reason for hiding this comment

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

Yes I meant that there is an alternative way to do it, I'll ping you in pms to discuss it.

@Hind-M Hind-M self-assigned this Feb 14, 2023
@Hind-M
Copy link
Member Author

Hind-M commented Feb 15, 2023

@wolfv I am wondering if the powerloader user can choose the SSL backend when using the Context or is it something supposed to be handled internally?
cc @JohanMabille

@Klaim
Copy link
Member

Klaim commented Feb 15, 2023

@wolfv I am wondering if the powerloader user can choose the SSL backend when using the Context or is it something supposed to be handled internally?

We discussed this in a recent PR and the decision is purely based on which OS is being used, so it's fine if it's decided by powerloader. Basically we want to move that code from the mamba integration PR back to powerloader, probably just keeping the function deciding and calling it as the default value in the context options.

@wolfv
Copy link
Member

wolfv commented Feb 15, 2023

Just to clarify, this SSL backend setting is only relevant for the case where we have a statically linked binary (micromamba). In other cases, the curl default works fine since ca-certificates are properly installed. In the micromamba case we're forced to use the system one.

It is useful to be able to select the SSL backend, because it also controls which certificate store is used (e.g. keychain on macOS or the Windows certificate store on Windows). Users in corporate environments often have some self-signed certificates that they use to access the internet via some proxy.

@wolfv
Copy link
Member

wolfv commented Feb 15, 2023

So IMO being able to choose the backend with the Context, but leaving it nullopt by default is the way to go.

@Hind-M
Copy link
Member Author

Hind-M commented Feb 15, 2023

Ok! But then If we want to not expose ssl_backend_t enum, we would need to use some kind of equivalent enum related to the platform but independent from curl and which would be converted to the current curl enum within powerloader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make libcurl a private dependency
3 participants