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

Remove all install.sh code that installs python and leave it to the user #16000

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

emlowe
Copy link
Contributor

@emlowe emlowe commented Aug 10, 2023

One approach to keeping the installer working on newer Ubuntu versions and whatnot is to add in the code to handle this. (see #15920 or #15130)

However, a simpler and more maintainable approach is to make the user responsible for installing the proper version of python, sqlite, and openssl

This PR here removes all the code that installs python, sqlite, and openssl and just checks for the existence of versions and exits out if the versions aren't present.

This is easier to manage and allows the installer to work on a variety of platforms including new Ubuntu versions and other more esoteric versions of Linux

This may break some AWS deployment scripts as the code to install and compile python for amazon linux is removed

@emlowe emlowe added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Aug 10, 2023
Copy link
Contributor

@cmmarslender cmmarslender left a comment

Choose a reason for hiding this comment

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

Generally in favor of these changes. In addition to simplifying, I think it also gets rid of all the cases of needing sudo in the install script, which is a problem for some environments we try to install this.

There are a couple more things that can be cleaned up if we aren't going to be installing system level packages:

PACMAN_AUTOMATED
SKIP_PACKAGE_INSTALL

These vars at the top don't end up used anymore

Additionally, there is a whole section that checks and defines UBUNTU and DEBIAN vars that are no longer used as well with these changes.

@altendky altendky removed their request for review August 17, 2023 17:41
@wallentx
Copy link
Contributor

https://github.com/Chia-Network/chia-blockchain/blob/main/install-gui.sh#L106-L178
It'd be nice to remove all this sudo usage in install-gui.sh
Given the popularity of nvm, and due to the fact that we are installing EOL nodejs:12 on some distros, it'd be much better if we simply did a min version check, and exit 1 stating that the user needs nodejs > $MIN_VERSION installed.

@altendky
Copy link
Contributor

I know this is a bit self-centric, but I think it also is born out in the packages that get shipped. The system generally provides an easily accessible Python that will satisfy our requirements. The GUI requirements though are generally not provided in remotely recent enough versions. They are accessible more in the way that pyenv might provide Python where you have to go figure out what tool to acquire, how to use it, how to then create an isolated environment to work in and work in it and... That is functionality that this script retains for the Python side. So that's how I convinced myself that not installing Python was reasonable despite suggesting keeping the JS helpers stuff.

Maybe @ChiaMineJP can weigh in on how the GUI script works etc and how nvm compares to npm etc.

@github-actions
Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Oct 14, 2023
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jan 12, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the stale-pr Flagged as stale and in need of manual review label Jan 13, 2024
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Feb 5, 2024
Copy link
Contributor

github-actions bot commented Feb 5, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link

Pull Request Test Coverage Report for Build 7802799296

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 17 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.04%) to 90.881%

Files with Coverage Reduction New Missed Lines %
chia/harvester/harvester_api.py 1 73.02%
tests/simulation/test_simulation.py 1 96.52%
chia/server/node_discovery.py 3 79.9%
chia/wallet/wallet_node.py 3 88.16%
chia/full_node/full_node.py 9 84.81%
Totals Coverage Status
Change from base Build 7792246483: 0.04%
Covered Lines: 96594
Relevant Lines: 106251

💛 - Coveralls

@emlowe emlowe marked this pull request as ready for review February 6, 2024 21:04
@emlowe emlowe requested a review from a team as a code owner February 6, 2024 21:04
@emlowe emlowe requested review from cmmarslender and altendky and removed request for wallentx February 6, 2024 21:04
Copy link
Contributor

@cmmarslender cmmarslender left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just leaving "comment" for now so others have a chance to review and provide feedback without this getting merged, since I would count for required reviewers

@cmmarslender cmmarslender dismissed their stale review February 7, 2024 15:58

Ok with changes now, and just letting others take a look before approval

.github/workflows/test-install-scripts.yml Show resolved Hide resolved
install.sh Show resolved Hide resolved
install.sh Show resolved Hide resolved
@Starttoaster Starttoaster merged commit e50e1e6 into main Feb 14, 2024
265 checks passed
@Starttoaster Starttoaster deleted the EL.simplify-install branch February 14, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants