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 more libcurl calls #2421

Merged
merged 3 commits into from
Apr 5, 2023
Merged

Wrap more libcurl calls #2421

merged 3 commits into from
Apr 5, 2023

Conversation

Hind-M
Copy link
Member

@Hind-M Hind-M commented Mar 29, 2023

Part of #2356
This PR includes:

  • Moving curl_initialized from context to DownloadTarget class member
  • Moving hide_secrets function from url.{hpp, cpp} to util.{hpp, cpp}
  • Adding a getter for m_errorbuffer
  • Defining set_curl_handle for more curl calls wrapping

@Hind-M Hind-M self-assigned this Mar 29, 2023
@Hind-M Hind-M marked this pull request as ready for review March 29, 2023 14:11
@Hind-M Hind-M mentioned this pull request Mar 31, 2023
libmamba/include/mamba/core/util.hpp Outdated Show resolved Hide resolved
libmamba/src/core/curl.cpp Outdated Show resolved Hide resolved
libmamba/src/core/curl.cpp Outdated Show resolved Hide resolved
libmamba/src/core/curl.cpp Outdated Show resolved Hide resolved
@@ -159,12 +95,12 @@ namespace mamba
{
auto& ctx = Context::instance();

if (!ctx.curl_initialized)
if (!m_curl_initialized)
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 there is a risk that we do many times the same thing here since ctx.curl_initialized is a global, shared variable, while m_curl_initialized is a data member specific to a DownloadTarget data member.

This might not be a big deal here since we just set another variable in the context, but it might become later if we add stuff in this function.

What is the motivation for removing this variable from the context? Would it make sense to have it as part of a "DownloaderOption" subobject in the Context? (that could be done in another PR, but the current variable would remain in the context in this PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I actually found out that it's not used anywhere but here in the fetch... so no reason to keep it in the context! Do you see any, or think it would be used somewhere else?

Copy link
Member

@JohanMabille JohanMabille Apr 5, 2023

Choose a reason for hiding this comment

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

I think it should live with the ssl_verify variable and maybe move to CURLSetup or any configuration class for curl on the long run. This should be set once and for good. Keeping it in the context is ok for now.

@JohanMabille JohanMabille merged commit 4a9dea0 into mamba-org:main Apr 5, 2023
@Hind-M Hind-M deleted the libcurl branch April 5, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants