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

wiki/docs/tools/firmware.sh

Line 720 in d2bc828

if [[ (${identifier} = iMac19,1) || (${identifier} = iMac19,2) || (${identifier} = iMacPro1,1) ]]

[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.

[sharpenedblade]

Everything is working now. I think the logs are fixed too.

[AdityaGarg8]

rm ${verbose}

[AdityaGarg8]

Btw in cleanup_dmg function, workdir gets removed, which also has the dmg, so I wonder what's the actual advantage of specifically removing it here, since the next step before cleanup is simply firmware extraction and reloading drivers.

[sharpenedblade]

tmpfs is usually in ram, if you are running low on ram the script crashes. The DMG is almost 2GB so this saves a lot of ram space.

[sharpenedblade]

I would prefer to overwrite BaseSystem.dmg with fw.img without using extra space, but I dont know how to do that

[AdityaGarg8]

tmpfs is usually in ram, if you are running low on ram the script crashes. The DMG is almost 2GB so this saves a lot of ram space.

But it is not removed unless img is made, so what's the benefit here?

[sharpenedblade]

When I was testing the script it would crash a lot here and complain about low disk space, but once I added the rm it usually worked.

[sharpenedblade]

Im going to move the dmg dir to somewhere on disk.

[AdityaGarg8]

Don't squash the commit addressing these changes.

[AdityaGarg8]

nvram_txcap_quirk is broken in Method 1 (I haven't tested other methods yet). Cause is quite evident by the code itself. You are making a firmware-raw.tar.gz for the main firmware and firmware-raw.tar for extra firmware. After that you are only extracting firmware-raw.tar.gz on Linux and firmware-raw.tar is untouched.

I don’t understand how you moved the extra files to workdir on Linux as well.

Please test whatever you change.

[AdityaGarg8]

Also, separate out python_check for macOS in a separate function like it was in the older version. There is not need to unnecessarily complicate the create_firmware_archive

[AdityaGarg8]

No need to squash the commits btw.

[sharpenedblade]

I don’t understand how you moved the extra files to workdir on Linux as well.

Once I add those files to the right archive it will work. When the tarfile is extracted those files will be in $PWD, which is $workdir

Please test whatever you change.

I dont have a mac where that code is used.

Also, separate out python_check for macOS in a separate function like it was in the older version. There is not need to unnecessarily complicate the create_firmware_archive

Ok

[AdityaGarg8]

I don’t understand how you moved the extra files to workdir on Linux as well.

Once I add those files to the right archive it will work. When the tarfile is extracted those files will be in $PWD, which is $workdir

Please test whatever you change.

I dont have a mac where that code is used.

You can change the model identifier to your Mac and path to the firmware folder to your chip. That's how I test on my Mac.

Also, separate out python_check for macOS in a separate function like it was in the older version. There is not need to unnecessarily complicate the create_firmware_archive

Ok

[AdityaGarg8]

What are you adding python_check in create_firmware_archive? Just put it in macOS specific code.

[sharpenedblade]

What are you adding python_check in create_firmware_archive? Just put it in macOS specific code.

I moved it

[AdityaGarg8]

Script looks good to me now, I've tested it on Ubuntu as well as Arch. I wonder if there could be a way to use /tmp if storage in RAM is sufficient. I would prefer not to write much on the SSD for such tasks.

[sharpenedblade]

I wonder if there could be a way to use /tmp if storage in RAM is sufficient.

I think its better to use the disk since a lot of macs only have 8Gb of RAM and if you have swap enabled its going to write to disk anyways.

I would prefer not to write much on the SSD for such tasks.

It shouldn't be a problem, a lot of things write more than this to disk. SSD wear only matters if you write large amounts of data regularly, a user will probably only run this script once.