theypsilon / Update_All_MiSTer

All-in-one script for updating your MiSTer
GNU General Public License v3.0
629 stars 27 forks source link

Incorrect syntax on line 99 #22

Closed wiggy808 closed 4 years ago

wiggy808 commented 4 years ago

Line 99 of the update_all.sh has a syntax error that occurs for some folks when running this for the first time due to the condition check on the "if" which does not have the double brackets.

Current: if ! ${SCRIPT_PATH} ; then

Should be: if [[ ! ${SCRIPT_PATH} ]] ; then

theypsilon commented 4 years ago

Hi @wiggy808 , thanks for sharing your feedback.

I would like to understand in which cases that error is happening since I couldn't reproduce it so far.

wiggy808 commented 4 years ago

I received the error when I did the update_all today for the first time, I uploaded just the update_all.sh and ran it from the OSD menu. It was the first time I ran the update_all.sh, and had just ran the update.sh the day before from a fresh build.  I also created an update_all.ini that I had altered for my liking (attached as txt since i couldnt upload .ini update_all.txt ).

The error I received was "syntax error unexpected then...". The "then" is after the "if" condition in the update_all.sh, which doesn't have the double brackets to test the file existence (-F) so it errors.  I think an easy fix is to add the double brackets around the if condition for checking the file's existence.

theypsilon commented 4 years ago

Ok, so to make it clear. It is the very first day you use update_all, meaning you never used it before in your MiSTer, right?

Is your MiSTer a fresh installed also? Or when did formatted your MiSTer SD card the last time?

I just did a fresh install on my MiSTer with the INI file you provide and update_all.sh from here: https://raw.githubusercontent.com/theypsilon/Update_All_MiSTer/master/update_all.sh

And I couldn't reproduce the error. So my wild guess is that it might be caused by you having an old Linus version. If that's the case, I would really love to know for sure, so I can consider that when coding.

The syntax error message I think is misleading. That if ! ${SCRIPT_PATH} ; then statement should be fine if ${SCRIPT_PATH} renders a command that returns 0 or 1, which should be the case here. Note that the single bracket IF syntax is a shortcut to the test command in bash after all, and single bracket IFs are all over the place.

I'm very intrigued by your error. If you don't mind, I would love to investigate it further. I hope you can provide me a little more context, so I can track down the root cause. Thank you!

theypsilon commented 4 years ago

@wiggy808 now that I think it further, your error could also mean that your connection had some issue during the first curl download. That could be tested by trying a second and a third time without performing any modification.

wiggy808 commented 4 years ago

@theypsilon

Thanks for the response, I think you hit on something I didn't consider. But I'll respond as detailed as i can, be prepared, this may be lengthy, haha.

" It is the very first day you use update_all, meaning you never used it before in your MiSTer, right?" Correct

"Is your MiSTer a fresh installed also? Or when did formatted your MiSTer SD card the last time?" This part was something that kinda got me thinking today about your second response and maybe a key to the issue:

"@wiggy808 now that I think it further, your error could also mean that your connection had some issue during the first curl download. That could be tested by trying a second and a third time without performing any modification."

I'm not convinced a curl issue occurred, (only cause I was watching it, and it seemed to be downloading just fine), but definitely possible. But what got me thinking is the "fresh installed" and "formatted" comments.

My MiSTer SD card came pre-formatted from UltimateMister when I ordered it. My "fresh" install was a misnomer, I basically wiped the file system, not the underlying OS on the SD card in May. I was having issues manually installing rbf / mra / rom etc. when I came across an article about the scripts... So having made a file backup (not image backup) of the original UltimateMister SD card, I removed everything under /media/fat and recopied the file backup of the fat directory back to /media/fat. It was then that I ran the update.sh script, took a long time but seemed to go without a hitch, hard to tell honestly if there were errors since it scrolled so fast.

Afterwards, I was noticing some disconnection in the updates for cores between jotego and the main ones. Doing more research, and talking to Discord folks on the channel, they indicated I needed to run the update_all.sh script. I read up on it here on github, downloaded it, created an update_all.ini (basically changed a few true / false items, no biggie), uploaded both to the SD card, then ran the update_all.sh on the mister and saw the error on the bottom, after the echo "Update All 1.2 finished. Your MiSTer..." message was displayed.

I did run the update_all again just now, with no modifications since we talked last night, and no error this time. I'm assuming the update_all.log is not appended but newly written each time as it had the current date, and only today's results. Side suggestion but maybe create each update_all.sh result log with updateall$(date +"%m%d%Y").log so you get one for each time its run, wishful thinking =)

Let me know what else I can do to help you recreate the issue, but I most likely had an older Linus version. Currently, I'm on 4.19.0-socfpga-r1 but I'm not sure what I may have been on before the update or update_all scripts were run. I'm not able to find a boot dir or a grub config or rpm, etc, lol so not able to see what other kernels might be installed or were installed previously.

Thanks again for your help, greatly appreciated!

theypsilon commented 4 years ago

Now that you said that the latest run gave you no error, makes me think more that you ran into a connectivity issue.

Do you use MiSTer with wifi? Sometimes the problem is the wifi dongle catching a poor signal. I would doublecheck with ethernet connetion if you are running into frequent issues to make sure that the problem is not caused by the wifi connection.

If your Linux installation was from this year, there should be no issues related to that most probably. In any case, you could try formatting and installing from scratch, but I don't think that's needed at the moment.

About storing old logs. The problem is that they are actually heavy files, they could eat a lot of space of those users that update frequently. But maybe storing latest 10 logs is good idea. I will think about it.

wiggy808 commented 4 years ago

No wifi use here, the transfer speeds are never as good or reliable as ethernet like yous said, so I've only used ethernet on my MiSTer.

The image was from May (when I bought it), but I don't know if the guys at UltimateMister use an older image when they ship the sd cards out or not.

I attached something that I tested on my MiSTer that worked to keep the last 10 logs. The top is for creating my 13 test log files of receding dates, the bottom is the script that will remove all but the latest 10 files. If ya like it, can include in image. I do alot of bash scripting on the side so I can always help with more if ya like, love helping the gaming community, just hit me up anytime. clean_update_all_log.txt

theypsilon commented 4 years ago

Hey @wiggy808 did you ever have this problem again?

In case you didn't, I think we should close this issue, and maybe open another one about the 10 logs feature.

wiggy808 commented 4 years ago

I agree, the error is a one off. My buddy who did the upgrade for the first time this week also received the error, then never since. So it never comes back once you've done the upgrade. Thanks for the followup, and let me know if you need any assistance, always available to help where / when I can.

On Tue, Jul 21, 2020 at 12:29 AM José Manuel Barroso Galindo < notifications@github.com> wrote:

Hey @wiggy808 https://github.com/wiggy808 did you ever have this problem again?

In case you didn't, I think we should close this issue, and maybe open another one about the 10 logs feature.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/theypsilon/Update_All_MiSTer/issues/22#issuecomment-661775024, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQFDTN5ZPHQT6YZVPAVRBQDR4VUZLANCNFSM4OXUELPA .

theypsilon commented 4 years ago

I'm closing this as the issue doesn't happen more, and the mentioned code is gone.

The logs feature can be discussed here: https://github.com/theypsilon/Update_All_MiSTer/issues/24