Skip to content
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.

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

Fix vs2019 pre-commit failures relating to zlib warnings #5766

Merged
merged 7 commits into from
Nov 30, 2022

Conversation

cmannett85-arm
Copy link
Collaborator

PRs are currently held up by vs2019 warnings-as-errors, relating to lossy implicit integer conversions.

PRs are currently held up by vs2019 warnings-as-errors, relating
to lossy implicit integer conversions.
@cmannett85-arm
Copy link
Collaborator Author

@derekbruening Help! I'm trying to fix the vs2019 pre-commits, thinking that they were just caused by some simple lossy implicit conversions, but it's turning into something more complicated. I simply don't know about how the project is built, and I don't have access to a Windows machine I can try and build locally on.

This is from the last successful build of vs2019-32 (aa99f09):

##[group]Runner Image
Image: windows-2019
Version: 20221027.1
...
Could NOT find ZLIB (missing: ZLIB_LIBRARY) (found version "1.2.11")

And this is from the my precommit:

##[group]Runner Image
Image: windows-2019
Version: 20221119.1
...
Found ZLIB: C:/Strawberry/c/lib/libz.a (found version "1.2.11") 

The hosted runner images haves been updated, and it now includes zlib, which enables a define, which unlocks new code:

D:\a\dynamorio\dynamorio\clients\drcachesim\common/gzip_istream.h(86): error C2220: the following warning is treated as an error
D:\a\dynamorio\dynamorio\clients\drcachesim\common/gzip_istream.h(86): warning C4244: 'argument': conversion from '__int64' to 'int', possible loss of data
D:\a\dynamorio\dynamorio\clients\drcachesim\common/zlib_istream.h(113): warning C4244: 'argument': conversion from '__int64' to 'int', possible loss of data

I corrected those errors, but I'm hitting unresolved symbol linker errors in the vs2019-32 build. However I think they're related to this, rather than a missing library:

C:\Strawberry\c\lib\libz.a : warning LNK4272: library machine type 'x64' conflicts with target machine type 'x86'

The x64 builds are successful, but then go onto fail the unit tests quite badly.

@derekbruening
Copy link
Contributor

Filed #5767 as this also broke the weekly build

@derekbruening
Copy link
Contributor

I corrected those errors, but I'm hitting unresolved symbol linker errors in the vs2019-32 build. However I think they're related to this, rather than a missing library:

C:\Strawberry\c\lib\libz.a : warning LNK4272: library machine type 'x64' conflicts with target machine type 'x86'

So they only installed the 64-bit but not 32-bit zlib? And the cmake code finds the wrong-bitwidth one, which is not separated into a dir with the bitwidth in the name. I would think others might hit this too? Maybe we should file a bug vs the github actions runners if there is not one already and see if there is some solution when installing strawberry zlib. If we have to fix it ourselves we'll have to write cmake code to check the bitwidth and maybe add to PATH (see below).

The x64 builds are successful, but then go onto fail the unit tests quite badly.

Do you mean this:

228: <Application D:\a\dynamorio\dynamorio\build_debug-internal-64\suite\tests\bin\client.winxfer.exe (6416). Unable to load client library: zlib1__.dll
228: 	Cannot find library.>

That is a PATH issue. I wonder if it is a github runner issue as DR's private loader should search the PATH.

@cmannett85-arm
Copy link
Collaborator Author

So they only installed the 64-bit but not 32-bit zlib?
We're using a 64bit OS (Windows Server 2019), so the provided version of Perl is also 64 bit. Strawberry Perl also ships with a load of libs which includes a 64bit zlib.

I wouldn't consider this a bug with GH as we're building against a library that ships with an unrelated program. Ideally we should be asking Chocolatey or vcpkg to install a specific version of zlib, but that's a bigger change.

@derekbruening
Copy link
Contributor

I'm thinking let's unblock the suite by just disabling zlib when AUTOMATED_TESTING and WIN32 are both set and let #5767 cover further improvements. I'm putting that in now on top of the warnings you've already solved.

…; instead set AUTOMATED_TESTING in base_cache
@derekbruening derekbruening merged commit 1f40176 into master Nov 30, 2022
@derekbruening derekbruening deleted the iX-zlib-pre-commit-fix branch November 30, 2022 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants