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

Update CONTRIBUTING.md #262

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Update CONTRIBUTING.md #262

merged 3 commits into from
Jan 10, 2024

Conversation

beyang
Copy link
Member

@beyang beyang commented Jan 8, 2024

  • remove sourcegraph/sourcegraph dependency
  • prefer asdf to install deps
  • break into subsections

Test plan

Dev docs only

- remove sourcegraph/sourcegraph dependency
- prefer `asdf` to install deps
- break into subsections
CONTRIBUTING.md Outdated
- https://plugins.jetbrains.com/plugin/14912-ktfmt
- [Install `asdf`](https://asdf-vm.com/guide/getting-started.html)
- Run `asdf install` in the root directory of this repository
- If not using `asdf`, you can install the programs in `.tool-versions` through other means, but ensure the versions match.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not reference nodejs & pnpm (.tool-versions) so directly. They are the agent's requirements (not plugin's). Hence, asdf mention seems to be redundant here too.

The agent has its own .tool-versions. Having the same lib version declarations in these two files may lead to inconsistency. What if the agent specifies nodejs 20.4.0 but the plugin tells it's 18.18.2 (this is the actual case right now)?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to keep .tool-versions in sync with the agent .tool-versions. I have hit on issues where building the agent from source fails if I don't have the right dependencies where I run ./gradlew :runIde

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need both .tool-windows if jetbrains on itself does not need pnpm?
I hit the same issues when I tried first time to build everything, but at that time I haven't got asdf installed.
My understanding is that when grade will run pnpm in the cody dir, it should automatically read asdf settings from cody and use proper runtime automatically. Isn't that the case?

As for asdf installation and other dependencies needed for cody I thin we should simply link to the:
https://github.com/sourcegraph/cody/blob/main/doc/dev/index.md
This way we won't need to manually keep both docs in sync.

Copy link
Member

Choose a reason for hiding this comment

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

Agree that we should ideally pick up the asdf settings from the Cody repo. I haven't gotten this working, but I haven't spent much time investigating it either because I've been able to work around it by having the right environment when running Gradle.

I'm fine either way we go.

BTW, I use rtx instead of asdf because it it's more intuitive and works more reliably for me. rtx is compatible with asdf .tool-version files.

@beyang beyang merged commit aeb9f18 into main Jan 10, 2024
1 check passed
@beyang beyang deleted the bl/update-docs branch January 10, 2024 22:00
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.

5 participants