Improvements to CONTRIBUTING doc #2611
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
philrz
commented
Dec 8, 2022
| @@ -10,7 +10,8 @@ Install these dependencies: | |||
|
|
|||
| 1. [Node](https://nodejs.org/en/download/package-manager/) - the version specified in the `.node-version` file at the root folder. | |||
| 2. [Yarn](https://yarnpkg.com/) - a package manager for installing dependencies and starting Brim in dev mode. | |||
| 3. [Go](https://go.dev/doc/install) - to compile some Zed dependencies. | |||
| 3. [Go](https://go.dev/doc/install) - to compile some [Zed](https://zed.brimdata.io/) dependencies. | |||
| 4. Typical command line tools, such as `make`, `unzip`, and `curl` | |||
There was a problem hiding this comment.
I called these out because I found that on a minimal install of Ubuntu or Windows, some of these tools were not present. The debug logs generated when the yarn commands fail are fairly clear in explaining what's missing, but I figure if we mention these up front it might save the reader some debug cycles.
philrz
commented
Dec 8, 2022
| "release": "electron-builder --mac", | ||
| "release": "electron-builder --publish never", |
There was a problem hiding this comment.
In addition to dropping the --mac so it builds for the detected OS, the --publish never prevents it from seeking out a GitHub token env var to publish with and failing when it's not found. Our Actions Workflows generally handle the job for the GitHub-published ones.
jameskerr
approved these changes
Dec 9, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community issue #2569 led me to revisit our user-facing docs for creating release artifacts. I thought they might be in some ways confusing or incomplete when viewed by an outsider. Specifically, the
yarn releasethat was referenced previously was wired up to run on MacOS, which may have suited our internal needs but might confuse a user such as the one in #2569 running on a non-Mac OS. Since Electron Builder defaults to building for the detected OS, I've changed it over to that.We also only had code signing details for MacOS (and the ones we had needed a little adjustment), so I've included the ones for Windows as well.
I fixed a couple typos, fixed some links, and improved some other wording while I was in there.
I'll add PR comments to further point out a couple specific changes.