Skip to content

ci: windows support #105

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

Merged
merged 2 commits into from
Apr 22, 2022
Merged

ci: windows support #105

merged 2 commits into from
Apr 22, 2022

Conversation

carenas
Copy link
Contributor

@carenas carenas commented Apr 20, 2022

Configure and build (using cmake with Visual Studio) in Windows and validate no regressions are introduced by running some of the tests

Still barebones and only to serve as a starting point and guideline for
how to integrate mostly non autotools environments.

Selects Intel 32-bit specifically as it is the one that has been tested
the most and also has the less number of warnings.

Test should be improved further so it is at least equivalent to what is
done in Linux, but that is orthogonal to having it integrated, and the
tests that were disabled would work locally (albeit in a newer version),
so this at least does the minimum to prevent regressions by validating
both the interpreter and JIT.
set do11=yes
set do12=yes
set do12=no
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my worry. If something does not work, we might not be able to do anything with it, just disable tests. CI is usually good for actively developed targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, we don't have yet functionality that allow us to debug remotely broken tests, which is why the only option now is to disable them.

but I happened to already build that for windows just because I was curious on why those 2 tests were failing and you can see both the code and a run with a downloadable set of files showing the output in:

https://github.com/carenas/pcre2/actions/runs/2199761640

there is also a tentative solution for Linux there, and of course fixes pending for the real underlying issue (which I am still puzzled about, because it will seem to be an extra empty line from pcre2test, and is consistently broken in that old OS github provides for its action VM)

@PhilipHazel PhilipHazel merged commit f28e826 into PCRE2Project:master Apr 22, 2022
@PhilipHazel
Copy link
Collaborator

I have merged this, but like Zoltan I don't like the idea of disabling some of the tests. Let's hope this can be resolved before the next release.

@ltrzesniewski
Copy link
Contributor

Note that the x86 build with VS produces a warning:

D:\a\pcre2\pcre2\src\pcre2_jit_compile.c(5889,40): warning C4018: '>=': signed/unsigned mismatch [D:\a\pcre2\pcre2\build\pcre2-16-static.vcxproj]

But this looks like a compiler issue (there's no warning on x64 for instance). I always apply this patch: ltrzesniewski/pcre-net@3c8c0e9 to fix this, since I compile with warnings as errors. You didn't want to include this patch upstream, but maybe it could be considered in an #if which checks for VS?

I don't get the C4146 warnings in my builds though.

@zherczeg
Copy link
Collaborator

PCRE2_UCHAR is converted to int by msvc? Strange. Signedness usually kept when promoting to larger value size.

@ltrzesniewski
Copy link
Contributor

I remember I didn't understand why it emitted this warning back when I had the issue the first time. The code looked correct. 😕

@carenas carenas deleted the win32 branch April 22, 2022 15:16
@PhilipHazel
Copy link
Collaborator

I suspect it's not the PCRE2_UCHAR that is the problem. My guess is that "2" is being given the type int. Could you try change that to "2u"? (Just guessing.)

@ltrzesniewski
Copy link
Contributor

Changing 2 to 2u didn't fix the issue, it's caused by the a_pri + b_pri >= max_pri part of the expression.

I think I understand what's going on though. I'll open a separate PR so we can discuss this there.

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.

4 participants