[Windows] Change CONFIG.dat file location, behavior #335
Conversation
Signatures of some functions were changed to be more restrictive and expressive. Partly style, partly to let the compiler catch errors.
CONFIG.dat file will appear in the user's AppData directory now. Also refactor a bit: define function for getting CONFIG.dat path, joining filesystem paths (it's not very smart though, but better than pulling in the .NET framework to get the right function), and use new signature for formatResultBuffer.
MCGallaspy
changed the title
Then I must be a proto-idea of an egg... 😛 |
Replace wchar string literals with by wrapping them in the TEXT(...) macro, which uses char or wchar depending on whether the UNICODE preprocessor variable is defined. Makes code portable! Refactor writeConfigurationFileBuffer -- there seems to have been unreachable code. Refactor searchKeyIndex to be more concise.
Also refactor to remove global variable prevVerStr.
|
@radinamatic this should almost be ready for testing. I just want to give it a sanity check before unleashing you on it in full force. |
This makes me sound like a bloodhound. You guys must still see me as an enforcer of sorts 😜 |
Yes. Let me add an item for that. That said, it shouldn't affect this too much. Can you download the installer, and ensure that it just installs okay, then I'll push that change and we can start testing upgrading/start at boot behavior? |
|
Once I push the change, I'd like to test it out like the following scenarios:
Maybe others? |
|
Sounds fine by me, to begin with.. If something pops up that needs a more specific scenario, we'll get to it after this... If you didn't push this yet, should I still test the # 101 build? |
|
@radinamatic should be good to go with build 102. |
|
Ok, will download now and start testing tomorrow! 👍 |
|
Hm, the version check might not be working correctly: it seems to detect the previous installation on VM snapshots (of 2 different Windows 7 machines) that do not have any. I've searched the registry for any
|
|
And no wonder it reports an error afterwards:
|
|
Regarding the In case of legit upgrade scenario (and not with detection of non-existing previous installation), at this point the installer should include the option for the users to choose the old-version content folder themselves. Could we try to implement that? |
|
On another hand, # 102 build behaves the same on Windows XP. This time though, I chose not to "upgrade", and the installer could not proceed, since there was no old database to delete... 😛
|
Yeah, this is a legitimate point. Please open an issue for it, I'd like to address it separately. |
|
I think both the issues you reported stem from the fact that an "upgrade" was attempted on a fresh install. Submitting a patch shortly. |
|
#341 issue opened. |
|
Retested the 0.14.2 on completely new Windows XP VM. Start on boot option selected in the installer does not work with scheduled task run by the
|
|
@radinamatic okay, don't spend too much time on 0.14.2 on XP, I'll see if I can replicate the failure myself. I was able to get 0.14.2 working fine re: start at boot on W7 using a different image. Not sure what my original issue was, but I'll move on for now. |
|
Some updates: I tried 0.14.2 -> 0.16.0 on W7. I didn't get any errors during installation as you reported, but that still concerns me! Can you try to identify how to replicate that behavior? That said, I did notice two things:
Let me try out upgrading from other version -> 0.16.0, then we'll take stock again. |
After several tryouts the only thing that I can conclude is that it happens only if the server is already running in the background, which is to be expected if the previous 0.14.2 installation is set to start on boot, and user did not expressly stop the server and python processes prior to upgrading to 0.16. When I did that test case (manually stopping the 0.14 server), or when I had 0.14 version which was not set to start on boot, no errors appeared during the upgrade. |
|
And if you have the "Automatically restart the system after errors" checkbox (in Advanced system recovery options) deactivated, you will not even see the error unless you watch carefully the cmd window while it executes the |
No worries, I figured a lot of 0.16 core features are still in the re-making. Videos are safely in the |
|
I did indeed try upgrading while 0.14.2 KA Lite was still running, but my system didn't crash. Also the red text that appears in the cmd prompt during the Sorry, but I'm not able to replicate the crash -- can you perhaps try to get a VM in a state just prior to the crash, ensure that it happens consistently, and then we'll see about uploading the image somewhere so I can further debug it?
I'm not familiar with this option and I couldn't seem to find docs for it (for W7). Can you describe how to enable/disable it? |
|
Did you also try the upgrade from 0.15 to 0.16? I had the same "logoff" error there too? Let me fetch a screenshot of that "Automatically restart the system after errors" option. |
|
XP, but it's in the same place on 7 & 8. |
|
Ok, after several more tryouts, I wasn't able to reproduce it consistently... 😒 The only thing that I can see remaining to try is to download a completely new VM image and make one last upgrade test from 0.14 to 0.16. Will report back after. |
|
@MCGallaspy I'd forget about that error while upgrading on W7 for now, as I couldn't replicate it on a completely new W7 image, a new 0.14.2 install and upgrade to 0.16. Error is there, and even though it doesn't force logoff/restart of the VM, this time I managed to see a bit more of that error terminal output in red and even catch a glimpse of a phrase, something like "folder not available" or "folder not present" at the end... You said that it was an "unfortunately worded message", do you have the exact wording of that message? |
Yeah, if you check in the Program Files directory where KA Lite is installed, you should find a zipfile called something like |
|
Ok, this is what I get with what you say above, and it looks pretty tame compared to what I see in that error during install: The two lines |
|
Try running |
|
Nope, all seems fine here: I tried the above with the server running in the background, and logged in as both admin Windows user and standard one with no admin rights, since it was there that the error occasionally appeared, but no dice... Maybe we should set this aside unless it starts raising its nasty head in a more consistent manner. |
|
Okay, at this time I've managed to upgrade from 0.13 and 0.14 to 0.16 with apparent success inasmuch as this PR is concerned, meaning that:
@radinamatic you still have quite a few unaddressed bugs, which I'll try to summarize shortly below. Additionally, I've also encountered at least one apparent application error from upgrading 0.13 -> 0.16, which I'll file elsewhere. That said, I think if we set the bar for this PR to addressing the above points, then we can call it a success and consider the other bugs separately. |
|
Additionally, it seems we must ensure the start-at-boot scheduled task is set to run whether the user is logged in or not (at least on Windows 7). |
|
Ok, will re-testing if start-at-boot scheduled task executes correctly whether the user is logged in or not on Win 7/8. I think that so far it did work on 7, but I can't recall for Win 8. |
|
We can tackle start at boot separately -- I just wanted to note it down. I would like specifically to test whether user data + content is migrated from the prior location to the user's .kalite dir, and that the task-tray config options work without error in 0.16. But looking more closely at the thread, it seems you already confirmed this for 0.12 and 0.15, yes? And I independently verified it for 0.13 and 0.14. If my understanding is correct, I think we should consider the other issues separately, as this thread is getting unwieldy. |
|
Yes, user data + content is seemingly migrated from the prior location to the user's .kalite dir, and I haven't seen any task-tray config errors with installer # 104 in any upgrade scenarios. I say "seemingly" because videos and langpacks are there in the migrated .kalite dir, but are not functional with this installer. As soon this is solved we can definitely confirm that migration is, indeed, successful. And yes, we should start considering other issues separately, this discussion is getting too long to manage... 😉 |
It won't be resolved in this PR, it's an application issue.
Agreed, I'll open separate issues and close this one once I get someone to review it. |
|
@rtibbles consider this a last effort to get a reviewer -- if you're not able or willing in the next few days, could you kindly just press the "merge" button for me? |
|
These should cover known errors @radinamatic, any that I missed here? |
[Windows] Change CONFIG.dat file location, behavior
|
Note to self: update build server installers branch to build from. |
Still a WIP. Will fix #321.
Todo:
config.h.Will appreciate a code review. I am but an egg with C++\Windows dev. Willing to discuss in great detail.