Skip to content

Graceful exit if install script detects installation#26

Merged
ChrisC merged 5 commits intomainfrom
graceful-exit-on-install-test
Jul 3, 2025
Merged

Graceful exit if install script detects installation#26
ChrisC merged 5 commits intomainfrom
graceful-exit-on-install-test

Conversation

@ChrisC
Copy link
Contributor

@ChrisC ChrisC commented Jun 24, 2025

Rather than throwing an error when the install script detects that at-driver is already installed (which causes a CI process to fail if at-driver is already installed), let's just exit early and log what happened.

This is necessary to permit automated installs of at-driver on long-lived self-hosted runners.


ChrisC added 2 commits June 23, 2025 21:31
removes the deprecated win32 installer to avoid type errors
@ChrisC ChrisC requested a review from jugglinmike June 24, 2025 04:35
Copy link
Member

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing behavior is useful because it loudly surfaces contradictions in the end-user's faulty assumptions regarding the state of their system.

Since we're motivated by an operational need which is specific to our new deployment environment, I'd much rather address the problem in that context. This could technically be done without changing this project's code, but it wouldn't be pretty (we'd have to parse the standard error stream for this specific error message).

I can think of two alternative solutions which preserve the guardrail while also enabling our new use case in a stable way:

  • Exit with a distinct error code for the "already installed" case, document that error code, and check for it in the new GitHub Workflow
  • Expose a new sub-command to detect the presence of the server (e.g. at-driver check) and use it to guard the invocation of at-driver install in the new GitHub Workflow

The latter approach would also make this condition even easier to discern from the GitHub interface than the current patch (since GitHub would be aware whenever we "skipped" the at-driver install command and would render the step accordingly). That said, I'd support either solution.

@ChrisC
Copy link
Contributor Author

ChrisC commented Jun 24, 2025

The existing behavior is useful because it loudly surfaces contradictions in the end-user's faulty assumptions regarding the state of their system.

I don't agree with this assessment. With this change, the end-user is still pretty loudly notified that at-driver is already installed, and the installation process ends early.

I'm thinking about the experience using tools like homebrew, where it's not really that clear to the end-user whether something is flagged fully as an error or not. Here's a screenshot of what homebrew does when you try to re-install wget for example:
image

Here all I get is a string "warning", but it's not clear to me how the program actually handled the exception (nor do I really care, but I appreciate that it didn't just try to re-install automatically).

Additionally, as a CI user of this at-driver, I would greatly prefer the installer to gracefully handle this particular exception, instead of having to add complexity to our CI workflow to handle it.

If there's a really simple, widely adopted GH workflow convention for skipping steps on certain kinds of scenarios, I would be open to using that... but again, I'd rather not add any less conventional complexity to the workflow, unless it's really necessary (like in a situation where we have less control of the software we're trying to run on the workflow).

@jugglinmike
Copy link
Member

If there's a really simple, widely adopted GH workflow convention for skipping steps on certain kinds of scenarios, I would be open to using that

+- name: Check for the AT Driver server
+  id: checkForATDriver
+  working-directory: node_modules/.bin
+  run: ./at-driver check
+  continue-on-error: true
 - name: Configure the system to support the AT Driver server
   env:
     DEBUG: "*"
   working-directory: node_modules/.bin
   run: ./at-driver install --unattended
+  if: ${{ steps.checkForATDriver.outcome == 'failure' }}

Additionally, as a CI user of this at-driver, I would greatly prefer the installer to gracefully handle this particular exception, instead of having to add complexity to our CI workflow to handle it.

I think this speaks to a more subjective difference of maintenance preferences--one that we can't justify hashing out in the context of this feature. Since you're doing the legwork (indeed, since you've already done the legwork, in some senses), I'll leave the decision to you.

@jugglinmike
Copy link
Member

I hate to say it, but I'm having second thoughts about all of these solutions. The underlying problem is that the installation-detection logic is currently version-agnostic. That means that we can't properly support anyone who installs (e.g.) version 1.1 and later tries to update to version 1.2. With the patch proposed here, that second installation attempt would simply report Already installed without actually updating the system-level code on their machine.

I believe that safely supporting re-installation will mean extending the logic to determine which version is currently installed. There's probably some macOS-native way to do this, but finding the relevant documentation (assuming it even exists) could be a challenge. Alternatively, we might explore cramming the version number into the voice name so that every version has a distinct voice. An even less sophisticated (but more tractable) solution could be writing the version identifier to a text file placed in a stable location of the file system...

Stepping back a bit, I think the hazard is a credible threat to anyone who wants to use this software in the way we're preparing to use it. Like you, though, I'm not excited about complicating this patch given that it's just a small piece of a larger effort. I just now started to document this hazard with a new issue--something to be addressed with a follow-up patch--but I don't know if I have the stomach to deploy this change without a fix.

Maybe a good blend of "safe" and "expedient" (though certainly not "operationally efficient") would be to unconditionally uninstall the software before every installation attempt (e.g. at-driver uninstall || true; at-driver install).

As ever, though, I'm open to suggestion.

@ChrisC
Copy link
Contributor Author

ChrisC commented Jul 3, 2025

... With the patch proposed here, that second installation attempt would simply report Already installed without actually updating the system-level code on their machine.

I believe that safely supporting re-installation will mean extending the logic to determine which version is currently installed. There's probably some macOS-native way to do this, but finding the relevant documentation (assuming it even exists) could be a challenge. Alternatively, we might explore cramming the version number into the voice name so that every version has a distinct voice. An even less sophisticated (but more tractable) solution could be writing the version identifier to a text file placed in a stable location of the file system...

Having worked on fixing the uninstall script recently, unfortunately I don't think it's quite ready to be run repeatedly and still requires some closer examination. I like the idea of letting the user know that an update is available, but I think we can iterate here and follow up in a separate PR (especially since it may be a little complex to implement.) I can create an issue for the idea if you haven't already! But I'll go ahead land and publish this once I get the tests passing.

@ChrisC ChrisC force-pushed the graceful-exit-on-install-test branch from 325730c to e75f644 Compare July 3, 2025 21:25
@ChrisC ChrisC merged commit 729433a into main Jul 3, 2025
9 of 20 checks passed
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.

2 participants