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

mariadb-connector-cpp: initial commit (rediscovered) #26026

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

xiadnoring
Copy link

@xiadnoring xiadnoring commented Nov 25, 2024

Summary

Changes to recipe: mariadb-connector-cpp/1.1.4

Motivation

mariadb-connector-cpp support

Details

on c++
the past pull request: #24653

perseoGI
perseoGI previously approved these changes Nov 25, 2024
Copy link
Contributor

@perseoGI perseoGI left a comment

Choose a reason for hiding this comment

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

LGTM!

jcar87
jcar87 previously requested changes Nov 25, 2024
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

  • do not rename libraries
  • retain the declspec

@perseoGI - please remember we do not rename libraries in Conan Center - as the library names may be already relied upon outside of Conan, and we do not want potential consumers to have to use this library differently in Conan. If the upstream project generates mariadbcpp-static.lib - then the only action is to make sure this is reflected in cpp_info.

As for the declspec being removed here: 0d5e959 - I had added it to reflect what upstream defines for consumers https://github.com/mariadb-corporation/mariadb-connector-cpp/blob/d2dc327ee54cdfcb91bf241dbc6cdcccf5447c4c/CMakeLists.txt#L575 . It may be that the header files define it as a fallback, but I think we should closely match the original files.

Note that some of the commits in this PR are from here #24653 - and I had added and authored some fixes. Unsure why that PR was closed and reopened as a new one, but I think it should correctly reflect all authors before merging.

@xiadnoring
Copy link
Author

  1. I wanted the pull request to be in a prominent place again.
  2. OK

@jcar87
Copy link
Contributor

jcar87 commented Nov 25, 2024

  1. I wanted the pull request to be in a prominent place again.

We we already looking into it as of last Friday - we are actively going through the backlog, so no need to open/reopen :)

@xiadnoring
Copy link
Author

xiadnoring commented Nov 25, 2024

you stopped working at some point, I thought it would be abandoned. 😅

Although it was a weekend 🙂

@xiadnoring xiadnoring closed this Nov 25, 2024
@xiadnoring xiadnoring reopened this Nov 25, 2024
@xiadnoring
Copy link
Author

  1. @perseoGI, I'm sorry for letting you down.
  2. @jcar87, I'm sorry that I almost appropriated your merits to myself and so on.

perseoGI
perseoGI previously approved these changes Nov 27, 2024
@jcar87
Copy link
Contributor

jcar87 commented Dec 4, 2024

@jcar87, I'm sorry that I almost appropriated your merits to myself and so on.

Oh no worries here! not about merits - we just need to make sure we trace the correct author attribution for CLA purposes - as well as avoiding the minor confusion with the two PR on the reviewers side

@jcar87 jcar87 dismissed stale reviews from perseoGI and themself December 4, 2024 16:23

team review

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.

3 participants