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

idna: add recipe #22891

Merged
merged 10 commits into from
Mar 7, 2024
Merged

idna: add recipe #22891

merged 10 commits into from
Mar 7, 2024

Conversation

toge
Copy link
Contributor

@toge toge commented Feb 25, 2024

Specify library name and version: idna/*

@prince-chrismc prince-chrismc mentioned this pull request Feb 25, 2024
Open
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@AbrilRBS AbrilRBS self-assigned this Feb 26, 2024
AbrilRBS
AbrilRBS reviewed Feb 26, 2024
View reviewed changes
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.

One question about the target name, other than that seems good!

recipes/idna/all/conanfile.py Outdated Show resolved Hide resolved
recipes/idna/all/conanfile.py
cmake = CMake(self)
cmake.install()

def package_info(self):
Copy link
Member

Choose a reason for hiding this comment

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

Ran out of time to look into this specificly, but it seems like upstream expects different naming than idna::idna for the link target? Could you please double chec that for me? Thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RubenRBS
Thank you for your comments.
I don't know the exact target since there is no install target, but it looks like ada-idna in CMakeLists.txt in benchmark is the official target.
https://github.com/ada-url/idna/blob/main/benchmarks/CMakeLists.txt#L3

toge and others added 2 commits February 27, 2024 23:28
@toge @AbrilRBS
remove sample topics
03ed369 
Co-authored-by: Rubén Rincón Blanco <[email protected]>
@toge
fix target name
1c49cff
@conan-center-bot

This comment has been minimized.

Sign in to view
toge and others added 2 commits February 28, 2024 14:24
@toge @valgur
fix license
5528026 
Co-authored-by: Martin Valgur <[email protected]>
@toge @valgur
Add VirtualBuildEnv
fa4b539 
Co-authored-by: Martin Valgur <[email protected]>
@conan-center-bot

This comment has been minimized.

Sign in to view
@uilianries uilianries mentioned this pull request Feb 28, 2024
Merged
@uilianries
Copy link
Member

I just opened a PR to the upstream adding these cmake install steps: ada-url/idna#41

uilianries
uilianries reviewed Feb 28, 2024
View reviewed changes
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.

Looking, only small details.

recipes/idna/all/conanfile.py Outdated Show resolved Hide resolved
recipes/idna/all/conanfile.py Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Sign in to view
toge and others added 2 commits February 28, 2024 22:26
@conan-center-bot

This comment has been minimized.

Sign in to view
Croydon
Croydon reviewed Feb 29, 2024
View reviewed changes
recipes/idna/all/CMakeLists.txt Outdated

include(GNUInstallDirs)

install(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be probably nicer to apply this as a patch to the current version, then it can be easily dropped in future versions

Copy link
Member

Choose a reason for hiding this comment

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

They indeed dropped: ada-url/idna#41

Copy link
Member

Choose a reason for hiding this comment

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

Agree, we can simplify here if we package the latest commit @toge :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RubenRBS
Thanks a lot!
I will update this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 8 (c33982d0a10f4f9a3b757537b36f66021e2ddcd7):

  • idna/cci.20240228:
    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 8 (c33982d0a10f4f9a3b757537b36f66021e2ddcd7):

  • idna/cci.20240228:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit c4308b4 into conan-io:master Mar 7, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uilianries uilianries uilianries left review comments

@AbrilRBS AbrilRBS AbrilRBS approved these changes

@valgur valgur valgur approved these changes

@Croydon Croydon Croydon approved these changes

Assignees

@AbrilRBS AbrilRBS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@toge @conan-center-bot @uilianries @Croydon @valgur @AbrilRBS