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

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

Merged
merged 3 commits into from
Jan 4, 2021
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b275bc
Add the possibility to specify regex backend
remiburtin Nov 22, 2020
199191b
Handle auto with_regex option
remiburtin Dec 2, 2020
6a732b5
Update the regex option with the chosen backend when set to auto
remiburtin Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions recipes/libgit2/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}
default_options = {
"shared": False,
Expand All @@ -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",

}

@property
Expand Down Expand Up @@ -84,6 +86,10 @@ def requirements(self):
self.requires("mbedtls/2.16.3-gpl")
if tools.is_apple_os(self.settings.os) and self.options.with_iconv:
self.requires("libiconv/1.16")
if self.options.with_regex == "pcre":
self.requires("pcre/8.44")
elif self.options.with_regex == "pcre2":
self.requires("pcre2/10.35")

def source(self):
tools.get(**self.conan_data["sources"][self.version])
Expand Down Expand Up @@ -124,6 +130,9 @@ def _configure_cmake(self):
cmake.definitions["BUILD_EXAMPLES"] = False
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":

cmake.definitions["STATIC_CRT"] = "MT" in str(self.settings.compiler.runtime)

Expand Down