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

jinja2cpp: add new version 1.3.2 #21305

Merged
merged 13 commits into from
Jul 30, 2024
Merged

Conversation

toge
Copy link
Contributor

@toge toge commented Nov 22, 2023

Specify library name and version: jinja2cpp/1.3.2

jinja2cpp/1.3.2 requires Boost::json.
But Boost::json is not avaiable in apple-clang.
https://github.com/conan-io/conan-center-index/pull/5246/files#r616257192

In this PR, I will drop apple-clang support for jinja2cpp/1.3.2.
I'm trying to relax apple-clang condition for boost.


@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.


if (NOT DEFINED JINJA2_PRIVATE_LIBS_INT)
set(JINJA2CPP_PRIVATE_LIBS ${JINJA2CPP_PRIVATE_LIBS}
- Boost::variant Boost::filesystem Boost::algorithm Boost::lexical_cast Boost::json

Choose a reason for hiding this comment

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

@toge , would you mind telling me why we removing boost::regex, boost::json?

boost::regex seems to significantly speed ups template loading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmorozov
Oh, it's my mistake.
Thank you for your review!

Choose a reason for hiding this comment

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

I have added special option to choose which regex to use - default one is boost regex, but if it is not suitable feel free to override and use std regex as it was earlier

https://github.com/jinja2cpp/Jinja2Cpp/blob/4393211baa8ba69437d070be3c9fcba59aad6793/CMakeLists.txt#L15

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@AbrilRBS AbrilRBS self-assigned this Nov 23, 2023
@conan-center-bot

This comment has been minimized.

@@ -76,6 +76,12 @@ def validate(self):
raise ConanInvalidConfiguration(
f"{self.ref} requires C++{self._min_cppstd}, which your compiler does not support."
)
if Version(self.version) >= "1.3.1" and self.settings.compiler == "apple-clang":

Choose a reason for hiding this comment

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

I have a special switch for this case
https://github.com/jinja2cpp/Jinja2Cpp/blob/master/CMakeLists.txt#L18

for apple-clang there can be -DJINJA2CPP_WITH_JSON_BINDINGS=rapid, which doesn't require boost::json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmorozov
Thanks a lot!
I will fix this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmorozov
I tried to compile jinja2cpp/1.3.1 with apple-clang with INJA2CPP_WITH_JSON_BINDINGS=rapid.
But I get the same compile error due to Boost::json.
This is because CMakeLists.txt in jinja2cpp/1.3.1 tries to link Boost::json despite the value of INJA2CPP_WITH_JSON_BINDINGS.
I will try to make jinja2cpp/1.3.1 not compatible with apple-clang in this PR, because I think it is necessary to fix CMakeLists.txt or conan's boost recipe.

@ghost ghost mentioned this pull request Jan 13, 2024
3 tasks
@conan-center-bot

This comment has been minimized.

@toge
Copy link
Contributor Author

toge commented Feb 1, 2024

@RubenRBS
Cloud you review it?

Comment on lines 14 to 39
diff --git a/src/helpers.h b/src/helpers.h
index 3af280f..b1c31fe 100644
--- a/src/helpers.h
+++ b/src/helpers.h
@@ -32,12 +32,21 @@ struct MultiStringLiteral
#endif
}

+#if defined(_MSC_VER) && (_MSC_VER <= 1900)
+ template<typename CharT>
+ auto GetValueStr() const
+ {
+ auto memPtr = SelectMemberPtr<CharT, &MultiStringLiteral::charValue, &MultiStringLiteral::wcharValue>::GetPtr();
+ return std::basic_string<CharT>(this->*memPtr);
+ }
+#else
template<typename CharT>
constexpr auto GetValueStr() const
{
constexpr auto memPtr = SelectMemberPtr<CharT, &MultiStringLiteral::charValue, &MultiStringLiteral::wcharValue>::GetPtr();
return std::basic_string<CharT>(this->*memPtr);
}
+#endif

template<typename CharT, const char* MultiStringLiteral::*, const wchar_t* MultiStringLiteral::*>
struct SelectMemberPtr;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a separate patch. Submitted upstream as well, if possible.

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @toge, I'll get to this review early next monday, this PR fell thru the cracks and I just now found it again :)

@toge toge changed the title jinja2cpp: add version 1.3.1 jinja2cpp: add version 1.3.2 Jun 27, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@toge toge changed the title jinja2cpp: add version 1.3.2 jinja2cpp: add new version 1.3.2 Jul 1, 2024
@conan-center-bot

This comment has been minimized.

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.

Hello and thank you for bumping this recipe.

Boost regex is not mandatory in 1.3.2, but used by default due its speed:

jinja2cpp/Jinja2Cpp@1.2.1...1.3.2#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR20

jinja2cpp/Jinja2Cpp@1.2.1...1.3.2#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R277

I would suggest adding a logic in generate to follow that support:

def generate(self):
    tc = CMakeToolchain(self)
    if Version(self.version) >= "1.3.2":
        tc.cache_variables["JINJA2CPP_USE_REGEX"] = "std" if self.dependencies["boost"].options.without_regex else "boost"

@toge
Copy link
Contributor Author

toge commented Jul 29, 2024

@uilianries
Sorry for late response.

Your suggested code doesn't work well.

CMake Error at CMakeLists.txt:188 (target_link_libraries):
  Target "jinja2cpp" links to:

    Boost::regex

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Boost::regex is always required in external_boost_deps.cmake.
https://github.com/jinja2cpp/Jinja2Cpp/blob/1.3.2/thirdparty/external_boost_deps.cmake#L56

It may be a bug.

I added with_regex option to select regex library.
Could you review it again?

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 7 (f5fbed679dd4d18874ff61ae526c9a35a73b0b62):

  • jinja2cpp/1.3.2:
    All packages built successfully! (All logs)

  • jinja2cpp/1.2.1:
    All packages built successfully! (All logs)

  • jinja2cpp/1.1.0:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 7 (f5fbed679dd4d18874ff61ae526c9a35a73b0b62):

  • jinja2cpp/1.3.2:
    All packages built successfully! (All logs)

  • jinja2cpp/1.1.0:
    All packages built successfully! (All logs)

  • jinja2cpp/1.2.1:
    All packages built successfully! (All logs)

@uilianries
Copy link
Member

@toge thank you for pinging. I'll take a look tomorrow 😁

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.

LGTM. Thank you @toge!

@uilianries uilianries requested a review from AbrilRBS July 30, 2024 12:03
@conan-center-bot conan-center-bot merged commit 3ad8288 into conan-io:master Jul 30, 2024
20 checks passed
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.

6 participants