search

hamishcoleman / thinkpad-ec

Infrastructure for examining and patching Thinkpad embedded controller firmware
GNU General Public License v2.0
1.07k stars 115 forks source link

Add makefile target to download README text file for ISO images #21

Closed ypid closed 7 years ago

hamishcoleman commented 7 years ago

Can you change it to call the resulting files "%.txt.orig" to closer match their name from lenovo.

Ideally we would have a way to reject invalid filenames - otherwise we could "make fred.txt.orig" and have it go try to fetch that. I dont think that adding the txt files to the Description.txt makes a lot of sense - it feels too much like duplication, but checking if there is a %.iso.orig line in there could work.

ypid commented 7 years ago

Can you change it to call the resulting files "%.txt.orig" to closer match their name from lenovo.

It seems that Lenovo offers different READMEs to match the different upgrade methods. That seems to be encoded in the last two chars in the filename:

uc: Something like "Upgrade via CD" us: Something like "Upgrade via M$ software"

Thats why I left the iso in the filename.

Ideally we would have a way to reject invalid filenames

Good idea. I added a simple check to ensure the filename prefix is 8 chars. Could be extended to regex but I don’t have enough information what the filename actually means.

hamishcoleman commented 7 years ago

For the filename check, I was thinking more along the lines of:

-       perl -e 'die "Was expecting a 8 charactar filename prefix as used by Lenovo." if length("$(subst .iso.orig,,$@)") != (8 + 4)'
+       @echo -n "Downloading release notes for "
+       @scripts/describe $(subst .txt,,$@)

With the file naming, the .orig suffix has been consistently used to mark a file that has its original contents from the upstream vendor, so that is another reason to not use the names in your patch. I still think just sticking to the lenovo names is the best option - including the "uc" that you mention, thus "g2uj25uc.txt.orig"

ypid commented 7 years ago

Thanks for merging and your feedback. I was/am a bit busy the last days but I wanted to get back to this PR. Updating the PR and merging then is also a good idea.