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

I'd like to contribute zstd and asio #91

Closed
ned14 opened this issue Dec 5, 2019 · 33 comments
Closed

I'd like to contribute zstd and asio #91

ned14 opened this issue Dec 5, 2019 · 33 comments
Labels
help wanted Extra attention is needed

Comments

@ned14
Copy link

ned14 commented Dec 5, 2019

I have a cmake hunterised fork of zstd at https://github.com/ned14/zstd, and a cmake hunterised fork of asio at https://github.com/ned14/asio

How do you guys want it contributed given that I assume hunter-packages is no longer open?

@ned14 ned14 added the help wanted Extra attention is needed label Dec 5, 2019
@NeroBurner
Copy link

could you try to bring you cmake specific changes upstream?

@ned14
Copy link
Author

ned14 commented Dec 5, 2019

Chris won't allow it for ASIO.

For zstd, I can ask, and I shall.

@ned14
Copy link
Author

ned14 commented Dec 5, 2019

facebook/zstd#1922

@bkotzz
Copy link
Member

bkotzz commented Dec 5, 2019

FYI we do still fork packages if needed, it’s just done in the cpp-pm org. Let us know if you need one

@ned14
Copy link
Author

ned14 commented Dec 5, 2019

You'll definitely need one for ASIO.

@rbsheth
Copy link
Member

rbsheth commented Dec 5, 2019

@ned14 New forks here:

https://github.com/cpp-pm/zstd
https://github.com/cpp-pm/asio

Added you to the packages team also. Please send your PRs against the hunter-* branches I made there.

@ned14
Copy link
Author

ned14 commented Dec 6, 2019

I'm currently negotiating with the zstd maintainers for them to directly support hunter. So far it goes well.

I'll hopefully push ASIO later today. Thanks for your help.

@ned14
Copy link
Author

ned14 commented Dec 6, 2019

@rbsheth Sorry, you've used the wrong base for the hunter-* branch. It ought to be https://github.com/cpp-pm/asio/releases/tag/asio-1-12-2 which is the latest stable release. Can you redo the branch against that release tag please?

Also, with old cmake hunter, the branch names in the packages directory were named after the release e.g. v0.5-p0. Has the policy on this changed? If so, should I ignore the branch naming conventions specified by https://docs.hunter.sh/en/latest/creating-new/create/cmake.html?

@ned14
Copy link
Author

ned14 commented Dec 6, 2019

hunterisation zstd PR has been submitted to facebook/zstd#1924

Can hunter directly use repos without a CMakeLists.txt in their base directory? It would be a shame to have to fork zstd purely and exclusively just to stick a dummy CMakeLists.txt in its root.

@NeroBurner
Copy link

We could try to add that functionality to hunter

Something like the subdirectory feature for git repos

ruslo#928

@rbsheth
Copy link
Member

rbsheth commented Dec 6, 2019

@ned14 Fixed: https://github.com/cpp-pm/asio

I think the branches were just the version and the releases had the -pX suffixes.

@NeroBurner
Copy link

@ned14 for zstd you can use hunter_source_subdir() when the PR is merged

#95

@ned14
Copy link
Author

ned14 commented Dec 9, 2019

zstd accepted the PR, so it's ready to go. I've subscribed to PR #95 so I'll see when it's ready. Meanwhile I should be able to PR ASIO soon.

@rbsheth
Copy link
Member

rbsheth commented Dec 11, 2019

@ned14 #95 was merged and released in https://github.com/cpp-pm/hunter/releases/tag/v0.23.236, looking forward to your PRs!

@ned14
Copy link
Author

ned14 commented Dec 12, 2019

I've got an ASIO PR ready, just was awaiting the SHA. I should push it soon.

@ned14
Copy link
Author

ned14 commented Dec 12, 2019

What's the point of hunter-testing when PRs against hunter are CI checked?

Also the advice to use Appveyor doesn't work, due to lack of an appveyor.yml file in the root of hunter. I don't know why you don't have one of those.

@NeroBurner
Copy link

PRs against hunter check the consistency of the hunter package declaration. They don't check if the packages themselves build successfully. The packages themselves are tested at hunter-testing

The process is documented here https://docs.hunter.sh/en/latest/creating-new/create/cmake.html

The cpp-pm guys are working on simplifying this process (see #9 and #28 for example). Input on this is highly appreciated

@ned14
Copy link
Author

ned14 commented Dec 13, 2019

The current new package add process is quite confusing for the uninitiated, but I am making progress:

Is it okay to remove whole chunks of CI testing? Specifically, the mingw CI tests, ASIO doesn't officially support mingw, and there's a header only target for those folk in any case.

@NeroBurner
Copy link

remove whole chunks of CI testing?

yes, just add a comment on why the chunk was commented out and add a link to a failing ci-job showing the error. For example: https://github.com/ingenue/hunter/blob/pkg.ceres-solver/appveyor.yml#L32

I'd recommend looking into why gcc and clang are failing. It looks like pthreads is missing, as it fails at the linking stage
https://travis-ci.org/ned14/hunter-testing/jobs/624616633#L1009

/home/travis/build/ned14/hunter-testing/_testing/Hunter/_Base/79224c5/239e635/f550390/Install/lib/libasio_shared.so: undefined reference to `pthread_create'

@ned14
Copy link
Author

ned14 commented Dec 13, 2019

Already fixed in the PR at cpp-pm/asio#3

@rbsheth
Copy link
Member

rbsheth commented Dec 13, 2019

@ned14
Copy link
Author

ned14 commented Dec 13, 2019

Ok PR is opened for hunter-testing cpp-pm/hunter-testing#20

I'm a bit confused now what happens next for #101. Do I have to do anything, or does what I PRed to hunter-testing auto-migrate over to hunter?

@rbsheth
Copy link
Member

rbsheth commented Dec 13, 2019

@ned14 First, we merge this PR. Then, I update pkg.template with the latest master and cut a branch pkg.asio. Then, your testing PR is merged into that branch.

As such, you should update this PR #101 with the -p1 release.

@ned14
Copy link
Author

ned14 commented Dec 13, 2019

As such, you should update this PR #101 with the -p1 release.

Just to be clear, PR #101 should be updated with the -p1 release, but without the changes to travis and appveyor right? To be absolutely clear, everything in hunter-testing apart from those two files?

@rbsheth
Copy link
Member

rbsheth commented Dec 13, 2019

That's right! I know it is confusing, but that's how the system works right now.

@ned14
Copy link
Author

ned14 commented Dec 13, 2019

-p1 is pushed to PR #101

@rbsheth
Copy link
Member

rbsheth commented Dec 13, 2019

@ned14 Thanks, couple comments on #101 and then I can merge

@rbsheth
Copy link
Member

rbsheth commented Dec 13, 2019

@ned14
Copy link
Author

ned14 commented Dec 17, 2019

Given:

https://ci.appveyor.com/project/ned14/hunter-testing/builds/29599780/job/wkuw7nx1mdirnh25

... could someone have a look at https://github.com/ned14/hunter-testing/blob/pkg.zstd/cmake/projects/zstd/hunter.cmake and tell me what I'm doing wrong with the new source subdir facility?

@NeroBurner
Copy link

NeroBurner commented Dec 17, 2019

I'm sorry, I goofed up the example in the documentation

#105

you need to have another include to hunter_source_dir hunter_source_subdir and then use the command

hunter_source_subdir(
    zstd
    SOURCE_SUBDIR
    "build/cmake")

edit: hunter_source_dir --> hunter_source_subdir()

@ned14
Copy link
Author

ned14 commented Dec 17, 2019

@NeroBurner
Copy link

NeroBurner commented Dec 17, 2019 via email

@ned14
Copy link
Author

ned14 commented Dec 18, 2019

zstd just in time for Christmas!

@ned14 ned14 closed this as completed Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants