libwdi

libwdi copied to clipboard

This patch-set includes various build fixes that were needed to build libwdi inside Msys2.

Thanks for the PR! Please find my comments below:

• DLL_EXPORT to LIBWDI_DLL_EXPORT → That's fine with me
• CRLFs in zadig.h → That's weird. I've always made sure that my git clients on Windows were configured to leave the default client line encoding alone, unless specifically set in .gitattributes (and we don't have a rule for .h there), so I have no idea how the GitHub repo ended up with a CRLF source... This needs fixing indeed.
• is_x64 and Windows version as explicit libwdi APIs → I have to disagree with this. Libwdi is a driver installation library, not a 32 vs 64-bit or Windows version detection library. The focus and specific purpose of the library should be with drivers (and possibly items that are directly associated with driver installation landscape such as getting a USB vendor name, because this may be helpful for the end user). I don't see getting the Windows version fit that scope, especially as we don't have anything in our APIs that warrants caring about the Windows version, and, more importantly, this would give an implicit contract that I (not you) will both be providing accurate Windows version reporting and will be updating the API as Microsoft releases new versions of Windows which, if the current internal version detection continues to work in the context of libwdi on Windows 12, Windows 13 or whatnot, I really see no incentive to invest time doing. And that's not counting the need to properly document these new APIs. Plus Microsoft makes Windows detection more and more difficult these days, and we've actually had to take a guess on a build number watershed to differentiate between Windows 10 and Windows 11, so even right now, I can't make a promise that our Windows versioning is accurate, which is all the more reason I want to keep it internal.
  Just like it is the case in the UNIX model, while it may be tempting to do so, I consider that a library or application should limit its scope to a specific mandate (do one thing and do it well), which, in the case of libwdi is the installation of a Windows driver and any direct corollary tasks . In that context, Windows version detection, if downstream users of libwdi need it, should not be provided by libwdi but by a separate dedicated library or some other means, because this is clearly outside of the library's official scope. Otherwise, if you start opening the door to adding loosely related API on top of loosely related API onto a library, the only natural endgame is for that library to eventually become a self contained full fledged OS, to satisfy everyone...
  So, while I am fine with adding improvements to the versioning, I am not fine with making the versioning of x64 detection APIs public. If you need version/x64 detection in your application, please refrain from the temptation of adding a "wart" to a third party library, even if it seems like a good place to do so, on account that it would solve a specific unrelated downstream problem of yours.
• stdfn.h removal → Well, this was mostly used for Windows versioning, so I'm fine with moving that stuff elsewhere and get rid of it, as long as it doesn't require making the Windows versioning API public.
• .o to .lo → Can you explain what issue you're trying to fix here? We're producing executables, so I'm not sure what's wrong with using .o for the compiled .rc object that ends up in the final executables instead of .lo. Does msys2 actually complain about this?

is_x64 and Windows version as explicit libwdi APIs → I have to disagree with this.

I quite understand your comment. I should have mentioned in my merge request that I'm trying to fix issues affecting the shared library build. Sorry for the confusion. It has been a while since I wrote these patches.

As you can see, there are linker errors affecting the zadig build, because the Windows Version and Architecture code isn't explicitly exported:

 $ ./configure --with-wdkdir=/path/to/wdk-dir/ --disable-static --enable-shared --disable-32bit --enable-64bit --enable-examples-build
    ...
 $ make
    ...
  CCLD     zadig.exe
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: zadig-zadig.o: in function `WinMain':
C:\workspace\libwdi\examples/zadig.c:1755: undefined reference to `GetWindowsVersion'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: zadig-zadig.o:zadig.c:(.rdata$.refptr.nWindowsVersion[.refptr.nWindowsVersion]+0x0): undefined reference to `nWindowsVersion'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: zadig-zadig.o:zadig.c:(.rdata$.refptr.WindowsVersionStr[.refptr.WindowsVersionStr]+0x0): undefined reference to `WindowsVersionStr'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: zadig-zadig_net.o: in function `CheckForUpdatesThread':
C:\workspace\libwdi\examples/zadig_net.c:513: undefined reference to `is_x64'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\workspace\libwdi\examples/zadig_net.c:533: undefined reference to `is_x64'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: zadig-zadig_net.o: in function `DownloadFile':
C:\workspace\libwdi\examples/zadig_net.c:313: undefined reference to `is_x64'
collect2.exe: error: ld returned 1 exit status
make[2]: *** [Makefile:381: zadig.exe] Error 1
make[2]: Leaving directory '/c/workspace/gsync-tool-pack/libwdi/examples'
make[1]: *** [Makefile:412: all-recursive] Error 1
make[1]: Leaving directory '/c/workspace/gsync-tool-pack/libwdi'
make: *** [Makefile:344: all] Error 2

In other words, even though you don't want libwdi to expose these APIs, it currently does do so implicitly in the static build because the code is reused. This patch simply makes these APIs explicit.

Do you have any thoughts about an alternative approach?

.o to .lo → Can you explain what issue you're trying to fix here? We're producing executables, so I'm not sure what's wrong with using .o for the compiled .rc object that ends up in the final executables instead of .lo. Does msys2 actually complain about this?

The reason for this is that without the fix I get a linker error in the MSYS2 MinGW x64 build environment:

 $ ./configure --with-wdkdir=/path/to/wdk-dir/ --disable-static --enable-shared --disable-32bit --enable-64bit --enable-examples-build
    ...
 $ make
    ...
  RC     zadig_rc.o
  CCLD     zadig.exe
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: cannot find zadig_rc.o: No such file or directory
collect2.exe: error: ld returned 1 exit status
make[2]: *** [Makefile:381: zadig.exe] Error 1
make[2]: Leaving directory '/c/workspace/gsync-tool-pack/libwdi/examples'
make[1]: *** [Makefile:412: all-recursive] Error 1
make[1]: Leaving directory '/c/workspace/gsync-tool-pack/libwdi'
make: *** [Makefile:344: all] Error 2

Given that libtool is wrapping the linker and the resource compiler, we have the .lo file, and passing it through to the libtool linker fixes the build error.

@pbatard Did you have time to have a closer look at this yet? Are there any changes you would like to see?

Yeah, sorry for the delay, but I'm afraid that libwdi has become a low priority project for me. I'm basically waiting to have nothing with higher priority to take a proper look at this... which I don't think is going to happen at least before Rufus 3.21 is out.

Makes sense. The project is mostly done - except for a few portability improvements like the ones you see here.

I'd like to get these changes in. I think they're a good improvement to the build system, and each patch is fairly self-contained, so could be cherry-picked individually.

As for is_x64 - I agree it's a bit ugly, though I don't think I'm making the existing situation worse than it already is; just fixing the linkage issue. There's a bigger question of whether there is a better design that can replace this API later.

With my apologies for taking so long to finally work on this PR, the various issues you highlighted above should now be fixed.

Please don't hesitate to let me know if you are still experiencing problems, and thanks again for your contribution!