-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Created Install Script for Debian #3041
base: testnet3
Are you sure you want to change the base?
Conversation
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
Hi, thx for taking the time to make this. However, this is based on an older version of the ubuntu script. It's better form to only require root for the actions that need it (installing packages, firewall rules), and not for the build. This was updated a few days ago for the ubuntu script. Note that the As a general comment, I feel this script tries to do too much. I understand you need to fetch the LLVM packages as debian repos are always hopelessly outdated, but does this script really need to support all kinds of ubuntu and debian variants? So please rebase your branch to bring it in line with the current script. I'll also add some comments on specific lines. |
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
source: https://apt.llvm.org/llvm.sh Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
I found a better solution for the best way to integrate Debian. I think it is a mistake to create one script for Debian like we have for Ubuntu. We should instead have a two step install process Step 1) run the official script provided by the LLVM organisation to install updated version of Clang and LLVM for Debian Step 2) run the SnarkOS Install script for Debian The SnarkOS install script for Debian will be almost identical to the updated install script for Ubuntu, This is in my opinion the best solution for three reasons:
I have made the following changes
|
Yes this seems like a better approach. I'm not sure we should bundle |
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
Yes, including a link to the llvm script in the readme is a better approach than bundling it. I removed the llvm script file. |
added sudo package to installation script Signed-off-by: fabrinasr <[email protected]>
added sudo package to installation script Signed-off-by: fabrinasr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
The decision to merge is not mine. @howardwu will merge when appropriate.
This is nice to have but not necessary for mainnet launch, we are focussing
our efforts on mandatory changes ATM.
…On Fri, 23 Feb 2024 at 15:34, fabrinasr ***@***.***> wrote:
I can confirm I just tested on Debian and it is running fine but I do not
have write access to this repo so it will not allow me to merge PR. Can you
merge PR ? @joske <https://github.com/joske>
—
Reply to this email directly, view it on GitHub
<https://github.com/AleoHQ/snarkOS/pull/3041#issuecomment-1961443567>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEB7RZPAR4IOEDPD34UOJDYVCSIXAVCNFSM6AAAAABCOQOKISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRRGQ2DGNJWG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I understand. I was thinking about it from the point of view of the people running Aleo Provers. I think Debian would be a better option for them. In my experience it is more stable than Ubuntu. This PR also made some minor improvements to the Ubuntu script. But I understand there might be bigger priorities right now for @howardwu |
Signed-off-by: fabrinasr <[email protected]>
Signed-off-by: fabrinasr <[email protected]>
I think this is good to go. It does not block mainnet - see label. It is not urgent but still very useful for new Aleo users. We've had a few users asking for debian |
@fabrinasr can you re-target to the |
cargo install --locked --path . | ||
``` | ||
|
||
Please ensure ports `4133/tcp` and `3033/tcp` are open on your router and OS firewall. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get rid of port recommendations. Also these are not even the defaults? 🤔
Motivation
I looked at the Ubuntu script created by Howard and I made one main modification to make it work for Debian. In the Debian repos the LLVM and Clang packages are old. To solve this issue I grabbed the LLVM and Clang packages directly from the LLVM repo. For the reviewer checking this... the original source for the LLVM part of the script is below.
Source: https://apt.llvm.org/llvm.sh
I made some minor modifications to the above script and Howard's Ubuntu Script to create a working Install Script for Debian.
I also added Debian Install instructions to the Docs and I added a Beta tag next to the Debian instructions in the Docs for clarity.
Test Plan
I have tested the script multiple times in Debian with no errors. I purged all installed packages and re-ran the script multiple times and got almost exactly the same output that you get when you run the Ubuntu Script in an Ubuntu OS.
Related PRs
(Link any related PRs here)