Implement `Tool` base class #1093

rmartin16 · 2023-02-11T02:49:11Z

Changes

Implements Tool and ManagedTool base classes for all tools
Implements a "tool registry" that contains all defined Tools
- This is different than the ToolCache that contains verified Tool instances
Promotes git and xcode to proper Tool classes
Refactors UpgradeCommand to abstract out Tool implementation details
Completes typing for the tools' interfaces

Notes

The motivation for the typing changes is to be helpful and provide better IDE autocomplete; not to pass mypy.
- In fact, running mypy returns a torrent of issues...but it was very useful to verify my typing additions.
Therefore, I ended up removing nearly all of the more "eccentric" changes that were purely for typing accuracy but added little to dev ergonomics.
That said, feel free to call out any of the typing shenanigans you do not think tip the scales towards helpful.

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

freakboy3742 · 2023-02-11T09:30:57Z

src/briefcase/platforms/iOS/__init__.py

@@ -14,7 +14,7 @@ def verify_tools(self):
        # Require XCode 10.0.0. There's no particular reason for this
        # specific version, other than it's a nice round number that's
        # not *that* old at time of writing.
-        verify_xcode_install(self.tools, min_version=(10, 0, 0))
+        Xcode.verify(self.tools, min_version=(10, 0, 0))


This is no longer correct - we definitely need at least Xcode 13.0 at this point, due to file formats.

bumped to 13.0.0

freakboy3742

I've had an initial look through this; definitely looks like it's heading in the right direction.

Not sure if it was on your todo list or not - but the upgrade command can be significantly cleaned up once this is in place, because we can iterate through all tools with a consistent upgrade interface, rather than needing to list tools specifically.

rmartin16 · 2023-04-25T18:03:34Z

@freakboy3742

Finalizing the upgrade command...at a decision point.

In order to blindly loop over all the tools to identify the ones that can be upgraded, each Tool needs to actually be verified. This presents challenges because 1) certain tools are only supported on certain platforms and 2) some require parameters for initialization that won't be available to the upgrade command.

I don't think I'm too keen to potentially over-engineer this and add a whole layer of "host verification" like we did for the Commands themselves. Although, this is an option.

Alternatively, I can leave it like I have it now....where errors in trying to verify a Tool are interpreted as "the tool cannot be upgraded". I'm not a big fan of this since it isn't being explicit at all...but it works.

Some middle ground perhaps? like a class-level flag on each Tool to signify whether a Tool could qualify for being managed; if it's set False, just skip verification (and any attempt to upgrade) altogether.

Any thoughts/insight would be appreciated.

freakboy3742 · 2023-04-26T15:53:50Z

I don't think I'm too keen to potentially over-engineer this and add a whole layer of "host verification" like we did for the Commands themselves. Although, this is an option.

I agree - this isn't a feature that needs to be overly complicated.

Alternatively, I can leave it like I have it now....where errors in trying to verify a Tool are interpreted as "the tool cannot be upgraded". I'm not a big fan of this since it isn't being explicit at all...but it works.

Some middle ground perhaps? like a class-level flag on each Tool to signify whether a Tool could qualify for being managed; if it's set False, just skip verification (and any attempt to upgrade) altogether.

That was my first thought. A bare-bones check with a class attribute to perform a simple OS check (so you don't try to verify Xcode on Linux, or Linuxdeploy on macOS) avoids needless work; if it meets that criterion, try to upgrade, and report the problem if it didn't work.

rmartin16 · 2023-04-26T16:45:27Z

That was my first thought. A bare-bones check with a class attribute to perform a simple OS check (so you don't try to verify Xcode on Linux, or Linuxdeploy on macOS) avoids needless work; if it meets that criterion, try to upgrade, and report the problem if it didn't work.

Ok; a simple host OS check will be simple enough.

However, there are still other issues as long as introspecting whether a Tool is managed is gated behind verification. Some tools require a bit of contextual information to verify (e.g. LinuxDeployURLPlugin, DockerAppContext). That's why I'm thinking of implementing a class attribute for can_be_managed as hint that a Tool cannot ever be managed thereby avoiding the need to verify at all to only then see that tool.managed_install is False (or to get an exception from verification because of missing information).

freakboy3742 · 2023-04-26T21:24:55Z

That was my first thought. A bare-bones check with a class attribute to perform a simple OS check (so you don't try to verify Xcode on Linux, or Linuxdeploy on macOS) avoids needless work; if it meets that criterion, try to upgrade, and report the problem if it didn't work.

Ok; a simple host OS check will be simple enough.

However, there are still other issues as long as introspecting whether a Tool is managed is gated behind verification. Some tools require a bit of contextual information to verify (e.g. LinuxDeployURLPlugin, DockerAppContext). That's why I'm thinking of implementing a class attribute for can_be_managed as hint that a Tool cannot ever be managed thereby avoiding the need to verify at all to only then see that tool.managed_install is False (or to get an exception from verification because of missing information).

While there may be benefit for DockerAppContext to extend Tool, it's not a Tool in the sense of any of the others - it's a binding of Docker to an App. There's not really an "upgrade" process for these, unless you count purging/rebuilding the Docker image - but we don't have any way to track "all images that have been built", I think it's safe to put on the "can't be managed" list.

LinuxDeployURLPlugin is a weird one, because it's not a single plug-in. It's a wrapper allowing end-users to specify a plugin that is available at a URL. However, we don't have any specific details of that URL - we only have a unique hash. The same goes with LinuxDeployFilePlugin.

I think the best we can do for LinuxDeployPlugins is to ensure that the plugins folder has been purged, so that the next time a plugin is requested, we are certain to have a clean download.

Are there any other tools that are problematic?

rmartin16 · 2023-04-26T21:35:09Z

Are there any other tools that are problematic?

Just these since I implemented Host OS verification:

<class 'briefcase.integrations.linuxdeploy.LinuxDeployLocalFilePlugin'>
LinuxDeployLocalFilePlugin.__init__() missing 2 required positional arguments: 'plugin_path' and 'bundle_path'

<class 'briefcase.integrations.docker.DockerAppContext'>
DockerAppContext.verify() missing 6 required positional arguments: 'image_tag', 'dockerfile_path', 'app_base_path', 'host_bundle_path', 'host_data_path', and 'python_version'

<class 'briefcase.integrations.linuxdeploy.LinuxDeployURLPlugin'>
LinuxDeployURLPlugin.__init__() missing 1 required positional argument: 'url'

While there may be benefit for DockerAppContext to extend Tool, it's not a Tool in the sense of any of the others

This brings up something that's been periodically coming to mind: what is a tool exactly?

For instance, ADB is not currently treated like a first class tool....but I think there are arguments to do so.

I don't have fully formed thoughts on this yet...

I think the best we can do for LinuxDeployPlugins is to ensure that the plugins folder has been purged, so that the next time a plugin is requested, we are certain to have a clean download.

hmm...at first thought, this starts to go outside of the scope of the Upgrade command...I'll have to think about it more...

rmartin16 · 2023-04-27T18:12:32Z

Spent a little more time thinking about the more overarching ideas.

Please let me know your thoughts; I've pushed a prototype implementation of this.

Delineating Managed Tools

I've created a dedicated ManagedTool subclass of Tool for tools that can be managed; I say "can" because WiX, Android SDK, etc. can be either managed or unmanaged per user. This provides the hint the Upgrade command needs to avoid complications with Tools that cannot be easily verified and are always unmanaged anyway.

What is a tool?

The more relevant pragmatic question is simply why subclass Tool or ManagedTool:

Is added to Tool registry
Implements Tool interface
- Verification
- Installation management

The Tool registry is immediately applicable to the Upgrade command (obviously) to iterate the tools; but anything that would need to arbitrarily to be considered a part of Briefcase's toolset should be a Tool. For instance, a hypothetical future Briefcase Command to verify the readiness of the host system for running an arbitrary Briefcase command.

If something needs to implement its own verification before it can be used or if its install needs to be managed, it should be a Tool.

So, with that in mind, something like ADB can reasonably be considered not a Tool for Briefcase's purposes. It is provided as part of AndroidSDK and piggybacks on that SDK's verification and install management. As such, being a part of the Tool registry would be redundant (and probably problematic).

This categorization may present some ideological inconsistencies, though. The Download, NativeAppContext, and Subprocess tools would not actually be considered a Tool under this definition. They do not need the Tool interface and probably don't need to be a part of the Tool registry any more than anything else in the stdlib. Pragmatically, though, I think it's fine...

`DockerAppContext`

In some ways, DockerAppContext is similar to ADB in that verification of Docker should confirm DockerAppContext will work. However, as you point out, DockerAppContext is really about managing Docker images...and that's why it requires its own verification (but doesn't require any installation management...unless perhaps support for image management is added). So, I think its status as a Tool is appropriate.

linuxdeploy Plugins

The status quo for linuxdeploy plugins is they are not as well integrated; for instance, the Upgrade command does not work for upgrading the GTK and Qt plugins...much less arbitrary local file and URL plugins.

I have added support to upgrade the GTK and Qt plugins in this PR.

As for support for upgrading local file and URL plugins, I'm inclined to make that out of scope for this PR. I may look at it a bit but I think it deserves more a specific PR.

Tool Audit

Name	Is a Tool?	Managable?	FS Footprint?
Android SDK	X	X	X
ADB
Docker	X		X
DockerAppContext	X		X
Download	X
Flatpak	X		X
Git	X		X
JDK	X	X	X
LinuxDeployBase
LinuxDeployPluginBase
LinuxDeployGtkPlugin	X	X	X
LinuxDeployQtPlugin	X	X	X
LinuxDeployLocalFilePlugin	X
LinuxDeployURLPlugin	X		X
LinuxDeploy	X	X	X
RCEdit	X	X	X
Subprocess	X
NativeAppContext	X
VisualStudio	X		X
WindowsSDK	X		X
WiX	X	X	X
Xcode	X		X
XcodeCliTools	X		X

freakboy3742 · 2023-04-29T02:19:19Z

The Tool registry is immediately applicable to the Upgrade command (obviously) to iterate the tools; but anything that would need to arbitrarily to be considered a part of Briefcase's toolset should be a Tool. For instance, a hypothetical future Briefcase Command to verify the readiness of the host system for running an arbitrary Briefcase command.

If something needs to implement its own verification before it can be used or if its install needs to be managed, it should be a Tool.

That sounds like a really solid baseline definition to me.

This categorization may present some ideological inconsistencies, though. The Download, NativeAppContext, and Subprocess tools would not actually be considered a Tool under this definition. They do not need the Tool interface and probably don't need to be a part of the Tool registry any more than anything else in the stdlib. Pragmatically, though, I think it's fine...

I think this is the key insight. We need to differentiate between something:

being a Tool (in the sense that it is a subclass of Tool)
having a verify() API
being an attribute on the Tool Registry

Most of the Tools are case (1). NativeAppContext is case (2) - it has a verify API, but just because it has that API doesn't mean it's inherently manageable. Subprocess and Download are in case (3) really only using tool as a convenient namespace to gather content; they're not tools, but API wrappers around native capabilities.

In some ways, DockerAppContext is similar to ADB in that verification of Docker should confirm DockerAppContext will work. However, as you point out, DockerAppContext is really about managing Docker images...and that's why it requires its own verification (but doesn't require any installation management...unless perhaps support for image management is added). So, I think its status as a Tool is appropriate.

From a purely theoretical perspective, I'm sure this could be argued either way; ultimately, I think it's the practicalities of whether the abstraction is more help or harm that matters here.

As for support for upgrading local file and URL plugins, I'm inclined to make that out of scope for this PR. I may look at it a bit but I think it deserves more a specific PR.

Totally agreed on this one - although I'd also be completely comfortable with "delete all existing plugins" as a viable
upgrade" strategy, as the verification process already managed downloading plugins on demand.

…introduced

rmartin16

Finally wrapped this up; was waffling on some of the typing stuff. I've added comments to things I wanted to proactively call out. I also updated the top post with a PR summary.

rmartin16 · 2023-05-21T18:42:13Z

src/briefcase/integrations/docker.py

-    def docker_data_path(self):
+    def docker_data_path(self) -> PurePosixPath:
        """The briefcase data directory used inside container."""
-        return "/home/brutus/.cache/briefcase"
+        return PurePosixPath("/home/brutus/.cache/briefcase")


Returning a Path here instead to better align with all the other *_path command properties.

src/briefcase/integrations/linuxdeploy.py

rmartin16 · 2023-05-21T18:51:27Z

src/briefcase/integrations/linuxdeploy.py

+    def verify_install(
+        cls: type[LinuxDeployT],
+        tools: ToolCache,
+        install: bool = True,
+        **kwargs,
+    ) -> LinuxDeployT:


Adding explicit typing to cls helps the IDE figure out which actual LinuxDeploy class will be returned....since it will know which class is calling verify_install().

This is also how a type checker in general would figure out which Class will actually come out of this function.

src/briefcase/platforms/linux/flatpak.py

rmartin16 · 2023-05-21T19:15:34Z

tests/commands/upgrade/test_call.py

I had two goals for testing:

Each possible workflow for Upgrade was exercised

The expected actions (or lack thereof) are taken for all tools that were in scope for the test

rmartin16 · 2023-05-21T19:31:46Z

tests/integrations/base/test_ToolCache.py

    # Tools that are intentionally not annotated in ToolCache.
    tools_unannotated = {"cookiecutter"}
    # Tool names to exclude from the dynamic annotation checks; they are manually checked.
    tool_names_skip_dynamic_check = {
        "app_context",
        "git",
-        "xcode",
        "xcode_cli",
        "ETC_OS_RELEASE",
    }


The xcode_cli Tool remains in this list because the test is actually checking if all Tools are defined as modules in briefcase.integrations. That is true for xcode but not for xcode_cli. This is all a little wonky but really just an edge case to all this pseudo type checking...

I guess an alternative would be to promote XcodeCliTools to its own module....but that doesn't seem especially necessary.

Agreed; they're sufficiently closely bound that I don't think it's worth splitting them out for the sake of making a test less confusing; however, it would be worth adding this comment into the code itself so that the context isn't forgotten.

rmartin16 · 2023-05-21T19:37:46Z

tests/integrations/linuxdeploy/conftest.py

The linuxdeploy tests were refactored to avoid using Dummy classes.

rmartin16 · 2023-05-21T19:52:25Z

src/briefcase/integrations/subprocess.py

-    def inner(sub, args, **kwargs):
+    def inner(*args: SubprocessArgsT, **kwargs):
        """Evaluate whether conditions are met to remove any dynamic elements in the
        console before returning control to Subprocess.

-        :param sub: Subprocess object
        :param args: list of implicit strings that will be run as subprocess command
        :return: the return value for the Subprocess method
        """
+        sub: Subprocess = args[0]
+        sub_args: SubprocessArgsT = args[1]
+        pos_args: Sequence[Any] = args[2:]
+


Properly typing a method decorator is near impossible...

Nonetheless, exposing the signature this way and manually extracting the arguments like this helps trick the IDE in to showing a more accurate signature when attempting to autocomplete. That's the goal anyway....

This also returns support for positional arguments....since this would've just errored previously.

Hrm... not sure how I feel about this. When you have to dramatically change your coding style to appease a tool, my eyebrows start to raise... are we sure the juice is worth the squeeze in this case?

That's fair; I reverted it (but did add support for positional arguments).

I'll try to get proper typing for the decorator working.

freakboy3742

Woof... that's a monster :-)

tl;dr - this mostly looks good to me. The parts that are actually about the Tool/ManagedTool upgrade seem mostly uncontroversial, and a good improvement.

Most of the issues that I saw are with the typing changes. For the most part, they're uncontroversial as well; the problems mostly fall into "what is the right way to type?" gap. It's entirely possible you're doing the right thing, and that it's my own knowledge that is lacking.

My bigger concern with the typing is maintaining the types - unless they're being actively maintained, they're going to decay, at which point they become more harm than good. You've clearly got something enabled in your editor that is checking these; but unless we're all using the same editor, we're not going to benefit. Is there any way to automate type checks as a CI concern? Do we just need to bite the bullet and add Mypy?

src/briefcase/integrations/android_sdk.py

freakboy3742 · 2023-05-22T00:11:43Z

src/briefcase/integrations/android_sdk.py

@@ -1258,7 +1270,7 @@ def run(self, *arguments, quiet=False):
                raise InvalidDeviceError("device id", self.device) from e
            raise

-    def install_apk(self, apk_path):
+    def install_apk(self, apk_path: SubprocessArgT):


Is this right? Shouldn't it be a str/Path?

SubprocessArgT is Union[str, Path]....so, yes. But I think I see your point. I typed this based on the usage of apk_path within install_apk() instead of how the signature should be presented to devs.

I'll update it to str | Path.

freakboy3742 · 2023-05-22T00:12:30Z

src/briefcase/integrations/android_sdk.py

@@ -1342,7 +1354,7 @@ def start_app(self, package, activity, passthrough):
                f"Unable to start {package}/{activity} on {self.device}"
            ) from e

-    def logcat(self, pid):
+    def logcat(self, pid: str):


Is it a str? seems like it should be an int...

Based on the current implementation, it must be a string because subprocess.Popen() does not accept ints in the command line to pass to the OS.

We can coerce it to a str, though.

freakboy3742 · 2023-05-22T00:56:35Z

src/briefcase/integrations/subprocess.py

-    def inner(sub, args, **kwargs):
+    def inner(*args: SubprocessArgsT, **kwargs):
        """Evaluate whether conditions are met to remove any dynamic elements in the
        console before returning control to Subprocess.

-        :param sub: Subprocess object
        :param args: list of implicit strings that will be run as subprocess command
        :return: the return value for the Subprocess method
        """
+        sub: Subprocess = args[0]
+        sub_args: SubprocessArgsT = args[1]
+        pos_args: Sequence[Any] = args[2:]
+


Hrm... not sure how I feel about this. When you have to dramatically change your coding style to appease a tool, my eyebrows start to raise... are we sure the juice is worth the squeeze in this case?

freakboy3742 · 2023-05-22T01:24:53Z

src/briefcase/integrations/xcode.py


+        ensure_xcode_is_installed(tools=tools, min_version=min_version)


Not sure it really matters, but any reason for keeping these standalone functions rather than methods on the class?

Originally, I was probably rationalizing leaving them to avoid churn....but then this PR exploded a bit...

Converted them to methods.

freakboy3742 · 2023-05-22T01:57:09Z

tests/integrations/base/test_ToolCache.py

    # Tools that are intentionally not annotated in ToolCache.
    tools_unannotated = {"cookiecutter"}
    # Tool names to exclude from the dynamic annotation checks; they are manually checked.
    tool_names_skip_dynamic_check = {
        "app_context",
        "git",
-        "xcode",
        "xcode_cli",
        "ETC_OS_RELEASE",
    }


Agreed; they're sufficiently closely bound that I don't think it's worth splitting them out for the sake of making a test less confusing; however, it would be worth adding this comment into the code itself so that the context isn't forgotten.

freakboy3742 · 2023-05-22T01:58:27Z

tests/integrations/base/test_ToolCache.py


    # Ensure defined Tool modules/classes are annotated in ToolCache.
    for tool_module_name in briefcase.integrations.__all__:
        if tool_module_name not in tools_unannotated:
            assert tool_module_name in ToolCache.__annotations__.keys()
-        for tool_name in tools_for_module(tool_module_name):
+        for tool_name in tools_for_module(tool_module_name).keys():


Is keys actually needed here? Isn't the default iterable over keys?

I distinctly remember something bombing out without this....but it isn't happening anymore... 🤔

I'll remove them.

freakboy3742 · 2023-05-22T01:58:39Z

tests/integrations/base/test_ToolCache.py

            if tool_name not in tool_klasses_skip_dynamic_checks:
                assert tool_name in ToolCache.__annotations__.values()

    # Ensure annotated tools use valid Tool names.
    for tool_name, tool_klass_name in ToolCache.__annotations__.items():
        if tool_name not in tool_names_skip_dynamic_check:
            assert tool_name in briefcase.integrations.__all__
-            assert tool_klass_name in tools_for_module(tool_name)
+            assert tool_klass_name in tools_for_module(tool_name).keys()


rmartin16 · 2023-05-22T03:42:32Z

tl;dr - this mostly looks good to me. The parts that are actually about the Tool/ManagedTool upgrade seem mostly uncontroversial, and a good improvement.

🥳

My bigger concern with the typing is maintaining the types - unless they're being actively maintained, they're going to decay, at which point they become more harm than good. You've clearly got something enabled in your editor that is checking these; but unless we're all using the same editor, we're not going to benefit.

PyCharm enables type checking by default; so, if you call a method using the wrong type for an argument, it'll add red squiggles under the argument. I'd have to imagine that VSCode supports something similar.

