Skip to content

Conversation

@Meng-Heng
Copy link
Collaborator

This PR is ready for reviews.

Test-bot: skip

@Meng-Heng Meng-Heng added this to the A19S22 milestone Feb 6, 2026
@Meng-Heng Meng-Heng added the maint label Feb 6, 2026
@keymanapp-test-bot
Copy link

User Test Results

Test specification and instructions

User tests are not required

#
_builder_get_operating_system() {
declare -g BUILDER_OS
# Note: on macOS, bash version 4 or above is needed for `declare -g`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder instead of putting the comment here, if we add a note in the README.md file to mention/link to the Shared Dependencies in the main repo.

Did you have to use HomeBrew to upgrade Bash?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used HomeBrew. Linking to the docs would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, as Darcy wrote, this is already documented at:
https://github.com/keymanapp/keyman/blob/2721c74a9188295320094da5237b8066a7387d08/docs/build/macos.md#L87

We could make this more visible in README.md for each of the sites.

I don't think we should add dependency comments into builder.inc.sh -- this is too buried to be useful.

Also, changes to builder.inc.sh need to be made in the keyman repo, because that's the primary version, but changes require builds on all platforms, and then ripple across to websites too, so best to avoid making changes unless really needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this is not the best place for it. I'm glad I was able to found out the reason behind my error and this will be a note for me to remember. Since there's already a document for this, I'll go ahead and close this PR. I'm still unsure on what to add for the README file so I won't do that now.

@Meng-Heng Meng-Heng closed this Feb 9, 2026
@github-project-automation github-project-automation bot moved this from Todo to Done in Keyman Feb 9, 2026
@mcdurdin mcdurdin deleted the maint/builder.inc.sh branch February 9, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants