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

libgit2/1.0.1: add an option to specify the regex backend to use #3685

Merged

Conversation

remiburtin
Copy link
Contributor

Specify library name and version: libgit2/1.0.1

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

Since version 1.0, libgit2 expose an option to specify the regex backend to use. This option isn't currently exposed in the conan recipe.

@CLAassistant
Copy link

CLAassistant commented Nov 24, 2020

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

All green in build 1 (7b275bcd15e4acd92a59f4fa1c0172c41b56c404)! 😊

@@ -22,6 +22,7 @@ class LibGit2Conan(ConanFile):
"with_https": [False, "openssl", "mbedtls", "winhttp", "security"],
"with_sha1": ["collisiondetection", "commoncrypto", "openssl", "mbedtls", "generic", "win32"],
"with_ntlmclient": [True, False],
"with_regex": ["default", "builtin", "pcre", "pcre2"],
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the default? builtin?
Using default is ambiguous. Use builtin instead.

Suggested change
"with_regex": ["default", "builtin", "pcre", "pcre2"],
"with_regex": ["builtin", "pcre", "pcre2"],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. In fact, this is how libgit2 works: https://github.com/libgit2/libgit2/blob/v1.0.1/src/CMakeLists.txt#L107-L157
default doesn't have the same behavior as builtin. Maybe default isn't very appropriate. What do you think about auto ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the link.
If I read it correctly, then auto will result in a package equal to builtin, pcre or pcre2 depending on the build machine.
I don't think auto will give us reproducible packages.

Maybe the safest would be to set the default to pcre, as we have a conan package for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. The builtin backend is pcre. But as you said, it may be preferable to set the default to conan pcre so that the dependency is explicit.
Originaly, I made default \ auto the default option so that the actual behavior isn't changed (withtout thinking about build reproductibility).

Copy link
Contributor

Choose a reason for hiding this comment

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

What you can do instead, is keep the auto option and do something like the cmake recipe instead (for the ssl backend).
It has an auto option that aliases to winssl on Windows and openssl on others.

This requires some knowledge of you (=the recipe editor) to know what option is selected on the different platforms.
By reading the c3i build logs on clang@Linux, gcc@Linux, clang@Macos and MSVC@Windows, you can deduce what default options is selected on each platform.

@@ -32,6 +33,7 @@ class LibGit2Conan(ConanFile):
"with_https": "openssl",
"with_sha1": "collisiondetection",
"with_ntlmclient": True,
"with_regex": "default",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"with_regex": "default",
"with_regex": "builtin",

Comment on lines 131 to 136
cmake.definitions["USE_HTTP_PARSER"] = "system"

if self.options.with_regex != "default":
cmake.definitions["REGEX_BACKEND"] = self.options.with_regex

if self.settings.compiler == "Visual Studio":
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cmake.definitions["USE_HTTP_PARSER"] = "system"
if self.options.with_regex != "default":
cmake.definitions["REGEX_BACKEND"] = self.options.with_regex
if self.settings.compiler == "Visual Studio":
cmake.definitions["USE_HTTP_PARSER"] = "system"
cmake.definitions["REGEX_BACKEND"] = self.options.with_regex
if self.settings.compiler == "Visual Studio":

@SSE4 SSE4 requested a review from uilianries November 25, 2020 05:25
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, please @remiburtin, follow MadeBR's suggestions.

@remiburtin
Copy link
Contributor Author

Sorry for the delay.
After a bit of diging through C3I build logs, I found out that regcomp_l is used by default on clang@macos and that the bundled pcre (builtin) is used on other platform.

@conan-center-bot
Copy link
Collaborator

All green in build 3 (6a732b533d96b7c08ad6fdcedd9f595854827518)! 😊

@prince-chrismc
Copy link
Contributor

@uilianries This is the oldest PR you are blocking, the changes you requested have been applied, if you have a change to take a second look that would be appreciated as always!

It has sufficient reviews otherwise

#3685 remiburtin 📝 libgit2 13 uilianries prince-chrismc, intelligide, madebr, SSE4

@uilianries
Copy link
Member

@prince-chrismc Thank you for pinging me!

@conan-center-bot conan-center-bot merged commit 52ec72c into conan-io:master Jan 4, 2021
@prince-chrismc
Copy link
Contributor

You are very welcome! 🤗

You can take a peek at prince-chrismc/conan-center-index-pending-review#1 there's three others you are blocking.
Hopefully it helps

@remiburtin remiburtin deleted the package/libgit2-regex-backend branch January 7, 2021 14:51
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.

8 participants