Skip to content

Simpler install#100

Merged
sand7000 merged 2 commits intoGaloisInc:masterfrom
sand7000:simpler_install
Apr 10, 2025
Merged

Simpler install#100
sand7000 merged 2 commits intoGaloisInc:masterfrom
sand7000:simpler_install

Conversation

@sand7000
Copy link
Copy Markdown
Contributor

@sand7000 sand7000 commented Apr 9, 2025

This addresses:

#48

It adds a script to simplify installation a bit and it adds a RUSTUP_TOOLCHAIN env var to wrapper.rs to make sure the installed tools always use the right toolchain.

@sand7000 sand7000 requested a review from RyanGlScott April 9, 2025 17:18
Copy link
Copy Markdown
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I am quite happy with the RUSTUP_TOOLCHAIN changes in wrapper.rs. I have more mixed feelings about the introduction of a wrapper script, since it's yet another way way to install mir-json that we would need to maintain going forward (e.g., remembering to update the toolchain version). That being said, it does replace multiple cargo/rustup commands with a single script, so I can see the appeal behind it. I won't push back if others are on board.

Comment on lines +10 to +13
echo "Please install rustup by running:"
echo " curl https://sh.rustup.rs -sSf | sh"
echo "and update your path:"
echo " source $HOME/.cargo/env"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of putting the exact shell commands to run here, I'd recommend pointing folks to the instructions at https://rustup.rs instead, as it actually recommends running a different command than what is listed here. What's more, this would future-proof the script in the event that the https://rustup.rs instructions change later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I took the command from the README which has a link to an old version of the rust book. I will fix both the script and the readme.

@sand7000
Copy link
Copy Markdown
Contributor Author

sand7000 commented Apr 9, 2025

I have more mixed feelings about the introduction of a wrapper script, since it's yet another way way to install mir-json that we would need to maintain going forward

This is fair although I find maintaining a README to be more error prone and more likely to fall out of date than a testable script. Case in point the Rust book reference in our README was already out of date.

@RyanGlScott
Copy link
Copy Markdown
Contributor

OK. I generally prefer to know exactly what cargo commands are being run under the hood, and if we ever want to upload mir-json to crates.io someday, then I suspect that we will have to make the installation script as close to a series of one or two cargo commands as possible. That being said, the installation script is small enough that it is easy to audit by hand, so I think I am fine with it.

@spernsteiner @qsctr, do you have any thoughts on this?

@qsctr
Copy link
Copy Markdown
Collaborator

qsctr commented Apr 10, 2025

I don't have strong feelings about this. I think the toolchain and component installations can actually be combined into one command, like we document in wrapper.rs:

" rustup toolchain install {} --force --component rustc-dev,rust-src",

(speaking of which, that line probably should be updated to just ./install-mir-json.sh, if we decide we are using the script.)

At that point, the shell script really is just turning two commands into one command, so I'm not sure that it's necessary. Also, I think there is some value in having the command for setting up the toolchain and the command for building & installing mir-json itself be separate, in case one of them goes wrong. But that mainly comes from past experiences trying to install software with complex installation scripts that fail somewhere in the middle, and not being able to figure out what went wrong, but as you say, this script is simple enough to inspect, so I guess it's fine.

@sand7000 sand7000 merged commit f7ad1a4 into GaloisInc:master Apr 10, 2025
5 checks passed
@sand7000 sand7000 deleted the simpler_install branch April 10, 2025 15:37
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.

Automatically switch to pinned Rust toolchain before mir-json installation

3 participants