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

Make C API installation instructions usable #2677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rzumer
Copy link
Collaborator

@rzumer rzumer commented Feb 25, 2021

The Rust toolchain is not installed globally by default, so it is impossible to use
Cargo to install a library unless installing another toolchain for root, messing with
the root environment so the whole user toolchain can be located, or installing locally
and then manually moving the output. This fixes the installation instructions to use
the simplest of those solutions, saving most users frustration and manual reading time.

@rzumer rzumer changed the title Make C API build instructions usable Make C API installation instructions usable Feb 25, 2021
The Rust toolchain is not installed globally by default, so it is impossible to use
Cargo to install a library unless installing another toolchain for root, messing with
the root environment so the whole user toolchain can be located, or installing locally
and then manually moving the output. This fixes the installation instructions to use
the simplest of those solutions.
@coveralls
Copy link
Collaborator

coveralls commented Feb 25, 2021

Coverage Status

Coverage decreased (-0.5%) to 82.932% when pulling 20cc644 on rzumer-patch-3 into 98e1872 on master.

cargo cinstall --release
$ cargo install cargo-c
$ cargo cinstall --destdir=$HOME/librav1e --release
# mv $HOME/librav1e/* / && rmdir $HOME/librav1e
Copy link
Collaborator

Choose a reason for hiding this comment

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

sudo might be mentioned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using mv like this won't yield correct permissions unfortunately.

We could just suggest an install into ~/.local

Copy link
Collaborator

Choose a reason for hiding this comment

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

The canonical way to copy with correct permissions is /usr/bin/install, but it unfortunately can't copy recursively.

I had some incantation that made cinstall work as well but I can't find it now.

Copy link
Collaborator Author

@rzumer rzumer Feb 26, 2021

Choose a reason for hiding this comment

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

# implies root, on my end moving the files with sudo seemed to set the owner to root and permissions also seem to match other libraries. I didn't run exactly those lines though but I also didn't change permissions or owner manually.

Suggesting a local installation seems okay to me though.

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.

4 participants