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

feat: env-doctor script #10078

Merged
merged 2 commits into from
Sep 17, 2024
Merged

feat: env-doctor script #10078

merged 2 commits into from
Sep 17, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Sep 12, 2024

incidental

Description

Adds yarn doctor which calls scripts/env-doctor.sh to diagnose and repair common problems with the development environment.

I made this using Aider:

Main model: claude-3-5-sonnet-20240620 with diff edit format, infinite output

# 2024-09-12 08:25:25.666744
+/add scripts/env-doctor.sh

# 2024-09-12 08:31:22.746300
+make env-doctor run a series of diagnostics on the development environment. each diagnostic has one remedy that is offered if its diagnostic test has an error exit code. each remedy is shown to the user and they are prompted to confirm whether to execute it. Each diagnostic and remedy has a description displayed to the user instead of the code. A user can reply "Yes" or"No", or "All" which answers Yes to the remaining questions. Each answer has an alias of its first letter (Y, N, A).

# 2024-09-12 08:35:00.044883
+add a check that `node --version` has the same version as in `.node-version`

# 2024-09-12 08:36:58.653105
+use fnm instead of nvm

# 2024-09-12 08:39:32.091136
+add a run_recommendation function that does not have a diagnostic and one of the recommendations is to run scripts/configure-vscode.sh to "Configure VS Code with recommended settings"

# 2024-09-12 08:45:13.008441
+add a last diagnostic "Build repo" that runs "yarn build". If that fails the remedy offered is to "git clean -fdx && yarn install && yarn build". If a remedy exits non-zero then alert the user to seek more help and exit the script.

# 2024-09-12 08:46:03.698422
+replace the NPM check with a check that `yarn --version` matches the version in the 'packageManager' field of the root package.json

# 2024-09-12 08:47:23.858173
+add a recommendation to run "yarn doctor" within a3p-integration

I wish Aider would include the chat message in the commit. Aider-AI/aider@16856bb looks like it was supposed to.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Maybe docs.agoric.com gets some advice to use yarn doctor. It could save some of the setup steps.

Testing Considerations

Needs some testing on different machines. Though it doesn't have to be perfect. I expect it will get iterated upon whenever someone runs into a problem.

I've tried it a few times locally. Once I had "build": "exit 1" to test the build remedy.

Upgrade Considerations

n/a

@Chris-Hibbert
Copy link
Contributor

I checked out this PR and ran it to see what it would do for me.

  • It noticed that I hadn't installed vscode; I allowed it to install and configure. I don't use vscode routinely, and apparently hadn't installed it since re-installing all s/w on my machine months ago. It's occasionally useful for editing puml
  • While it was running yarn doctor in a3p-integration, I kept seeing messages about detached HEAD. I have no idea what that was trying to do. It ended with this:
HEAD is now at 2d8ccb7 Merge pull request #41 from agoric-labs/7012-add-network-version-command
xsnap-worker.mk:134: *** target pattern contains no `%'.  Stop.
Failed with Error: make exited with code 2
    at ChildProcess.<anonymous> (file:///Users/cth/agoric-sdk/a3p-integration/proposals/s:stake-bld/node_modules/@agoric/xsnap/src/build.js:43:18)
    at ChildProcess.emit (node:events:518:28)
    at ChildProcess._handle.onexit (node:internal/child_process:294:12)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

PROBLEM ^^^  After correcting, run doctor again.

after a few more warnings about ava and vercel, it reported All checks and recommendations complete.

checking proposal acceptance ...
found "yarn" packageManager, processing...
warning ava > @vercel/nft > [email protected]: Glob versions prior to v9 are no longer supported
warning ava > @vercel/nft > glob > [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning ava > @vercel/nft > @mapbox/node-pre-gyp > [email protected]: This package is no longer supported.
warning ava > @vercel/nft > @mapbox/node-pre-gyp > [email protected]: Rimraf versions prior to v4 are no longer supported
warning ava > @vercel/nft > @mapbox/node-pre-gyp > rimraf > [email protected]: Glob versions prior to v9 are no longer supported
warning ava > @vercel/nft > @mapbox/node-pre-gyp > npmlog > [email protected]: This package is no longer supported.
warning ava > @vercel/nft > @mapbox/node-pre-gyp > npmlog > [email protected]: This package is no longer supported.

scripts/env-doctor.sh Outdated Show resolved Hide resolved
@turadg turadg changed the title env-doctor script feat: env-doctor script Sep 16, 2024
Copy link

cloudflare-workers-and-pages bot commented Sep 16, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7d9f90e
Status: ✅  Deploy successful!
Preview URL: https://f189b50f.agoric-sdk.pages.dev
Branch Preview URL: https://ta-doctor.agoric-sdk.pages.dev

View logs

@turadg turadg requested a review from mhofman September 16, 2024 19:11
@turadg turadg added automerge:squash Automatically squash merge bypass:integration Prevent integration tests from running on PR labels Sep 16, 2024
"Build repo" \
"yarn build" \
"Clean, reinstall dependencies, and rebuild" \
"git clean -fdx "**/node_modules" "**/bundles" && yarn install && yarn build"
Copy link
Member

@mhofman mhofman Sep 16, 2024

Choose a reason for hiding this comment

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

I didn't know glob patterns were supported by git cli, but that makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

learned this from @kriskowal

Copy link
Member

Choose a reason for hiding this comment

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

Apparently globs are not working for me. Where can I find info on how it's supposed to work?

Copy link
Member

Choose a reason for hiding this comment

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

Ok I found that globs may not be enabled by default. Also I think that glob pattern doesn't actually work. What worked for me (more aggressive version with more things cleaned):

-- ':(glob)**/node_modules/**' ':(glob)**/bundles/**' ':(glob)**/dist/**' ':(glob)**/build/**'

Copy link
Member

Choose a reason for hiding this comment

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

Also I tend to do bin/agd build these days instead as that also builds the golang stuff for those of use who care. However it has its own "download deps" logic which may not be desirable here.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also try using exclusions instead of inclusions. But I'll leave this as an exercise for whoever runs into the problem next. This is meant to be a crowdsourced solution set

Copy link
Member

Choose a reason for hiding this comment

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

Inclusions is probably safer. It's just that I don't understand how this line even works given the quoting, and that glob patterns are not enabled by default.

@mhofman mhofman self-requested a review September 16, 2024 20:13
Copy link
Contributor

@ivanlei ivanlei left a comment

Choose a reason for hiding this comment

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

I reviewed rapidly, ran shellcheck, and asked ChatGPT4o for a code review. Feedback is a mix of all of that.

None of the feedback is required to be acted on before merging or really ever.

package.json Outdated
@@ -53,6 +53,7 @@
"docs:markdown-for-agoric-documentation-repo": "run-s docs:markdown-build 'docs:update-functions-path md'",
"docs:markdown-build": "typedoc --plugin typedoc-plugin-markdown --tsconfig tsconfig.build.json",
"docs:update-functions-path": "node ./scripts/update-typedoc-functions-path.cjs",
"doctor": "scripts/env-doctor.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

low priority:

The rest of the values in this JSON object reference the scripts directory with a leading .. Worth doing here?

@@ -0,0 +1,121 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

low priority:

ChatGPT suggested:

command -v brew >/dev/null 2>&1 || { echo "brew is required but not installed. Exiting."; exit 1; }
command -v yarn >/dev/null 2>&1 || { echo "yarn is required but not installed. Exiting."; exit 1; }

Copy link
Contributor

Choose a reason for hiding this comment

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

ChatGPT also suggests:

if [[ "$OSTYPE" == "linux-gnu"* ]]; then
  # bail with error
fi

echo -e "${RED}✗ Failed${NC}"
echo -e "${YELLOW}Remedy:${NC} $remedy_description"
if [[ $AUTO_YES != "true" ]]; then
read -p "Do you want to apply this remedy? (Yes/No/All) " answer
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should explicitly initialize AUTO_YES with false at the start of the script?

Copy link
Contributor

@rabi-siddique rabi-siddique left a comment

Choose a reason for hiding this comment

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

Cool Stuff!

@turadg turadg added automerge:rebase Automatically rebase updates, then merge and removed automerge:squash Automatically squash merge labels Sep 17, 2024
@mergify mergify bot merged commit 0c8c6c4 into master Sep 17, 2024
79 checks passed
@mergify mergify bot deleted the ta/doctor branch September 17, 2024 16:09
Comment on lines +5 to +10
# Check if running on macOS
if [[ "$(uname)" != "Darwin" ]]; then
echo "Error: This script only works on macOS."
echo "Your current operating system is: $(uname)"
exit 1
fi
Copy link
Member

Choose a reason for hiding this comment

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

Why is this MacOS only? A decent chunk of us are on Linux

Copy link
Member Author

Choose a reason for hiding this comment

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

Because some remedies use Homebrew. Also Linux uses are less likely to need it. ;-) But feel free to improve the script.

"Build repo" \
"yarn build" \
"Clean, reinstall dependencies, and rebuild" \
"git clean -fdx "**/node_modules" "**/bundles" && yarn install && yarn build"
Copy link
Member

Choose a reason for hiding this comment

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

Apparently globs are not working for me. Where can I find info on how it's supposed to work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants