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

bd80d6b breaks builds for me #43

Closed bwachter closed 7 years ago

bwachter commented 7 years ago

The change to mcopy ~2 months ago breaks building for me (opensuse Tumbleweed):

touch -d @1 g2uj25us.iso.bat cp --reflink=auto g2uj25us.iso.orig g2uj25us.iso && ./scripts/copyFL2 to_iso g2uj25us.iso x230.G2HT35WW.s01D3000.FL2 && sed -i "s/__BUILT/sha1sum x230.G2HT35WW.s01D3000.FL2/" g2uj25us.iso.bat && mcopy -m -o -i g2uj25us.iso@@71680 g2uj25us.iso.bat ::AUTOEXEC.BAT && mdel -i g2uj25us.iso@@71680 ::EFI/Boot/BootX64.efi Error: could not find any FL2 files in g2uj25us.iso make: *** [Makefile:331: g2uj25us.iso] Error 1

Building from head (89483c4) is a bit less verbose, but same issue:

./scripts/generate_deps Descriptions.txt >.d/generated.deps ./scripts/ISO_copyFL2 from_iso g2uj25us.iso.orig x230.G2HT35WW.s01D3000.FL2.orig 01D3000.FL2 Error: could not find any files in g2uj25us.iso.orig matching 01D3000.FL2 make: *** [.d/generated.deps:33: x230.G2HT35WW.s01D3000.FL2.orig] Error 1

hamishcoleman commented 7 years ago

A change to mtools? I had thought that that was a pretty feature complete package. Can you post the version that is currently installed? Do you know the version that was installed prior to the update?

Also, can you run "bash -x ./scripts/ISO_copyFL2 from_iso g2uj25us.iso.orig x230.G2HT35WW.s01D3000.FL2.orig 01D3000.FL2" and post the output?

bwachter commented 7 years ago

I believe you misunderstood - commit bd80d6b switches to using mtools for extracting fl2 files. That's what broke things for me after I got back to this after half a year, not an update on my system.

I've built a version with the commit just before (47b3a6a), without issues, apart from the fl2 pathname in autoexec.bat to be only generated as \ (an issue I had half a year already, but forgot about)

mtools installed is mtools-4.0.18-8.9.x86_64

$ bash -x ./scripts/ISO_copyFL2 from_iso g2uj25us.iso.orig x230.G2HT35WW.s01D3000.FL2.orig 01D3000.FL2
+ FAT_OFFSET=71680
+ DIR=from_iso
+ case "$DIR" in
+ shift
+ ISO=g2uj25us.iso.orig
+ '[' '!' -e g2uj25us.iso.orig ']'
+ shift
+ FILENAME=x230.G2HT35WW.s01D3000.FL2.orig
+ '[' -z x230.G2HT35WW.s01D3000.FL2.orig ']'
+ shift
+ INTPATTERN=01D3000.FL2
+ '[' -z 01D3000.FL2 ']'
++ mdir -i g2uj25us.iso.orig@@71680 -/ -b
++ grep 01D3000.FL2
+ MATCH=
+ '[' -z '' ']'
+ echo 'Error: could not find any files in g2uj25us.iso.orig matching 01D3000.FL2'
Error: could not find any files in g2uj25us.iso.orig matching 01D3000.FL2
+ exit 1
hamishcoleman commented 7 years ago

Yes, I did misunderstand you, but luckily the error message you are getting is what I started to look into - and that has the same diagnostics for your actual problem.

It does look like mtools is doing something different for your system - can you run "mdir -i g2uj25us.iso.orig@@71680 -/ -b" and paste the output?

When I run that on my system, I get one line for each file, each one looking similar to: ::/FLASH/G2ETA8WW/306a9.hsh ::/FLASH/G2ETA8WW/$01D3000.FL2

So, that output gets grepped for 01D3000.FL2 and I get a match - I figure something different is happening for you with that command

bwachter commented 7 years ago

The output for me is lower case: ::/flash/g2eta8ww/$01d3000.fl2

Changing the grep in scripts/ISO_copyFL2 to grep -i fixes image builds for me.

For getting correct autoexec.bat I guess both the grep and sed in the Makefile would need to be case insensitive as well. Also, the match probably should be changed to only match at end of line, just in case.

hamishcoleman commented 7 years ago

Well, that'll do it..

I might also have a look at mtools to see if there is a hidden way to convince it to generate the uppercase output

bwachter commented 7 years ago

With this diff image builds work for me and I get a correct autoexec.bat:

diff --git a/Makefile b/Makefile
index 368a72b..75aa5a8 100644
--- a/Makefile
+++ b/Makefile
@@ -177,7 +177,7 @@ endef
 # my mind to it..
 #
 %.iso.bat: %.iso.orig autoexec.bat.template
-       sed -e "s%__DIR%`mdir -/ -b -i $<@@$(FAT_OFFSET) |grep FL2 |head -1|cut -d/ -f3`%; s%__FL2%`mdir -/ -b -i $<@@$(FAT_OFFSET) |grep FL2 |head -1|cut -d/ -f4`%" autoexec.bat.template >$@.tmp
+       sed -e "s%__DIR%`mdir -/ -b -i $<@@$(FAT_OFFSET) |grep -i FL2 |head -1|cut -d/ -f3`%; s%__FL2%`mdir -/ -b -i $<@@$(FAT_OFFSET) |grep -i FL2 |head -1|cut -d/ -f4`%i" autoexec.bat.template >$@.tmp
        mv $@.tmp $@
        touch -d @1 $@

diff --git a/scripts/ISO_copyFL2 b/scripts/ISO_copyFL2
index b839543..676586a 100755
--- a/scripts/ISO_copyFL2
+++ b/scripts/ISO_copyFL2
@@ -45,7 +45,7 @@ if [ -z "$INTPATTERN" ]; then
 fi

-MATCH=$(mdir -i "$ISO"@@"$FAT_OFFSET" -/ -b |grep "$INTPATTERN")
+MATCH=$(mdir -i "$ISO"@@"$FAT_OFFSET" -/ -b |grep -i "$INTPATTERN")
 if [ -z "$MATCH" ]; then
     echo "Error: could not find any files in $ISO matching $INTPATTERN"
     exit 1
hamishcoleman commented 7 years ago

Can you check the output of mtoolstest? when I run it, I get a mtools_lower_case=0 line. I had a look at my /etc/mtools.conf and there is a commented line that would set mtools_lower_case=1 - can you have a look in that file too?

bwachter commented 7 years ago

/etc/mtools.conf does have mtools_lower_case=1 in opensuse default configuration

hamishcoleman commented 7 years ago

Ah! Well, that explains why things are different. It is a pity that this setting affects the machine-readable output modes.

Can you try adding "export MTOOLS_LOWER_CASE=0" to the Makefile? From my reading of the mtools source code, that should override that mtools conf setting

bwachter commented 7 years ago

Can you try adding "export MTOOLS_LOWER_CASE=0" to the Makefile?

It builds with that. I'll test if the filename for flashing is correct once I got the bezels for my new x230 (will then also test and submit x230 version of my control/caps swap and dvorak patches)