macOS: New implementation of service publication

[cathyjf]

The current implementation of service publication on macOS uses avahi-client, but the majority of macOS machines do not have Avahi installed because macOS provides a native alternative (mDNSresponder), meaning that there is no reason to install Avahi.

The current implementation also attempts to load the Avahi client libraries using dlopen(3), which has a variety of restrictions on macOS, such as only being willing to load from certain directories. Depending on where the Avahi binaries are installed, they might not be loadable through the current invocation of dlopen(3).

Instead of using an Avahi client on macOS, it makes more sense to use the native macOS API for publishing services via mDNSresponder. This commit supplies such an implementation that uses the macOS native API. It also has the advantage of being much simpler than the previous implementation. Furthermore, this new implementation works on all macOS machines, because it relies only on native APIs, rather than on third-party software that is not commonly installed on macOS.

Type of Change

• [X] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [X] Dependency update - Removes a dependency on macOS (Avahi)
• [ ] Documentation update (changes to documentation)
• [ ] Repository update (changes to repository files, e.g. .github/...)

Checklist

• [X] My code follows the style guidelines of this project
  • My code uses the same style as the old implementation that I have replaced.
• [X] I have performed a self-review of my own code
• [X] I have commented my code, particularly in hard-to-understand areas
• [ ] I have added or updated the in code docstring/documentation-blocks for new or existing methods/components
  • N/A

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch must be updated before it can be merged. You must also Allow edits from maintainers.

• [X] I want maintainers to keep my branch updated

[Hazer]

@cathyjf that's something I wanted to investigate in the future. Thank you for your PR, this great.

Can you remove avahi from the Portfile too?

[cathyjf]

Can you remove avahi from the Portfile too?

Done. I also removed a reference to it from the documentation.

[ReenigneArcher]

• Can you add avahi here?

  https://github.com/LizardByte/Sunshine/blob/a940cdb394055139ca6a964289f414da562452e3/packaging/sunshine.rb#L39-L57

• I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

  Why did you mark it n/a? Anything you can add documentation blocks to would be immensely helpful as I have a goal to get to a point where everything is documented. Reference: https://docs.lizardbyte.dev/projects/sunshine/en/master/source_code/source_code.html

• Can you name the anonymous namespace?

[cathyjf]

• Can you add avahi here?

Done.

• Anything you can add documentation blocks to would be immensely helpful as I have a goal to get to a point where everything is documented.

I've now added a healthy additional amount of documentation.

• Can you name the anonymous namespace?

The C++ Core Guidelines ("a collaborative effort led by Bjarne Stroustrup") state in section SF.22: "Use an unnamed (anonymous) namespace for all internal/non-exported entities". I agree with this guideline. The use of the anonymous namespace makes it clear that the items inside it are not part of the public interface of the file. Without using this idiom, it's hard to tell what is part of the interface and what is not. For example, for the GNU/Linux and Windows implementations of service publication, there are a ton of top-level functions, and it's not at all clear at a glance what is part of the interface and what is part of the implementation. In my new implementation, it's very clear, because everything is inside an anonymous namespace except for one function (start), which is the public interface.

I agree with you that something similar could be achieved by naming the anonymous namespace something like _internal or _private. However, then we would be relying on the name of the namespace to give a clue as to its purpose. By contrast, when we use an anonymous namespace, the compiler enforces that the items inside it have internal linkage and cannot be accessed outside of the translation unit (file). Therefore, I believe we should follow the established C++ idiom and leave this namespace unnamed. However, if you want to deviate from the common practice, I'm happy to change it -- just please confirm this in a follow-up reply.

[ReenigneArcher]

Thank you for the updates and the explanation!

[cathyjf]

I have fixed the lint error.

[cathyjf]

I have now fixed the clang-format error, which unfortunately did not show up until after I fixed the cmake-lint error with the previous modification. That's why I didn't fix both at once.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 5.55556% with 34 lines in your changes missing coverage. Please review.

Project coverage is 8.98%. Comparing base (37b60fb) to head (a555b15). Report is 135 commits behind head on master.

Files with missing lines	Patch %	Lines
src/platform/macos/publish.cpp	5.55%	34 Missing :warning:

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #2786      +/-   ##
=========================================
+ Coverage    8.95%   8.98%   +0.03%     
=========================================
  Files          95      95              
  Lines       17386   17304      -82     
  Branches     8268    8232      -36     
=========================================
- Hits         1557    1555       -2     
+ Misses      12964   12884      -80     
  Partials     2865    2865              

Flag	Coverage Δ
Linux	6.78% <ø> (ø)
Windows	4.16% <ø> (ø)
macOS-12	10.09% <5.55%> (+0.07%)	:arrow_up:
macOS-13	9.99% <5.55%> (+0.07%)	:arrow_up:
macOS-14	10.30% <5.55%> (+0.08%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/platform/linux/publish.cpp	0.00% <ø> (ø)
src/platform/macos/publish.cpp	7.69% <5.55%> (+3.56%)	:arrow_up:

[ReenigneArcher]

Last question, do you have any ideas on how to unit test this?

[Hazer]

I haven't got the time to actually code review, but running this branch locally I got the DNS working just fine with Android, Switch and QT clients, I'll do a proper code review later.

[cathyjf]

Last question, do you have any ideas on how to unit test this?

It would be complicated to craft programmatic unit tests for this code. First, we would need to restructure the code so that where we currently directly call the macOS API functions (DNSServiceRegister, etc.), we would instead call some function objects that we define, so that, for testing purposes, the function objects can be replaced with fake versions. This restructuring would make the code more complicated. Next, we would need to actually implement fake versions of DNSServiceRegister and friends. The fake versions would not need to do any DNS operations, but they would need to interact with each other correctly, i.e., DNSServiceRegister would need to store the callback passed to it in some data structure; DNSServiceRefSockFD would need to return an actual file descriptor so that select can find that it's ready for reading (or alternatively we would need a fake version of select too); DNSServiceProcessResult would need to invoke the callback that was stored by DNSServiceRegister; and so on. Finally, we would then write some tests that store our fake versions of the functions inside the relevant function objects, before invoking the start function to test it.

After all that, we wouldn't really be able to test very much. We could write tests to verify that calling start causes the callback to eventually be called with the correct port. We could also write tests to ensure that failure cases don't crash and fail gracefully. The amount of functionality that we could verify with the tests is pretty limited.

The goal of writing tests for code is laudable but this isn't really a piece of code where it makes sense in my opinion to write unit tests, because the unit testing would be quite a bit more complicated than the code being tested, but not actually test very much.

[ReenigneArcher]

This unit test failures are not related to this PR. I need to investigate what's going on.