Skip to content

Conversation

emlowe
Copy link
Contributor

@emlowe emlowe commented Sep 17, 2025

Similar to what was done with python some time ago - eliminate the code in the install-gui script that tries to find and install node. This simplifies the script considerably, and it places the burden on the user to make sure they have the proper version for their OS

Since our CI always make sure the runners have the proper version of node installed this change has no affect on CI. Very few users try to install the run the GUI from source so this has limited impact to actual users - and those users can easily get NodeJS from official channels

The trigger for this change was problems the script was having using brew to install node on macOS 13. Rather than change it to use alternative means, this change is simpler and removes a bunch of scripting and follows along with the precedent we made earlier for python

@emlowe emlowe added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Sep 17, 2025
@emlowe emlowe marked this pull request as ready for review September 17, 2025 19:27
@emlowe emlowe requested a review from a team as a code owner September 17, 2025 19:27
Copy link

coveralls-official bot commented Sep 17, 2025

Pull Request Test Coverage Report for Build 17840058243

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 50 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.03%) to 91.129%

Files with Coverage Reduction New Missed Lines %
chia/_tests/simulation/test_simulation.py 1 96.47%
chia/wallet/wallet_node.py 1 87.34%
chia/consensus/blockchain.py 2 94.38%
chia/timelord/timelord_launcher.py 2 90.71%
chia/util/files.py 2 91.38%
chia/rpc/rpc_server.py 3 88.09%
chia/plotters/bladebit.py 5 63.27%
chia/server/address_manager.py 5 92.84%
chia/server/node_discovery.py 8 80.5%
chia/full_node/full_node.py 10 86.72%
Totals Coverage Status
Change from base Build 17773649562: -0.03%
Covered Lines: 102707
Relevant Lines: 112539

💛 - Coveralls

altendky
altendky previously approved these changes Sep 17, 2025
Copy link
Contributor

@ChiaMineJP ChiaMineJP left a comment

Choose a reason for hiding this comment

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

The new install script works when node and npm are installed.
However, it doesn't work as expected on the environment where neither node nor npm cannot be found.

image

@emlowe
Copy link
Contributor Author

emlowe commented Sep 18, 2025

Thanks for testing that @ChiaMineJP - I missed that case

@emlowe
Copy link
Contributor Author

emlowe commented Sep 18, 2025

@ChiaMineJP I believe I have fixed up the script to handle this case now

ChiaMineJP
ChiaMineJP previously approved these changes Sep 18, 2025
Copy link
Contributor

@ChiaMineJP ChiaMineJP left a comment

Choose a reason for hiding this comment

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

Now it looks good to me 👍

@pmaslana pmaslana merged commit 3652e60 into main Sep 18, 2025
384 of 385 checks passed
@pmaslana pmaslana deleted the EL.gui-install-simplification branch September 18, 2025 22:09
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.

6 participants