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

Add nmos-cpp #6891

Merged
merged 34 commits into from
Sep 2, 2021
Merged

Add nmos-cpp #6891

merged 34 commits into from
Sep 2, 2021

Conversation

garethsb
Copy link
Contributor

@garethsb garethsb commented Aug 17, 2021

Specify library name and version: nmos-cpp

This is a new addition to ConanCenter. This library is an implementation of the AMWA Networked Media Open Specifications in C++, licensed under the terms of the Apache License 2.0.

Done: The conandata.yml currently refers to sources from my fork of nmos-cpp, but that will be switched to the upstream when sony/nmos-cpp#195 is merged in the next few days.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@garethsb
Copy link
Contributor Author

garethsb commented Aug 17, 2021

Now run with conan-center hook locally: log

Build failures so far are all due to missing dns_sd.h, which is provided by either Bonjour or Avahi (with its compatibility library) depending on platform/choice.
How should we handle dependencies like these? SystemPackageTool?

On Linux, to use Avahi, you might want something like:

sudo apt-get install -f libavahi-compat-libdnssd-dev libnss-mdns avahi-utils

On Windows, the build dependencies are included in the source, but to run (e.g. the test_package), the Bonjour libraries need to be installed. That can currently be achieved with:

curl -L https://download.info.apple.com/Mac_OS_X/061-8098.20100603.gthyu/BonjourPSSetup.exe -o BonjourPSSetup.exe -q
& 7z.exe e BonjourPSSetup.exe Bonjour64.msi -y
msiexec /i Bonjour64.msi /qn /norestart

(That download URL has been stable for some time. Other ways we've found of getting Bonjour64.msi from Apple Downloads (e.g. via the Bonjour SDK itself) require an Apple ID.)

@garethsb
Copy link
Contributor Author

Hi @jgsogo and @danimtb, please would you advise me on the best way to approach the dependency on system-installed packages like these? Thanks!

@garethsb
Copy link
Contributor Author

Does the most recent continuous-integration/jenkins/pr-merge failure with no logs mean the CI simply rejected to build the recipe after I added system_requirements?

@prince-chrismc
Copy link
Contributor

👋 @SSE4 @uilianries @SpaceIm @madebr This PR can you some extra eyes please 🙏

@prince-chrismc
Copy link
Contributor

There's no commit status on your PRs 🤔
You'll need to retrigger CI, close the pr wait 10s and then re-open it 🔁
See if that helps

@garethsb garethsb closed this Aug 18, 2021
@garethsb garethsb reopened this Aug 18, 2021
@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

@garethsb We don't allow system_requirements for regular packages, instead, you need to open a PR for each system requirement and package them as a Conan package:

https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#kb-h032-system-requirements

The problem is, we can't track system packages and they may affect the package binaries/libraries. So if we package them, we can decide to use as build requirement only and also track which package name is installed at least. This is not a new directive, it's an experience from years of Bincrafters and Conan Community.

If you really need those system libraries, I recommend you first opening a PR for them. You can use glu as example.

@garethsb
Copy link
Contributor Author

Thanks, @uilianries. I can try to extract 8251313 to a separate recipe. How should I approach the Windows install, as I mentioned in #6891 (comment)?

@madebr
Copy link
Contributor

madebr commented Aug 18, 2021

Does it need bonjour, or only mdns?
mdns' sources can be downloaded at https://opensource.apple.com/tarballs/mDNSResponder/

@garethsb
Copy link
Contributor Author

garethsb commented Aug 18, 2021

Does it need bonjour, or only mdns?
mdns' sources can be downloaded at https://opensource.apple.com/tarballs/mDNSResponder/

On Linux, nmos-cpp can be built against mDNSResponder or against Avahi's compatibility library and header file. In both cases, there's both a client library that is linked and a daemon which the client lib talks to.

In my experience, developers prefer to work with the Avahi daemon, and use the compat lib. On Windows however, the Bonjour SDK is the simplest route.

@uilianries
Copy link
Member

@garethsb You are in a quagmire, for each package you try to solve, you first need to package another

BonjourPSSetup requires a separated package, similar to bazel. It's called installer package, because you use an installer and copy artifacts.

After that, you can package those system packages. Each one in a separated PR, the CI bot does not allow more than one project/recipe per PR. Fell free to ask for help when developing, as you can see, your case is not a regular recipe.

@prince-chrismc
Copy link
Contributor

Does mDNSResponder work for the windows build?

@garethsb
Copy link
Contributor Author

Does mDNSResponder work for the windows build?

It does. The issue is that you can't run more than one of these daemons / Windows services at a time, and I was hoping I could default to the most commonly used dependency on each platform (Avahi on Linux, Bonjour on Windows and macOS)... But writing a simple recipe to download and build mDNSResponder sources from Apple and depending on that in the nmos-cpp recipe - at least by default - could be a way to go...

@prince-chrismc
Copy link
Contributor

Here we can have

    def requirements(self):
        if self.setting.os == "Windows":
            # bonjour system here
        Else:
             # avahi or mdnsresponder
         

@garethsb
Copy link
Contributor Author

Here we can have

    def requirements(self):
        if self.setting.os == "Windows":
            # bonjour system here
        Else:
             # avahi or mdnsresponder
         

Yes... so long as there's an option to be able to pick and choose, since mDNSResponder can be made to work on Windows as well as the official, aged, Bonjour SDK 3.0.0 package, but the provided Visual Studio solution is out-of-date. On Linux, either mDNSResponder or Avahi works, ish, but the Avahi Bonjour compatibility library is missing some features, and Apple's mDNSResponder port (mDNSPosix) is missing unicast support (without a patch that I have lying around for one version but not the latest). Avahi could be built from source or installed with SystemPackageTool. Not yet sure where to focus, which has most chance of being accepted by CCI and which will satisfy most users.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

#7129 bumps it

@uilianries
Copy link
Member

I have a PR for Folly which is on same boat. Two profiles should solve it, but still the single profile approach can't block users.

recipes/nmos-cpp/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nmos-cpp/all/conandata.yml Show resolved Hide resolved
recipes/nmos-cpp/all/conanfile.py Show resolved Hide resolved
recipes/nmos-cpp/all/conanfile.py Show resolved Hide resolved
recipes/nmos-cpp/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nmos-cpp/all/conanfile.py Outdated Show resolved Hide resolved
recipes/nmos-cpp/all/conanfile.py Outdated Show resolved Hide resolved
@garethsb
Copy link
Contributor Author

garethsb commented Sep 1, 2021

You probably need to remove the openssl bump to work around the last cci error around openssl clashing with cmake's openssl version.
Alternatively, try the following in build requirements:

def build_requirements(self):
    # FIXME: optionally require a recent cmake version to avoid a potential openssl version clash
    if tools.Version(CMake(self).version < "3.17":
        self.build_requires("cmake/3.21.1")

(Untested)

What led me to add the build_requires was a previous CCI build error because some of the runners only had CMake 3.16.x, so if I understand correctly, I would expect to see the new failure still on those runners with the suggestion.

For now, I'll revert to openssl/1.1.1k.

@conan-center-bot

This comment has been minimized.

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

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Sep 1, 2021
prince-chrismc
prince-chrismc previously approved these changes Sep 1, 2021
@garethsb garethsb closed this Sep 2, 2021
@garethsb garethsb reopened this Sep 2, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@garethsb garethsb requested a review from jgsogo September 2, 2021 14:10
@conan-center-bot
Copy link
Collaborator

All green in build 45 (0393f972eebed0e4ea8bcdf07588b0ea633be038):

  • nmos-cpp/cci.20210902@:
    All packages built successfully! (All logs)

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.

None yet

8 participants