radicle-registry icon indicating copy to clipboard operation
radicle-registry copied to clipboard

Scripts for "cargo install"

Open CodeSandwich opened this issue 4 years ago • 7 comments

We have scripts/build-release, which generates the node and cli binaries.

It would be convenient to have a script to install the binaries from source with cargo install. It would ease the process of installating from source and remove the $PATH step.

My proposition: scripts/install_node:

DEFAULT_CHAIN=ffnet cargo install --path node 

scripts/install_cli:

cargo install --path cli

Also we should turn https://github.com/radicle-dev/radicle-registry#build-from-source to "install from source" using these new scripts and update the link in http://registry.radicle.xyz/docs/getting-started/#installation.

CodeSandwich avatar Apr 23 '20 16:04 CodeSandwich

Good stuff! Some thoughts :

  • I always read "build from source", so "install from source" is not so much of common language. Maybe that's fine though

  • I would prefer to separate the build from the installation. Say you have a local version of the binaries and we want to build a different one, and run both to compare something. With this approach, building implies installing, so doing that would require picking commands or ad hoc some to get there. If we separate the steps we wouldn't need that.

Maybe we pass a flag to the script to signal whether we want to install it or not?

NunoAlexandre avatar Apr 24 '20 06:04 NunoAlexandre

I like the overall idea but agree with @NunoAlexandre’s point. We should keep the language and make the installation optional. We should also considering making ffnet the default DEFAULT_CHAIN and specifying devnet as the chain in ./scripts/build-dev-node.

geigerzaehler avatar Apr 24 '20 08:04 geigerzaehler

There seems to be a misunderstanding, I want to add the installation scripts, not replace any existing ones, build-release would be kept untouched.

I always read "build from source", so "install from source" is not so much of common language.

I agree, install from source does look cumbersome, lets not rename it.

We should also considering making ffnet the default DEFAULT_CHAIN

That would force us, the developers, to pass DEFAULT_CHAIN=dev on every test compilation. I think that the current state is much more ergonomic for us. Building the binaries for the public is rare and scripted anyway.

CodeSandwich avatar Apr 24 '20 11:04 CodeSandwich

That would force us, the developers, to pass DEFAULT_CHAIN=dev on every test compilation. I think that the current state is much more ergonomic for us. Building the binaries for the public is rare and scripted anyway.

This depends on how we’re building the node for development. I personally use ./scripts/build-dev-node and ./scripts/run-dev-node as documented. Unless somebody uses cargo build --release alone this we could just adjust these scripts.

Another thing that came to mind: cargo install only works if ~/.cargo/bin is on PATH. So even with that script we need some additional explanation. Considering that I’d prefer to not have the script to keep them number of scripts we have to maintain and the users can possibly run small.

geigerzaehler avatar Apr 24 '20 13:04 geigerzaehler

Unless somebody uses cargo build --release alone this we could just adjust these scripts.

Sometimes I do use cargo directly, sometimes I'm using a custom script. I think that overall it's just another thing to remember and creates a trap for the newcomers. And what are we gaining? For every release build there are hundreds of development builds, so we should optimize the compilation UX for them.

cargo install only works if ~/.cargo/bin is on PATH

I think that this is set up automatically during the installation of cargo.

CodeSandwich avatar Apr 24 '20 14:04 CodeSandwich

cargo install only works if ~/.cargo/bin is on PATH

I think that this is set up automatically during the installation of cargo.

Could you verify this. If so, I’m down for this change.

geigerzaehler avatar Apr 27 '20 08:04 geigerzaehler

I've verified it and it's true: https://github.com/rust-lang/rustup#installation

CodeSandwich avatar Apr 27 '20 08:04 CodeSandwich