Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automate darwin install #43

Merged
merged 14 commits into from
May 16, 2024
Merged

Automate darwin install #43

merged 14 commits into from
May 16, 2024

Conversation

mzgoddard
Copy link
Contributor

This pull request depends on #42.

Add macos/darwin installation. This installs the voice for VoiceOver.

@mzgoddard
Copy link
Contributor Author

This change doesn't "install" keyboard support, that requires SIP to be disabled to add osascript to the System Settings > Security & Privacy > Accessibility list. The keyboard support step would best be done manually, which doesn't require SIP to be disabled.

README.md Outdated
@@ -32,20 +32,24 @@ which is required by the automation voice.

npm install -g @bocoup/windows-sapi-tts-engine-for-automation
Copy link
Contributor

Choose a reason for hiding this comment

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

I was unable to run this with the published package.

I had to do

chmod +x ./bin/at-driver

and ./bin/at-driver in place of all instances of at-driver

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this problem exists on the main branch, I've submitted a fix via a separate pull request.

README.md Outdated
If prompted for system administration permission, grant permission.

2. Start the server by executing the following command in a terminal:
3. Start the server by executing the following command in a terminal:

at-driver
Copy link
Contributor

Choose a reason for hiding this comment

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

This required sudo the first time in order to make the required directories

Copy link
Contributor

@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.

This change doesn't "install" keyboard support, that requires SIP to be disabled to add osascript to the System Settings > Security & Privacy > Accessibility list. The keyboard support step would best be done manually, which doesn't require SIP to be disabled.

This sounds like a good direction to me. It seems important to reflect this expectation in the installation script itself, even with the understanding that manual intervention would usually be necessary:

  • in the "interactive" setting (i.e. a developer typing at-driver install into their terminal), the script could prompt the user to enable keyboard support and pause until they do so
  • in the "unattended" setting (i.e. running at-driver install --unattended in a continuous integration environment), the script could attempt to perform the installation automatically (we know this will fail when SIP is enabled, but it would work in environments where SIP has been disabled, and I believe that includes GitHub Actions

These accommodations could of course be skipped entirely if the system is already configured to allow keyboard automation.

In pseudocode:

while hasKeyboardSupport() == false
  if unattendedInstall
    enableKeyboardSupport()
  else
    print('Please enable keyboard support and then press any key')
    waitForKeyPress()

@jugglinmike jugglinmike marked this pull request as ready for review April 8, 2024 23:24
@jugglinmike jugglinmike force-pushed the automate-darwin-ci-install branch from f46d337 to e05b38e Compare April 23, 2024 00:15
Copy link

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Other than the concern with running the uninstall command, but I don't think that has to be a blocker right now, considering this will run in CI (being tracked in #50)


/**
* @param {object} options
* @param {boolean} options.unattended - Whether installation should fail if

Choose a reason for hiding this comment

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

Very useful!

Copy link
Contributor

Choose a reason for hiding this comment

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

Glad you think so; I had trouble explaining it in a way that even kind of makes sense

@jugglinmike jugglinmike merged commit 74b5e29 into main May 16, 2024
6 checks passed
@jugglinmike jugglinmike deleted the automate-darwin-ci-install branch May 16, 2024 20:27
@jugglinmike
Copy link
Contributor

Thanks, @howard-e! Thanks, @stalgiag! Thanks, @mzgoddard!

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.

4 participants