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

Add avm to system path #3272

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Add avm to system path #3272

merged 3 commits into from
Sep 25, 2024

Conversation

shawazi
Copy link
Contributor

@shawazi shawazi commented Sep 24, 2024

Problem:

While following the documentation to install avm, my system was unable to locate the avm command. This was due to the absence of the Cargo binary directory ($HOME/.cargo/bin) in my system PATH. While this issue may stem from my own Rust installation, it might benefit users to have a reminder in the documentation about the necessity of including the Cargo binary directory in the system PATH for the successful execution of avm install latest.

Proposed Solution:

I suggest updating the documentation to include instructions for adding the entire Cargo binary directory to the system PATH. This will help users avoid similar issues and streamline their installation process.

Additional Concern:

avm install latest fails on my system, as does avm install 0.29.0 (both with the same error):

Screenshot From 2024-09-24 05-13-41

Thank you for considering this PR!

Add steps for mac or linux users to add the newly installed avm to their system path
Copy link

vercel bot commented Sep 24, 2024

@shawazi is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@shawazi
Copy link
Contributor Author

shawazi commented Sep 24, 2024

rust-lang/rust#127343

I wonder if the issue with time crate's compilation is due to the linked issue, or due to some personal rust installation issue.

Nevermind, ignore the time crate error, I found the relevant issues and solutions in this repo! #3131 (comment)

@acheroncrypto acheroncrypto added the documentation Improvements or additions to documentation label Sep 24, 2024
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

This looks great, however, there is one small issue: we mention bash, fish and zsh below the installation page, but this change only mentions bash and zsh. For consistency, could you also mention fish shell between the current shells?

During tests on a new VM with fish, avm was available immediately after installation of the dependencies and avm itself, without adding the cargo/bin to path or reloading the shell. However, in case someone else installed rust in a way that cargo/bin was not accessible by the system path, maybe it is important to leave in these path update sections. 

My apologies for the delay, and please let me know if there's anything else I should do.
@shawazi
Copy link
Contributor Author

shawazi commented Sep 25, 2024

Thank you for your feedback. I ran some tests in a fresh VM with fish, and noticed that avm is available in the same shell session without updating the fish path or reloading the shell. However, I looked up how to add cargo/bin to the system path for fish, and made some other minor syntax edits for consistency. Sorry for the wall of text, let me know if you need anything else.

Also, the time error was present there as well for attempting to use avm install latest.

Screenshot_2024-09-24_22-42-51

Should I make a separate PR and link to the solution in #3131 (comment) from the official documentation for getting started? I forgot to take a picture, but the same issue occurred with the time crate in the VM fish avm installation that required the 3131 solution.

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!

Should I make a separate PR and link to the solution in #3131 (comment) from the official documentation for getting started? I forgot to take a picture, but the same issue occurred with the time crate in the VM fish avm installation that required the 3131 solution.

Making changes to published crates is not really a great solution, especially when there are easier solutions such as running:

rustup default 1.79.0 && avm install <VERSION>

I don't think it's necessary to mention this in documentation because the problem is fixed already. We just need a new release (#3259), but it's currently on hold because of something external.

@acheroncrypto acheroncrypto merged commit 0c306c2 into coral-xyz:master Sep 25, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants