8000 Various tidy-ups and build fixes by jhol · Pull Request #274 · pbatard/libwdi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Various tidy-ups and build fixes #274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Various tidy-ups and build fixes #274

wants to merge 6 commits into from

Conversation

jhol
Copy link
Contributor
@jhol jhol commented Sep 14, 2022

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

@pbatard
Copy link
Owner
pbatard commented Sep 16, 2022 •
edited
Loading

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?

@jhol
Copy link
Contributor Author
jhol commented Sep 21, 2022

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
    
8000
...
 $ 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.

@jhol
Copy link
Contributor Author
jhol commented Oct 18, 2022

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

@pbatard
Copy link
Owner
pbatard commented Oct 18, 2022 •
edited
Loading

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.

@jhol
Copy link
Contributor Author
jhol commented Oct 18, 2022

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.

@pbatard
Copy link
Owner
pbatard commented Feb 24, 2023

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!

pbatard added a commit that referenced this pull request Feb 24, 2023
@pbatard
[misc] don't use libtool when invoking windres
ae66132 
* When building a shared library, libtool prouces _rc.lo instead of _rc.o => Call windres directly.
* Addresses part of #274.
* Also fix sed's mishandling of CRLFs...
pbatard pushed a commit that referenced this pull request Feb 24, 2023
@jhol @pbatard
[misc] replace CRLFs to LFs in examples/zadig.h
b66b7a0 
* Part of #274.
pbatard added a commit that referenced this pull request Feb 24, 2023
@pbatard
[zadig] fix symbol not found errors when compiling with dynamic lib
dae3093 
* Duplicate the Windows version lookup since we don't want to export the API.
* Addresses part of #274.
@pbatard pbatard closed this in 11f53e6 Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhol @pbatard
0