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

Inclusion Request: Thrift #711

Open
Croydon opened this Issue Apr 2, 2019 · 20 comments

Comments

Projects
None yet
4 participants
@Croydon
Copy link
Member

Croydon commented Apr 2, 2019

Package Details

Inclusion request

  • Fill all checkboxes with x if you agree with the statement or if it applies to your Conan recipe
  • I want that my Conan recipe is getting included into the Bincrafters organisation (conan-io/wishlist#143 (comment))
  • I have used the Bincrafters templates to develop my Conan recipe - https://github.com/bincrafters/conan-templates
  • My Conan recipe and all parts of it are licensed under the MIT license - https://choosealicense.com/licenses/mit/
    • Bincrafters are releasing all recipes under MIT license. If you want to have your recipe within Bincrafters we kindly ask you to do the same
    • it is okay if your test_package contains code from the upstream project under the upstream project's license, as long as it is clearly mentioned within the test_package files
  • My Conan recipe covers (at least) the latest stable version which is available for the package
  • I know that my Conan recipe is working on all operating system and compilers mentioned above right now
    • if not: please provide here links to your CI platforms and / or build logs and describe the problems you encounter
  • I'm willing to help to maintain the recipe even after it is included into Bincrafters (conan-io/wishlist#143 (comment))

All credits to @helmesjo

Checklist for the Bincrafters team

  • Ignore this if you are not part of the Bincrafters team
  • There is no such Conan package yet in conan-center, conan-community or Bincrafters
  • The project is either popular or deserves to be maintained for another good reason (we can't package every single project out there)
  • bincrafters-conventions reports no complains; the recipe fulfills our general quality requirements and conventions
  • The recipe is cloned into our organisation; CI is enabled and configurated and succeeds
  • There is a branch protection rule for stable/* branches, which requires a pull request with at least one approval first before things can get published
  • Considering granding the inclusion request author push rights to the recipe repository (there is no guarantee that everyone is getting push rights, we decide this on a case-to-case base)

@Croydon Croydon added the Packaging label Apr 2, 2019

@Croydon Croydon self-assigned this Apr 2, 2019

@uilianries

This comment was marked as resolved.

Copy link
Member

uilianries commented Apr 2, 2019

I think the recipe looks good but needs some update. We could accept and update, it will faster than requesting all changes

@Croydon

This comment was marked as resolved.

Copy link
Member Author

Croydon commented Apr 2, 2019

Well, I'm bringing it in, so it's not really an outside request anyway. So that is exactly what I planned.

I see that you did go ahead an cloned the repo already. I actually wanted to squash the history, since it is originally a fork of the templates repository, which makes not much sense to more to keep around

@uilianries

This comment was marked as resolved.

Copy link
Member

uilianries commented Apr 2, 2019

Ops, Sorry! We can squash anyway, we will need to force the push. I see no problem forcing the push, WDYT ?

@Croydon

This comment has been minimized.

Copy link
Member Author

Croydon commented Apr 4, 2019

I have cut of the old history, should be fine now

The package should be ready now, but conan-center is blocked since libevent isn't in conan-center yet 🤔

@SSE4

This comment has been minimized.

Copy link
Member

SSE4 commented Apr 4, 2019

@Croydon we may send inclusion requests for both, libevent and thrift. libevent is here for a while, and used in many packages.

@Croydon

This comment has been minimized.

Copy link
Member Author

Croydon commented Apr 4, 2019

Is the quality of libevent good enough?

  • there seems to be a weird workaround for a PATH problem on macOS? (in import())
  • do we really want to use an OpenSSL alias which is effectively a version range? Personally I would say no. I understand that we need to make sure that OpenSSL stays up-to-date but I would rather like to improve our tools in this direction
@SSE4

This comment has been minimized.

Copy link
Member

SSE4 commented Apr 4, 2019

  1. we have to fix that
  2. alias is not version range, it's a different concept. version range is dynamically computed expression, while alias is just a "symlink", or an alternative name of a package. it doesn't have overhead that version ranges have.
@Croydon

This comment was marked as resolved.

Copy link
Member Author

Croydon commented Apr 4, 2019

alias is not version range, it's a different concept. version range is dynamically computed expression, while alias is just a "symlink", or an alternative name of a package. it doesn't have overhead that version ranges have.

I didn't say that this alias IS a version range, but it ACTS like one. We still have non-reproducible builds

@uilianries

This comment was marked as resolved.

Copy link
Member

uilianries commented Apr 4, 2019

let's use OpenSSL/1.0.2r@conan/stable to keep the package reproducible. And it's more explicit for who reads the recipe.

@uilianries

This comment was marked as resolved.

Copy link
Member

uilianries commented Apr 4, 2019

@SSE4 Why we don't CMake to build libevent? The project offers a cmake file:

https://github.com/libevent/libevent/blob/release-2.1.8-stable/CMakeLists.txt

@SSE4

This comment was marked as resolved.

Copy link
Member

SSE4 commented Apr 4, 2019

@uilianries no idea, probably older versions didn't support CMake, or there were some issues with it. let's switch to CMake if possible.

@uilianries

This comment was marked as resolved.

Copy link
Member

uilianries commented Apr 4, 2019

@SSE4 Yes, there is no cmake support for 2.0.x . I'll try the CMake approach

@Croydon

This comment was marked as resolved.

Copy link
Member Author

Croydon commented Apr 5, 2019

For the protocol: We can look into using CMake starting with 2.2.x, but not right now: bincrafters/conan-libevent#8

Edit: I have created #723 for that matter.

@Croydon

This comment has been minimized.

Copy link
Member Author

Croydon commented Apr 9, 2019

Seems like @theirix have put this import() in several recipes. Could you please explain this problem further you workaround with this?

bincrafters/conan-libcurl@0d4e748
bincrafters/conan-libevent@2d686d1

@theirix

This comment has been minimized.

Copy link
Member

theirix commented Apr 10, 2019

@Croydon do you mean the problem that the copying solve? It is briefly described in a code comment. Autoconf creates little programs (conftest) that link against shared OpenSSL libraries and the only way to run conftest without dynamic linker errors is to bring dependent libraries into the current directory. It affects only shared library build on macOS 10.11+. The imports method guards the copying with OS check.

@uilianries

This comment has been minimized.

Copy link
Member

uilianries commented Apr 10, 2019

Before to request Conan Center inclusion, I think we should update Thrift to follow Bison idea: 1 project x 2 recipes (1 installer + 1 library).

@Croydon

This comment has been minimized.

Copy link
Member Author

Croydon commented Apr 10, 2019

@uilianries What is the advantage here? Saving disk space?

@uilianries

This comment has been minimized.

Copy link
Member

uilianries commented Apr 11, 2019

It's not about disk usage, the problem is maintaining two separated projects which are related to the same thing. For us it looks normal, we can find easily I think, but most users will search for canon-thrift only.

For example, Protobuf created many confusion, I needed to explain about protobuf-installer many times.

@Croydon

This comment has been minimized.

Copy link
Member Author

Croydon commented Apr 11, 2019

I don't mean why maintaining it in one repository, that is for sure better than having two repositories.

I mean why splitting it in two recipes (regular+installer). I know we had that discussion many, many times before... but I still don't see THE reason to do that. 🙈

@uilianries

This comment has been minimized.

Copy link
Member

uilianries commented Apr 12, 2019

Oh sorry, the problem is when you need to cross compile some code. The thrift app needs to read some input to generate the cpp files, but it runs in your host (amd64), however the libraries that you need are built for your target arch (arm).

For the thrift application you will need to use arch_build and for libraries you need arch. But you can't keep all together and install. Actually you could, but you would need to copy the application for all packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.