Although, your point still stands.

Is there any way to automate type checks as a CI concern? Do we just need to bite the bullet and add Mypy?

hmm....I can play around with this....but running mypy on Briefcase right now results in a flood of issues. A lot of the issues seem to result from ambiguity....since a lot of Briefcase code is so dynamic making static analysis difficult.

I can take a stab at it, though.

rmartin16 · 2023-05-26T23:08:53Z

Is there any way to automate type checks as a CI concern? Do we just need to bite the bullet and add Mypy?

If you're curious, I pushed a commit for what type checking support might start looking like. Admittedly, there might be better ways to do some of this.

Some complications and barriers:

Typing arbitrary JSON is tricky....TypedDict may be able to help
- This can be seen in managing the output from vswhere
- Although, the Apple command JSON is much more complicated....i'll have to return to it
Multiple inheritance and mixins
- This is why linuxdeploy.py is still a bit of a mess
- Python typing doesn't support an "Intersection" type; so, you can't say a variable is an object of multiple types as is necessary for LinuxDeploy....since it is both some subclass of LinuxDeployBase and Tool.
- This will likely prove to be an even larger issue given the prominent use of mixins elsewhere.
- Currently, I think the best recommendation is to create Protocols that represent the composed classes
Overloaded superclass signatures
- While typing supports overloading, it gets really wonky when you want to allow subclasses to optionally use some keyword arguments and also support keyword arguments unknown to the superclass.
- The most obvious way around this is to basically turn off typing and manually pop off kwargs....but then we may as well just put # type: ignore i guess
Platform-specific modules
- Similar to coverage, mypy complains about modules that are platform specific
- For instance, it says all the referenced attributes for winreg are missing on linux

So, running automated type checking may be possible....but not ultimately without some pain and compromise.

freakboy3742 · 2023-05-29T00:20:27Z

So, running automated type checking may be possible....but not ultimately without some pain and compromise.

Hrm... none of those seem especially appealing. I guess that means our options are:

Go on a huge "try to make this typing-correct" push with some combination of the approaches you've described
Leave the typing as is, make best-efforts to keep them correct, but accept that sometimes types might drift from reality
Pull out all the typing stuff completely.

3 seems like overkill, and 1 sounds like it's really not worth the effort (both in terms of the immediate refactoring that is required, and the ongoing headaches of maintaining it).

So - I guess that leaves 2.

This reverts commit c6f274b.

rmartin16 · 2023-05-29T18:59:58Z

So - I guess that leaves 2.

This was my conclusion as well. I have reverted the type-checking changes.

freakboy3742

As I said in my last review - the core Tool stuff all looks solid; and AFAICT all the typing stuff makes sense now as well (or, at least, as much sense as it's ever going to make :-) I've pushed one minor tweak adding a clarifying docstring, but otherwise I think this is good to go!

Nice work - you've definitely earned a celebratory beverage of choice on this one :-)

rmartin16 force-pushed the tool-base-class branch 4 times, most recently from 64588e3 to 0f3f9fb Compare February 11, 2023 03:38

freakboy3742 reviewed Feb 11, 2023

View reviewed changes

rmartin16 force-pushed the tool-base-class branch 2 times, most recently from a3c63d5 to 87d2272 Compare April 8, 2023 23:21

rmartin16 force-pushed the tool-base-class branch 2 times, most recently from b1bc051 to 8b7f6dc Compare April 21, 2023 23:33

rmartin16 force-pushed the tool-base-class branch from 47a30af to 61a3080 Compare April 25, 2023 14:52

rmartin16 force-pushed the tool-base-class branch 4 times, most recently from 6297f57 to 47f30ea Compare April 26, 2023 20:15

rmartin16 force-pushed the tool-base-class branch from 47f30ea to 3527db0 Compare April 26, 2023 21:26

rmartin16 force-pushed the tool-base-class branch 2 times, most recently from f95c09c to 79a8061 Compare April 27, 2023 18:12

rmartin16 force-pushed the tool-base-class branch 4 times, most recently from dd24a5e to 3256dde Compare April 28, 2023 20:23

rmartin16 force-pushed the tool-base-class branch from 3256dde to 0813bd0 Compare May 2, 2023 14:28

rmartin16 force-pushed the tool-base-class branch from ad03605 to 6552f3a Compare May 7, 2023 18:53

Add tests for Tool base class implementation

05c04c1

rmartin16 force-pushed the tool-base-class branch from 6552f3a to 05c04c1 Compare May 7, 2023 19:05

rmartin16 mentioned this pull request May 8, 2023

Conditional Coverage Reporting with Platform Spoofing #1262

Merged

4 tasks

rmartin16 added 5 commits May 12, 2023 22:52

Merge branch 'main' into tool-base-class

f479c45

Stop passing app_name to Flatpak.run() after bundle_identifier …

0bb0699

…introduced

Finish typing Tools' interfaces

c8348af

Merge branch 'main' into tool-base-class

dc0e58b

Minor tweaks to tests for Tool base

08ce029

rmartin16 commented May 21, 2023

View reviewed changes

rmartin16 marked this pull request as ready for review May 21, 2023 20:18

rmartin16 requested a review from freakboy3742 May 21, 2023 20:18

freakboy3742 requested changes May 22, 2023

View reviewed changes

rmartin16 added 4 commits May 22, 2023 11:33

Add None type for arguments that default to None and misc fixes

20f4ffb

Convert Xcode verification functions to methods

d1cf602

Revert typing hack for ensure_console_is_safe decorator

7d07c5c

[WIP] type checking for integrations

c6f274b

rmartin16 added 2 commits May 29, 2023 14:13

Revert "[WIP] type checking for integrations"

a1564bf

This reverts commit c6f274b.

Merge branch 'main' into tool-base-class

dd083ad

rmartin16 requested a review from freakboy3742 May 29, 2023 19:00

rmartin16 mentioned this pull request May 29, 2023

Add support to upgrade user-defined linuxdeploy plugins #1297

Open

Add clarifying comment about test.

cf396d6

freakboy3742 approved these changes May 29, 2023

View reviewed changes

freakboy3742 merged commit 2e0fe09 into beeware:main May 30, 2023

rmartin16 deleted the tool-base-class branch May 30, 2023 00:37

rmartin16 mentioned this pull request Jul 10, 2023

Building AppImages on macOS is No Longer Supported on v0.3.15 #1352

Closed


		ensure_xcode_is_installed(tools=tools, min_version=min_version)

Implement Tool base class #1093

Implement Tool base class #1093

Conversation

rmartin16 commented Feb 11, 2023 • edited Loading

Changes

Notes

PR Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

rmartin16 commented Apr 25, 2023 • edited Loading

freakboy3742 commented Apr 26, 2023

rmartin16 commented Apr 26, 2023 • edited Loading

freakboy3742 commented Apr 26, 2023

rmartin16 commented Apr 26, 2023

rmartin16 commented Apr 27, 2023

Delineating Managed Tools

What is a tool?

DockerAppContext

linuxdeploy Plugins

freakboy3742 commented Apr 29, 2023

rmartin16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartin16 May 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartin16 May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartin16 May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartin16 commented May 22, 2023

rmartin16 commented May 26, 2023 • edited Loading

freakboy3742 commented May 29, 2023

rmartin16 commented May 29, 2023

freakboy3742 left a comment

Choose a reason for hiding this comment

Implement `Tool` base class #1093

Implement `Tool` base class #1093

rmartin16 commented Feb 11, 2023 •

edited

Loading

rmartin16 commented Apr 25, 2023 •

edited

Loading

rmartin16 commented Apr 26, 2023 •

edited

Loading

`DockerAppContext`

rmartin16 May 21, 2023 •

edited

Loading

rmartin16 May 22, 2023 •

edited

Loading

rmartin16 May 22, 2023 •

edited

Loading

rmartin16 commented May 26, 2023 •

edited

Loading