Add plugin interface for publication channels (#2687)#2701
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
This is awesome - and very near flawless. I've flagged a couple of things inline - one cleanup for coverage, and one edge case that wouldn't be obvious unless you're familiar with all the edge cases of Briefcase usage.
The only other thing that stood out is a bikeshed thing that didn't become obvious until I saw all the code ... could we replace publication_channels with the simpler channels in the module name? publication_channels is a bit long and unwieldy as a name, and we're not likely to have some other concept of channel that requires the clarification (if for no other reason than -c/--channel would be ambiguous). I think it makes sense to keep the full BasePublicationChannel name - but the package doesn't need to be as long, IMHO.
src/briefcase/commands/publish.py
Outdated
|
|
||
| # Confirm host compatibility, that all required tools are available, | ||
| # and that all app configurations are finalized. | ||
| self.finalize(apps=self.apps.values()) |
| tools: ToolCache | ||
| dist_path: Path | ||
|
|
||
| def distribution_path(self, app: AppConfig) -> Path: ... # pragma: no cover |
There was a problem hiding this comment.
I get why this #pragma is needed... rather than adding it to the specific method, could we add this as a global coverage exclusion on @runtime_checkable, as no runtime protocol will ever be covered by tests.
|
Regarding the specific question about mock usage: the existing pattern that you’ve observed/followed is mostly born out of the fact that a simple mock by itself doesn’t give us enough tracking - or, at least, if we did use a mock, we’d end up with a mock that is almost as complex as the Dummy that we’ve implemented, because there needs to be side effects to some function calls etc. That said - there are places in the code where we do use mocks; and for “simple interface adherence”, a mock might well be a better approach. Given that a Channel plugin is a relatively simple interface with a single method and some properties, it might be worth using them here. If you’re up for the experiment, then I wouldn’t oppose using them here (or in a follow up). |
b26872f to
6cba735
Compare
|
Thanks for the review! Both items addressed:
On naming: I was torn between On |
freakboy3742
left a comment
There was a problem hiding this comment.
Nice refactor on the apps-to-X handling!.
I've applied the refactor renaming publication_channels->channels; I get what you're saying about code being read more often than it's written. However, the name channel should be clear, as there's no other "channel" concept in Briefcase; and retaining the "full" name in class names retains the disambiguation in the once place where the classes are actually implemented. I've applied a couple of other minor fixes that I found along the way.
A couple of things fell out of live testing.
Firstly, there's a registration inconsistency with iOS and ios - this is an area where we've historically had some confusion, but in this case, the registered names need to be the platform_name and output_format as registered by the classes. I've rolled this into the changes that I've made.
However, even with this change, there's some issues using the plugin interface:
- As flagged inline, the implementation of
__call__for the Publish command includes a "does the app exist" check that circumvents the "if no distribution path, package the app" logic. I think this should be a straightforward case of deleting that block of code. - Even with that block gone, Android apps can't be built, because
distribution_path()requires knowledge of the packaging format.
I flagged this during the design process; but it's evidently going to a problem out of the box. Android projects have three potential distribution formats (aab, apk, and debug-apk). Play Store publication requires an AAB artefact; but in the more general case, the publication process would need to be aware of which packaging artefact has been selected.
iOS apps won't publish at present, because there's no implementation of package for iOS. However, that's part of the bigger "how to publish to the App Store" problem, rather than something specific to this PR.
Longer term, there's probably a need to factor out packaging format plugins in the same way as channel plugins are defined here. We don't have to implement that change here, but we should at least give some thought to how the two will interact.
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| **COMING SOON** | |||
There was a problem hiding this comment.
I guess we should drop this now... :-)
pyproject.toml
Outdated
| app = "briefcase.platforms.windows.app" | ||
| visualstudio = "briefcase.platforms.windows.visualstudio" | ||
|
|
||
| [project.entry-points."briefcase.publication_channels.ios.xcode"] |
There was a problem hiding this comment.
In order to find the plugin, this capitalisation needs to be:
| [project.entry-points."briefcase.publication_channels.ios.xcode"] | |
| [project.entry-points."briefcase.publication_channels.iOS.Xcode"] |
That points out an inconsistency in other registrations - briefcase.formats.iOS should be registering Xcode = "briefcase.platforms.iOS.xcode" (and similarly for macOS, and the commented out tvOS, watchOS...)
| :param platform: The target platform (e.g., "ios") | ||
| :param output_format: The output format (e.g., "xcode") |
There was a problem hiding this comment.
Platform identifiers and output formats have capitalisation:
| :param platform: The target platform (e.g., "ios") | |
| :param output_format: The output format (e.g., "xcode") | |
| :param platform: The target platform (e.g., "iOS") | |
| :param output_format: The output format (e.g., "Xcode") |
src/briefcase/commands/publish.py
Outdated
| # Check the apps have been built first. | ||
| for app_name, app in self.apps.items(): | ||
| binary_file = self.binary_path(app) | ||
| for app_name_key, app_obj in apps_to_publish.items(): | ||
| binary_file = self.binary_path(app_obj) | ||
| if not binary_file.exists(): | ||
| raise BriefcaseCommandError( | ||
| f"Application {app_name} has not been built. " | ||
| f"Application {app_name_key} has not been built. " | ||
| "Build (and test!) the app before publishing." | ||
| ) |
There was a problem hiding this comment.
This block of code circumvents the "cascading" call to package on L53/54.
|
If I understand correctly, the remaining Android issue is that |
I don't think it's that far from being fully resolved. If we replicate the argument handling from Longer term, we can look at factoring out the packaging formats as plugins in the same way as we've factored out publication plugins. Packaging will require a slightly more complicated API (as there's more steps involved) - but we already have a bunch of built-in implementations to prove out that the interface is complete; From the perspective of the publish command, I'm not sure anything changes beyond needing to know what packaging formats exist, which one has been selected, and how to get the filename for the distribution artefact. |
freakboy3742
left a comment
There was a problem hiding this comment.
Looks good; as promised, I've pushed one last piece that adds enough support for packaging formats that the Android backend is able to "successfully not package" an app.
I've also added an -u option, on the basis that updating before publication isn't an unreasonable thing to do if we're also cascading the package process.
I'm happy with the code at this point, so I'm marking this as accepted. I'd appreciate some eyeballs on my most recent update to confirm whether you see any problems with the approach; assuming you don't, I'll merge and call this done!
|
Looks good to me! Added some docs cross-linking between publish, the plugin interface, and the platform how-to guides. Also extracted the "no working channels" note into a shared snippet. The main |
|
@freakboy3742 That name is much more future-proof. Good catch! |
Introduce a
BasePublicationChannelABC andPublishCommandAPIprotocol for publishing apps to distribution channels viabriefcase publish, using dynamic discovery through entry points following the existing pattern for platforms and debuggers.Publication channel plugins:
briefcase.publication_channels.ios.xcode)publish_app(app, command, **options)to perform publication--channelChanges to
PublishCommand:importlib.metadata.entry_points()s3default and iOS/webpublish_appstubsAdd placeholder channels for iOS App Store (#2697) and Google Play Store (#2698) that raise
BriefcaseCommandErrorwhen invoked.Document the publication channel plugin interface in the plugins reference and link the publish command to it. Update publish command reference to reflect the new
--channelflag behavior.Consideration: Mock(spec=...) for test doubles
In this PR I followed the project's established pattern of hand-rolled dummy subclasses, which work well and are readable. But as the plugin interfaces grow (platforms, debuggers, publication channels all share the entry point →
class → instance pipeline), it might be worth considering
Mock(spec=BasePublicationChannel)as an alternative. Thespecargument enforces interface parity automatically, catching mismatches between the test double and the real class without manual upkeep. pytest-mock and itsmockerfixture make this particularly convenient in pytest.My ramblings on using
Mock(spec=...)for test doubles.PR Checklist: