Hazer
commented
2 days ago @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?
Open cathyjf opened 2 days ago
Hazer
commented
2 days ago @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
commented
2 days ago Can you remove avahi from the Portfile too?
Done. I also removed a reference to it from the documentation.
ReenigneArcher
commented
1 day ago Can you add avahi here?
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
commented
1 day ago
- Can you add
avahihere?
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
commented
1 day ago Thank you for the updates and the explanation!
cathyjf
commented
1 day ago I have fixed the lint error.
cathyjf
commented
1 day ago 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]
commented
23 hours ago Attention: Patch coverage is 5.55556% with 34 lines in your changes missing coverage. Please review.
Project coverage is 8.98%. Comparing base (
90fd371) to head (cb92e34). Report is 8 commits behind head on master.
ReenigneArcher
commented
23 hours ago Last question, do you have any ideas on how to unit test this?
Hazer
commented
17 hours ago 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
commented
58 minutes ago 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.
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 ofdlopen(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
.github/...)Checklist
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.