-
Notifications
You must be signed in to change notification settings - Fork 7
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
Convert installer to a sequential process with progress tracking #85
Conversation
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.
I've reviewed the code from 30,000 ft. Now I need to test it.
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.
When the steps worked, it was great to speed through several. I think we need a way of remembering what options a user chooses at the beginning so that the "Choose Your Setup" can be skipped/remembered in case an install is cancelled or errors.
I was testing on the TUI.
Some aspects seem to hang with the current changes and that needs to be investigated.
alt_installer.py
Outdated
utils.net_get( | ||
config.LOGOS64_URL, | ||
target=downloaded_file, | ||
app=app, | ||
evt='<<GetLogos>>' | ||
) |
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.
Ran a test install and got to this step. CLI with DEBUG reports that a download was started but no download percentage is returned. I could've swore we had this before. Is this a regression?
2024-03-29 21:35:24 DEBUG: Progress: 70%
[****************************------------]
2024-03-29 21:35:24 INFO: 'Logos_v29.1.0.0022-x64.msi' exists in /home/thwright/Downloads.
2024-03-29 21:35:24 DEBUG: Comparing size of https://downloads.logoscdn.com/LBS10/Installer/29.1.0.0022/Logos-x64.msi and /home/thwright/Downloads/Logos_v29.1.0.0022-x64.msi.
2024-03-29 21:35:24 DEBUG: Getting headers from https://downloads.logoscdn.com/LBS10/Installer/29.1.0.0022/Logos-x64.msi.
2024-03-29 21:35:24 DEBUG: Starting new HTTPS connection (1): downloads.logoscdn.com:443
2024-03-29 21:35:24 DEBUG: https://downloads.logoscdn.com:443 "HEAD /LBS10/Installer/29.1.0.0022/Logos-x64.msi HTTP/1.1" 200 0
2024-03-29 21:35:24 DEBUG: content_length = '363597824'
2024-03-29 21:35:24 DEBUG: content_md5 = 'wC4ZwUmm8OxhTI+pT/MddQ=='
2024-03-29 21:35:24 DEBUG: url_size = 363597824 B; file_size = 363597824 B
2024-03-29 21:35:24 DEBUG: Comparing MD5 of https://downloads.logoscdn.com/LBS10/Installer/29.1.0.0022/Logos-x64.msi and /home/thwright/Downloads/Logos_v29.1.0.0022-x64.msi.
2024-03-29 21:35:24 DEBUG: Getting headers from https://downloads.logoscdn.com/LBS10/Installer/29.1.0.0022/Logos-x64.msi.
2024-03-29 21:35:24 DEBUG: Starting new HTTPS connection (1): downloads.logoscdn.com:443
Be that as it may, it now appears to hang. I let it set for a while to make sure it was actually downloading, and after 10 minutes it still had not completed on a 300mb down connection, so it seems like there is some issue in the net_get code that has changed and is now affecting the download.
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.
This looks like it's not even the actual download, but rather just getting the headers to determine the file size and MD5 sum. That should be a relatively quick process. Is this still happening for you? I'm not seeing the problem my testing.
alt_installer.py
Outdated
import wine | ||
|
||
|
||
def ensure_product_choice(app=None): |
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.
I ran an install but had to hit Ctrl+C to escape a download (as detailed below) that appeared to have failed.
On starting the program again, I was hoping that I would be able to choose Install and start again at the downloading the MSI step. Instead, I was presented a menu for choosing the FaithLife product.
I'm guessing the Ctrl+C means that the install steps were logged/stored? Perhaps these need to be stored sequentially rather than all at once?
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.
I haven't yet tried where I have to re-download anything other than the release list XML file. I'll try to truncate either the installer or the appimage and only re-download the last few MB as a first test of the download function. It should resume and add the missing bytes.
alt_installer.py
Outdated
|
||
if not utils.find_installed_product(): | ||
wine.install_msi() | ||
config.LOGOS_EXE = utils.find_installed_product() |
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.
This step hung for 7 minutes before I cancelled it.
I restarted the install for a third time through—definitely nice to skip over steps like fonts—and the installer again hung at this function for 4 mins before cancelling again.
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.
I can't recreate this. I've run multiple installations by first removing the INSTALLDIR and the config file, as well as running the installer with the app already installed. Are we using different parameters? I'm using Logos 10 v29.1.0.0022, recommended appimage, system winetricks, don't install fonts, passive install of app, so ./LogosLinuxInstaller.py -PDF
.
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.
I did not use -P, but the same otherwise. And I let fonts install.
Also, until we implement a TUI status bar, it would probably be good to change the install progress reporting to Info level rather than Debug, or possibly output a CLI msg alongside an Info logging level. |
0bf6595
to
811e0d7
Compare
This what I see if I don't use
This is the same info presented to the GUI app: same messages, same progress bar. Here's the
Would it be helpful to move some of the |
Non-verbose/non-debug as in cli msg? |
That's what I meant to say, yes. But now I realize that maybe I misunderstood your earlier comment. Are you thinking it would be helpful to have more terminal output with the default verbosity (no |
More, and specifically, some kind of progress bar, so what you shared would be perfect. One of my thoughts for TUI is #64, just so the user has some kind of feedback. I've debated whether I need a fuller TUI to support this, but CLI msgs would be easiest to implement. |
The I also added some significant changes to the I've successfully tested the TUI and GUI with and without a preexisting config file. |
I've added in Logos9 compatibility and done some more cleanup. This is again ready for review. |
The last commit works in the recommendation from #92 |
I have just run an install from start to finish that was successful. I have just tested the resume capabilities and this appears good to me. I will give the code a once over and barring anything jarring I will merge once done. My only outstanding concerns are the strange networking issues I bumped into, but I had never seen that before. |
I would love for these commits to be squashed as this is a lot of commits and several are for typos/formatting. For instance, it would be great to have a single pep8 commit rather than several. Do you see this as being a possibility or would there be a dreadfully tedious amount of cleanup? If so, could you merge the simple ones at least? |
I'll take a look at squashing and get back with you |
add alt_installer module add alt_installer module make alt_installer compatible with tui_app further refine alt_installer.py & implicated funcs fix pep8 warnings general cleanup ensure buttons are disabled during installation fix issues after conflict resolution pep8 cleanup rename files comment out unused code general cleanup move set_winetricks to conrol.py reinstate APPDIR_BINDIR as temp config variable only add necessary env vars from config organize config var defaults into dicts organize config var defaults into dicts update var names use list of funcs that require app installation use list of funcs that require app installation pep8 fixes
remove old installer module use explicit decode to avoid winetricks terminal output errors
add logos 9 compat; better use of logos_reuse_download correctly handle Logos9 installation formatting fix minor layout fixes rename queue rename queue rename queue
start winetricks in thread for better UX fix typo
fix winetricks install in GUI Control Panel pep8 fixes pep8 fixes pep8 fixes pep8 fixes
The commits have been squash-glomerated into a smaller number. I decided to avoid trying to reorder them, though. |
This is a complete rewrite of the
installer.py
module, initially included as a new modulealt_installer.py
. Thealt-installer
module should ultimately replace theinstaller
module before this PR is merged. The main goal is it allow for resuming an installation, or theoretically fixing a broken one, without having to start over by deleting theINSTALLDIR
.Here are the main changes:
f"{config.INSTALLDIR}/data/bin"
instead ofconfig.APPDIR_BINDIR
).