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

new package: dpp 10.0.34 #25745

Merged
merged 29 commits into from
Oct 30, 2024
Merged

Conversation

braindigitalis
Copy link
Contributor

Summary

Adds new recipe dpp/10.0.34

Motivation

This library does not exist in Conan yet.

Details

Adds the initial build for dpp 10.0.34. I am the lead developer of the library in the recipe.
Installs from a zip of the release.


@CLAassistant
Copy link

CLAassistant commented Oct 28, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@braindigitalis
Copy link
Contributor Author

Conan v1 pipeline ❌

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

Changes not allowed in build 1:

[recipes/dpp/all/conanfile.py, recipes/dpp/config.yml]

The folder test_package is required for every recipe. Please, add it for dpp/all/test_package.

Conan v2 pipeline ❌

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

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

Changes not allowed in build 1:

[recipes/dpp/all/conanfile.py, recipes/dpp/config.yml]

The folder test_package is required for every recipe. Please, add it for dpp/all/test_package.

how exactly is this to be tested with a test package? it is a discord library and requires a token to test it?

@AbrilRBS AbrilRBS self-assigned this Oct 28, 2024
@AbrilRBS
Copy link
Member

Hi @braindigitalis thanks a lot for taking the time to add this to CCI, we appreciate it!

A few points about the PR:

I recommend you use the package template available in https://github.com/conan-io/conan-center-index/tree/master/docs/package_templates/cmake_package as the basis for your package, which will improve the recipe and simplify the review process

how exactly is this to be tested with a test package? it is a discord library and requires a token to test it?

There's no need to actually run the library/tests - usually ensuring that a call to any of the functions implemented in a .cpp file is enough for the test_package, ideally something that interacts with the dependencies, but we don't want to actually make connections to any server and the like. Usually this translates into calling a version function, or instantiating a client but not connecting it, or something along those lines. We can help find the right think to do once the recipe has been transformed to the package template linked above

(Also note that currently your PR is not set to allow us to directly commit on your branch, which limits how useful we can be while reviewing, I also recommend enabling it here, thanks!)

Again, thanks for the effort in opening the PR, happy to help where needed :)

@conan-center-bot

This comment has been minimized.

@braindigitalis
Copy link
Contributor Author

... why are there no logs?
i can't exactly diagnose and fix if there are no logs from the CI.
DPP builds successfully on all of the platforms listed above, outside of conan. they are all officially supported.

Also the reason you cannot contribute to the PR is because it is from within an organisation, this is a known issue in github. Someone reported it as an issue in 2021 and github being github, still havent fixed it.

@uilianries
Copy link
Member

@braindigitalis Hello and thank you for your PR! Please, take a look in the build log available on the comment above: #25745 (comment)

As Abril commented, it's highly recommended using the templates to avoid extra changes needed. You actual recipe misses conandata.yml and requires Conan 2.x only (Conan 1.x is still mandatory in CCI).

@braindigitalis
Copy link
Contributor Author

everything I've read says you're getting rid of Conan 1.x, surely we aren't expected to still support it too? I'm confused now as I've spent a long time getting this ready using only Conan 2 stuff.

@braindigitalis
Copy link
Contributor Author

your template appears to be full of irrelevant stuff that is not needed by this library. why do you have stuff in the template to toggle fPIC, this isn't usually something we let the end user decide on, as it simply breaks builds...? a lot of stuff in there for detection of compiler etc we do in dpp's makefile, seems a lot of it is to work around broken cmakelists on projects that are not properly portable... is this the intent of the template?

also this cannot be worked around.

CMake 3.16 or higher is required.  You are running version 3.15.7

we require cmake 3.16 (around since 2019!), what's with the ancient version?

@uilianries
Copy link
Member

uilianries commented Oct 29, 2024

everything I've read says you're getting rid of Conan 1.x, surely we aren't expected to still support it too? I'm confused now as I've spent a long time getting this ready using only Conan 2 stuff.

Most of Conan 2.x features are compatible to Conan 1.x so all recipes in CCI are running for both major versions. Indeed there are new features for Conan 2.x that really nice and will not be ported to Conan 1.x. Specifically speaking about your current recipe only coordinates_to_conandata is a Conan 2.x feature and not available for Conan 1.x

The Conan 1.x support in ConanCenterIndex should be ended by the next week, so you can wait until the next week to re-visit this PR, or just update your recipe to be both Conan 1.x and Conan 2.x compatible.

Still, we use conandata.yml and not Git commands directly in the recipe. The main advantage is being able to parse, validate and conandata; and re-use the same file for multiple versions without touching the recipe. The template shows how to use conandata in ConanCenterIndex: https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/cmake_package/all/conanfile.py#L98

Using conandata.yml will make your recipe compatible with both Conan 1.x and 2.x btw

your template appears to be full of irrelevant stuff that is not needed by this library. why do you have stuff in the template to toggle fPIC, this isn't usually something we let the end user decide on, as it simply breaks builds...? a lot of stuff in there for detection of compiler etc we do in dpp's makefile, seems a lot of it is to work around broken cmakelists on projects that are not properly portable... is this the intent of the template?

The template covers a general cmake project, most compatible with almost all project that are using cmake in ConanCenterIndex. The fPIC and shared option are configurable yes, and for most of projects it's transparent as CMake itself is capable to manage it.

CMake 3.16 or higher is required. You are running version 3.15.7

Please, read the design decision from 2020: https://github.com/conan-io/tribe/blob/main/design/004-tools-cmake.md

In case you need >=3.15, which is totally fine, you can use tool_requirements, as indicate in the template: https://github.com/conan-io/conan-center-index/blob/master/docs/package_templates/cmake_package/all/conanfile.py#L95. The current recommendation for cmake is to include it with a version range like self.tool_requires("cmake/[>=3.16 <4]")

@conan-center-bot

This comment has been minimized.

@braindigitalis
Copy link
Contributor Author

how do i get the test errors please? They were messaged into this thread last time, this time i'm not seeing them?

@conan-center-bot

This comment has been minimized.

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.

Thanks! Some suggestions, otherwise looks good

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

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@braindigitalis
Copy link
Contributor Author

Fix for the test_package issue.

Also, when compiling locally for mac, I'm seeing:

[ 42%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/dave/version.cpp.o
[ 43%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/discordclient.cpp.o
[ 43%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/discordevents.cpp.o
[ 44%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/discordvoiceclient.cpp.o
[ 44%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/dispatcher.cpp.o
[ 45%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/dns.cpp.o
[ 45%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/dtemplate.cpp.o
[ 46%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/emoji.cpp.o
[ 46%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/entitlement.cpp.o
[ 47%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/etf.cpp.o
[ 47%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/events/automod_rule_create.cpp.o
/Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/src/dpp/dave/session.cpp:250:92: warning: adding 'uint64_t' (aka 'unsigned long long') to a string does not append to the string [-Wstring-plus-int]
  250 |                 creator.log(dpp::ll_warning, "Attempted to verify credential for unrecognized user ID: " + uid);
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/src/dpp/dave/session.cpp:250:92: note: use array indexing to silence this warning
  250 |                 creator.log(dpp::ll_warning, "Attempted to verify credential for unrecognized user ID: " + uid);
      |                                                                                                          ^    
      |                                              &                                                           [    ]
/Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/src/dpp/dave/session.cpp:643:51: warning: adding 'uint64_t' (aka 'unsigned long long') to a string does not append to the string [-Wstring-plus-int]
  643 |                 throw std::invalid_argument("Unknown user ID: " + user_id);
      |                                             ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/src/dpp/dave/session.cpp:643:51: note: use array indexing to silence this warning
  643 |                 throw std::invalid_argument("Unknown user ID: " + user_id);
      |                                                                 ^        
      |                                             &                   [        ]
[ 48%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/events/automod_rule_delete.cpp.o
In file included from /Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/src/dpp/discordvoiceclient.cpp:32:
In file included from /Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/src/dpp/voice/enabled/enabled.h:42:
In file included from /Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/library/../include/dpp/isa_detection.h:24:
/Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/src/library/../include/dpp/isa/neon.h:59:75: error: extraneous ')' before ';'
   59 |                         neon_float increment_vector = vmulq_f32(vdupq_n_f32(increment), floats));
      |                                                                                                ^
[ 48%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/events/automod_rule_execute.cpp.o
[ 49%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/events/automod_rule_update.cpp.o
[ 49%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/events/channel_create.cpp.o
[ 50%] Building CXX object library/CMakeFiles/dpp.dir/__/src/dpp/events/channel_delete.cpp.o
1 error generated.
make[2]: *** [library/CMakeFiles/dpp.dir/__/src/dpp/discordvoiceclient.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
2 warnings generated.
make[1]: *** [library/CMakeFiles/dpp.dir/all] Error 2
make: *** [all] Error 2

dpp/10.0.34: ERROR: 
Package 'af8638d5202ad1fe911bcc8efbce95e7cc7fc7cc' build failed
dpp/10.0.34: WARN: Build folder /Users/abril/coding/conan-center-index/recipes/dpp/all/conan_home/p/b/dpp55d84446a36e9/b/build/Release
ERROR: dpp/10.0.34: Error in build() method, line 56
	cmake.build()
	ConanException: Error 2 while executing

Running with

arch=armv8
build_type=Release
compiler=apple-clang
compiler.cppstd=gnu17
compiler.libcxx=libc++
compiler.version=16
os=Macos

Is this a known issue? Thanks!

thanks, this is a known issue, but i forgot to add a workaround. We just need this:

tc.cache_variables["AVX_TYPE"] = "AVX0"

to force DPP to not attempt to enable AVX/NEON for now, until this can be patched properly upstream. It isn't something i'd want to try and hack in downstream with a patch in conan, and it doesnt cause any issue to disable NEON except a slight loss of performance when mixing received audio which isnt a feature many users use anyway.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@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.

Almost good to go.

recipes/dpp/all/conanfile.py Show resolved Hide resolved
recipes/dpp/all/conanfile.py Show resolved Hide resolved
recipes/dpp/all/conanfile.py Show resolved Hide resolved
braindigitalis and others added 2 commits October 30, 2024 10:23
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
@braindigitalis
Copy link
Contributor Author

I've bought through your recommendations @uilianries however I left the pr with ci running last night waiting for it to complete and this AM it still hasn't finished, how long should the CI take to run, was this just an anomaly? Thanks!

@braindigitalis
Copy link
Contributor Author

braindigitalis commented Oct 30, 2024

this now fails because it does not publicly bring in nlohmann json. only fails on 2.x... any ideas?

@conan-center-bot

This comment has been minimized.

Co-authored-by: Uilian Ries <uilianries@gmail.com>
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. The recipe looks good to be merged now. Thank you for your PR!

To understand the review process better, please, read https://github.com/conan-io/conan-center-index/blob/master/docs/review_process.md#getting-your-pull-request-reviewed

In summary, need to pass by the CI and have +2 approves.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

All green in build 26 (70740ce5d6fe2fe3305d32c1b9114ccb12b7ac4d):

  • dpp/10.0.34:
    Built 7 packages out of 11 (All logs)

Conan v2 pipeline ✔️

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

All green in build 26 (70740ce5d6fe2fe3305d32c1b9114ccb12b7ac4d):

  • dpp/10.0.34:
    Built 6 packages out of 10 (All logs)

@conan-center-bot conan-center-bot merged commit e2dc122 into conan-io:master Oct 30, 2024
8 checks passed
@uilianries
Copy link
Member

@braindigitalis Your PR has been merged and is now available in Conan Center:

$ conan search -r conancenter dpp 
Found 1 pkg/version recipes matching dpp in conancenter
conancenter
  dpp
    dpp/10.0.34

The https://conan.io/center updates the data base using a scheduled job, so the package should be listed there after some hours. Regards.

OMGtechy pushed a commit to OMGtechy/conan-center-index that referenced this pull request Dec 31, 2024
* initial conan commit for dpp

* fix

* fix

* test package

* fix: specify minimum cmake version

* change to 1.6

* we dont support users messing with fPIC

* homepage, url

* package_type

* conandata

* add get

* Update recipes/dpp/all/conandata.yml

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* revised from feedback

* import copy

* remove unused imports

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>

* enforce minimum compiler

* workaround issue with AVX not working on neon

* import Version

* rm the pkgconfig and cmake find module after install

* on windows, we need the includedirs and subdirs for the lib to be found

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Uilian Ries <uilianries@gmail.com>

* Update recipes/dpp/all/conanfile.py

Co-authored-by: Uilian Ries <uilianries@gmail.com>

* transitive headers for nlohmann (fixes: 2.x)

Co-authored-by: Uilian Ries <uilianries@gmail.com>

---------

Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
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.

5 participants