Cleanup and refactor firmware.sh

[sharpenedblade]

There are minor visual changes and some of the "warnings" about installing packages got removed but most of the logic is exactly the same. 9c427e2 has a gnarly diff but the code changes are actually really small.

[AdityaGarg8]

Previously it was possible to manually run the python script using python3 firmware.sh. It can be useful in various cases, like having some firmware archive and want to rename it. Iirc, that's why I chose a polyglot script. Will it still be possible?

[AdityaGarg8]

Also you have removed many intended blank lines from the code. It becomes a pain to read what is written on the terminal without blank lines. Lots of blank lines missing in create_deb, create_rpm and create_arch_package.

[AdityaGarg8]

Also, the commit history is a pain to review here. Probably commit like

Refactor Install packages. Refactor loading and unloading of drivers.

Etc.

Squash the commits which are addressing minor mistakes made by you.

Lastly, do the miscellaneous changes like shell check fixes, refactoring the python bit etc.

[AdityaGarg8]

If you want, you can open a new PR with a proper commit history. Probably shall also fix the intended blank lines, since hunting them shall be difficult.

[sharpenedblade]

Previously it was possible to manually run the python script using python3 firmware.sh. It can be useful in various cases, like having some firmware archive and want to rename it. Iirc, that's why I chose a polyglot script. Will it still be possible?

I am going to make the script accept flags to pick a method so I can just add a flag to run the rename function

Also you have removed many intended blank lines from the code. It becomes a pain to read what is written on the terminal without blank lines. Lots of blank lines missing in create_deb, create_rpm and create_arch_package.

All of those were changed in d0c05f8, I can just revert that commit.

Also, the commit history is a pain to review here. Probably commit like

I can squash down some of the commits but its not going to be that much better. Github is messing up the diff, its a lot cleaner if you run git diff master --diff-algorithm minimal.

[AdityaGarg8]

Last 2 commits are absolutely unnecessary, The only thing that is good is replacing echo with cat and renaming firmware.tar to firmware-raw.tar. And please don't remove logging. It's required for have an idea where the script fails, in case it does. Also, why python_check is there in create_firmware even if it is macOS only check?

[AdityaGarg8]

                for file in "$mountpoint/brcmfmac4364b2-pcie.txt" \
                        "$mountpoint/brcmfmac4364b2-pcie.txcap_blob"
                do
                    if [ -f "$file" ]
                    then
                        sudo cp ${verbose} "$file" /lib/firmware/brcm
                    fi
                done

And why has this been removed?

[AdityaGarg8]

Also replace == with = in comparisons. IIRC == is bash specific, not POSIX compliant

[sharpenedblade]

This isnt the latest version yet, I havent pushed my changes. I will request a review when im done.

[sharpenedblade]

And why has this been removed?

Its appended to the tar archive so its not placed in the ESP anymore.

[AdityaGarg8]

And why has this been removed?

Its appended to the tar archive so its not placed in the ESP anymore.

It's still placed in esp, and should be placed on esp

https://github.com/t2linux/wiki/blob/d2bc82844e3da299dfad2eb1dcb56a0a6abd9a6b/docs/tools/firmware.sh#L720

[AdityaGarg8]

This isnt the latest version yet, I havent pushed my changes. I will request a review when im done.

Mark it as a draft then

[sharpenedblade]

And why has this been removed?

Its appended to the tar archive so its not placed in the ESP anymore.

It's still placed in esp

The current pushed branch is in a half-fixed state, its in the ESP on my local branch

and should be placed on esp

Why? Its just like the other firmware files, we just need to rename it on macos

[AdityaGarg8]

And why has this been removed?

Its appended to the tar archive so its not placed in the ESP anymore.

It's still placed in esp

The current pushed branch is in a half-fixed state, its in the ESP on my local branch

and should be placed on esp

Why? Its just like the other firmware files, we just need to rename it on macos

Because ESP files are renamed on Linux, not macOS

[sharpenedblade]

Im not sure exactly what to put in the help output, its a bit unclear right now. Im open to renaming the subcommands too.

[sharpenedblade]

Because ESP files are renamed on Linux, not macOS

I put it in the firmware-raw.tar.gz bundle but copy it on linux from the extracted tarball instead of giving it to the python script.

[AdityaGarg8]

Im not sure exactly what to put in the help output, its a bit unclear right now. Im open to renaming the subcommands too.

I'd rather say add the rename only and get from macOS functionality. It would be better to give options to users rather than doing automatation. I don't think there will be a big gain by just reducing the step of choosing the method by the user. In fact passing these long parameters is a bigger pain.

[AdityaGarg8]

Get from macOS should be good if you want to add some sort of systemd service which you planned before. Rename only should be good as script is no longer polyglot

[AdityaGarg8]

Try to make things simple.

[AdityaGarg8]

Because ESP files are renamed on Linux, not macOS

I put it in the firmware-raw.tar.gz bundle but copy it on linux from the extracted tarball instead of giving it to the python script.

Change was absolutely unnecessary, but it's fine if it works.

[sharpenedblade]

I don't think there will be a big gain by just reducing the step of choosing the method by the user. In fact passing these long parameters is a bigger pain.

There is no UX change if you run the script without any subcommands. If you dont pass anything then it uses the old code which interactively prompts the user.

I'd rather say add the rename only and get from macOS functionality.

rename_only is the same as running the old script with python3. get_from_macos is just method 4 from before.

Change was absolutely unnecessary, but it's fine if it works.

The idea was to add just firmware.tar.gz to the ESP in the future with renamed files in it.

[sharpenedblade]

Try to make things simple.

In the script in general or just the argument parsing code? Argument parsing in bash is a PITA, especially without gnu-isms. Im not sure how much simpler it can be.

[AdityaGarg8]

I don't think there will be a big gain by just reducing the step of choosing the method by the user. In fact passing these long parameters is a bigger pain.

There is no UX change if you run the script without any subcommands. If you dont pass anything then it uses the old code which interactively prompts the user.

there is no UX change even after refactoring and cleanup, still we are agreeing to it, cause it makes code less bulky. Similarly, we don't really need these automation features cause it's not useful at all.

I'd rather say add the rename only and get from macOS functionality.

rename_only is the same as running the old script with python3. get_from_macos is just method 4 from before.

Change was absolutely unnecessary, but it's fine if it works.

The idea was to add just firmware.tar.gz to the ESP in the future with renamed files in it.

[AdityaGarg8]

Try to make things simple.

In the script in general or just the argument parsing code? Argument parsing in bash is a PITA, especially without gnu-isms. Im not sure how much simpler it can be.

What I meant by try to make things simple was to not add unnecessary features like automation

[sharpenedblade]

Similarly, we don't really need these automation features cause it's not useful at all.

We can stick it inside of a systemd service and handle everything firmware related automatically. I also want to make a (very) simple gui to download the isos and get the firmware, so I want to be able to automate the firmware script. Its only ~20 LOC and the prompting should have been seperated from the actual code anyways.

[AdityaGarg8]

The only use case for automation I feel is that first boot systemd service of yours. That too I'll suggest first testing. I guess we can do automation in a different PR.

[sharpenedblade]

What I meant by try to make things simple was to not add unnecessary features like automation

I want to eventually make the installation seamless where you can just download a gui app, click your distro, reboot into linux, follow the installer, then have a working system.

[AdityaGarg8]

Similarly, we don't really need these automation features cause it's not useful at all.

We can stick it inside of a systemd service and handle everything firmware related automatically. I also want to make a (very) simple gui to download the isos and get the firmware, so I want to be able to automate the firmware script. Its only ~20 LOC and the prompting should have been seperated from the actual code anyways.

GUI seems to be a nice idea. But again, we can keep it for a different PR. Also, we need gui for both macOS and Linux then.

[sharpenedblade]

GUI seems to be a nice idea. But again, we can keep it for a different PR. Also, we need gui for both macOS and Linux then.

Im not going to PR that here, I just wanted to get the firmware script ready since I was already moving stuff around.

Also, we need gui for both macOS and Linux then.

It would be in something static like rust or go and in a cross platform toolkit

[AdityaGarg8]

Also, you probably can use one letter arguments if it's specifically to be run at backend as a systemd service or gui. Much easy to add here.

[AdityaGarg8]

Then you can do an it before read to check for the variable set by the argument and if it's missing, use read to get the variable

[sharpenedblade]

Also, you probably can use one letter arguments if it's specifically to be run at backend as a systemd service or gui. Much easy to add here.

Its 7 LOC to handle the long arguments so its not going to simplify things that much. I can change it but I thought its clearer to make them "subcommands"

[AdityaGarg8]

Also, you probably can use one letter arguments if it's specifically to be run at backend as a systemd service or gui. Much easy to add here.

Its 7 LOC to handle the long arguments so its not going to simplify things that much. I can change it but I thought its clearer to make them "subcommands"

So as I said before, add them when you really need them.

[AdityaGarg8]

I.e., when you have actually started working on your gui/systemd service

[AdityaGarg8]

You probably can keep them in your local copy, but I don't find any use of them for now in the wiki.

[sharpenedblade]

Il take the automation commit out and put it in a new PR with the systemd stuff

[AdityaGarg8]

Wasn't python working without sudo before?

[sharpenedblade]

Wasn't python working without sudo before?

No, when extracting the EFI tarball on linux we use sudo for every other command, which messes up permissions. I tested it and it was broken, but adding sudo to python3 fixes it.

[AdityaGarg8]

Wasn't python working without sudo before?

No, when extracting the EFI tarball on linux we use sudo for every other command, which messes up permissions. I tested it and it was broken, but adding sudo to python3 fixes it.

I don't think it's required for a method other than EFI

[AdityaGarg8]

Atleast that's what's done in the current version of the script

[sharpenedblade]

Atleast that's what's done in the current version of the script

There is no good way to execute a bash function with sudo.

[AdityaGarg8]

Atleast that's what's done in the current version of the script

There is no good way to execute a bash function with sudo.

Is there any good reason why we have switched to making the python bit a function instead of previous polyglot script.

[AdityaGarg8]

$USER will still not be root if running with sudo iirc

[sharpenedblade]

Atleast that's what's done in the current version of the script

There is no good way to execute a bash function with sudo.

I found a not-hacky fix for the permissions thats better anyways.

Is there any good reason why we have switched to making the python bit a function instead of previous polyglot script.

Not really, but the previous method was hacky and made us rely on $0.

[sharpenedblade]

$USER will still not be root if running with sudo iirc

That is the point, we want $workdir to be the current (non-root) user

[AdityaGarg8]

$USER will still not be root if running with sudo iirc

That is the point, we want $workdir to be the current (non-root) user

Looks good then.

[AdityaGarg8]

Also for the Fix Message commit, I dunno if "$package is missing" would be right for makepkg, cause the logic checks for makepkg as well as coreutils.

[sharpenedblade]

Also for the Fix Message commit, I dunno if "$package is missing" would be right for makepkg, cause the logic checks for makepkg as well as coreutils.

I split that if up to only install the right packages.

[AdityaGarg8]

Also for the Fix Message commit, I dunno if "$package is missing" would be right for makepkg, cause the logic checks for makepkg as well as coreutils.

I split that if up to only install the right packages.

It was intentionally not split so that latest version of both is installed. Brew for some reason hasn't included coreutils as a dependency but without it makepkg doesn't work.

[sharpenedblade]

Also for the Fix Message commit, I dunno if "$package is missing" would be right for makepkg, cause the logic checks for makepkg as well as coreutils.

I split that if up to only install the right packages.

It was intentionally not split so that latest version of both is installed. Brew for some reason hasn't included coreutils as a dependency but without it makepkg doesn't work.

GNU coreutils are stable so it shouldnt really matter what version they are as long as it is somewhat modern.