cygwin buildfix #21006

baentsch · 2023-05-20T04:51:03Z

Implements the cygwin-specific build remediation documented in #19531. Without this, "master" fails to build on cygwin.

t8m · 2023-05-22T11:19:08Z

Could you please amend the commit message to add Fixes #19531 ?

baentsch · 2023-05-22T11:38:33Z

Could you please amend the commit message to add Fixes #19531 ?

Done.

t8m · 2023-05-23T07:30:05Z

The Fixes note should be on a separate line in the commit message body (body is separated from the title of the commit by one empty line). No need to amend the commit now, we can do it when merging.

baentsch · 2023-05-23T09:09:51Z

Thanks for the explanation. Will do my best to remember for next PR commit. Or rather, may it be an idea to update https://github.com/openssl/openssl/blob/master/.github/PULL_REQUEST_TEMPLATE.md with more definite reminders for all contributor's benefit (what to check & do for a PR)?

I also understand that I won't have to do anything on this PR any more: Please correct me if I'm wrong.

baentsch · 2023-05-29T12:09:57Z

Just found this problem when doing an oqsprovider CI job for cygwin... Would a PR be welcome adding github-action-based cygwin testing to this project as well? Or even add it to this PR?

t8m · 2023-05-29T14:20:53Z

Adding it to this PR would be OK.

tmshort

Just a nit.

tmshort · 2023-06-01T15:14:36Z

Configurations/10-main.conf

@@ -1717,6 +1717,7 @@ my %targets = (
        CFLAGS           => picker(default => "-Wall",
                                   debug   => "-g -O0",
                                   release => "-O3"),
+        ex_libs          => add("-lcrypt32"),


Noting that the 64bit version of this library is still called crypt32.lib, but in a different architecture directory.

Goodness, this. Accordingly we should successfully be able to test "win32" and "win64". See in f09f774 .

It was really just a note to me, to not confuse 32-bit vs 64-bit... sigh.

tmshort · 2023-06-01T15:15:34Z

providers/implementations/storemgmt/winstore_store.c

@@ -23,7 +23,9 @@
 #include "prov/implementations.h"
 #include "prov/bio.h"
 #include "file_store_local.h"
-
+#ifdef __CYGWIN__
+#include <Windows.h>


Nit: should the include file be capitalized?

Just to check capitalization doesn't matter on Windows :-) Fund aside: Changed in f09f774. Will be disappointed if it fails...

openssl-machine · 2023-06-02T16:00:17Z

This pull request is ready to merge

baentsch · 2023-06-04T12:46:19Z

Adding it to this PR would be OK.

So done in f09f774.

baentsch · 2023-06-04T16:28:14Z

CI now passes (and fails without the code changes), so asking for a new review @t8m @tmshort

tmshort · 2023-06-08T03:31:56Z

The new CI is failing... Issues with isspace() that weren't fixed in #21127

baentsch · 2023-06-08T06:40:19Z

The new CI is failing... Issues with isspace() that weren't fixed in #21127

Can re-enable strict checks here after #21151 has landed.

baentsch · 2023-06-08T10:38:39Z

Instead of adding another .yml for the test, could you add the cygwin job to the Windows Github CI .yml?

Sure. Will do after #21151 has landed to test all together. Please let me know whether you're OK that we're not running "make test" as part of this CI: quic, symbols and simpletest (timer) fail under cygwin. Otherwise, I'll need to create a few separate issues as all those failures puzzle me (and I don't know QUIC, the OSSL-specific use of nm and the timer precision mechanisms under cygwin sufficiently well, respectively).

t8m · 2023-06-08T12:55:48Z

I suggest keeping this PR without the make test and once it is merged opening a new draft one with enabling it + creating issues for the failures.

baentsch · 2023-06-10T04:06:09Z

The new CI is failing... Issues with isspace() that weren't fixed in #21127

@tmshort: Now fixed. strict checks now passing.

Instead of adding another .yml for the test, could you add the cygwin job to the Windows Github CI .yml?

@t8m: Now added to windows.yml. cygwin tests passing.

Please let me know if you'd still expect me to do anything on this PR. Edit/Add: CI failure is not due to cygwin.

t8m · 2023-06-10T10:50:41Z

providers/implementations/storemgmt/winstore_store.c

@@ -23,7 +23,9 @@
 #include "prov/implementations.h"
 #include "prov/bio.h"
 #include "file_store_local.h"
-
+#ifdef __CYGWIN__
+#include <windows.h>


Please add one space between # and include

done in aa85bd1

t8m

Approved, we will squash the commits when merging.

Changed since

tmshort

Looks fine, but there was still a failing test, seemingly not related to these changes? I'll approve, pending the CI.

EDIT: It looks as though check-ansi is failing in a number of other PRs.

openssl-machine · 2023-06-12T00:00:20Z

This pull request is ready to merge

t8m · 2023-06-12T06:19:40Z

Squashed and merged to master branch. Thank you for your contribution.

Fixes #19531 Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21006)

Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21006)

Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21006) (cherry picked from commit e3b01eb)

t8m approved these changes May 22, 2023

View reviewed changes

t8m removed the approval: otc review pending This pull request needs review by an OTC member label May 22, 2023

baentsch force-pushed the cygwin-buildfix branch from 7f254d9 to cc39e9e Compare May 22, 2023 11:38

tmshort previously approved these changes Jun 1, 2023

View reviewed changes

tmshort added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 1, 2023

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jun 2, 2023

baentsch mentioned this pull request Jun 4, 2023

Invocations of isdigit() questionable #21123

Closed

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Jun 5, 2023

baentsch force-pushed the cygwin-buildfix branch from 0dd466a to 6df8625 Compare June 7, 2023 11:52

baentsch mentioned this pull request Jun 8, 2023

Cast the argument to unsigned char when calling isspace() #21151

Closed

baentsch added 9 commits June 9, 2023 18:04

cygwin buildfix: Fixes openssl#19531

2436e6a

correct lib name

3715d06

add cygwin CI

ed3c01b

change to default checkout action

f8d3471

checkout before install

3d177f0

correct path

64fcfb0

re-enable stricht checking in cygwin

2be67bd

display gcc version used on cygwin

bae50fb

enable strict build again & merge cygwin into windows.yml

8e06f0b

baentsch force-pushed the cygwin-buildfix branch from bd5e104 to 8e06f0b Compare June 9, 2023 16:09

t8m reviewed Jun 10, 2023

View reviewed changes

PR feedback

aa85bd1

t8m approved these changes Jun 10, 2023

View reviewed changes

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jun 10, 2023

t8m requested a review from tmshort June 10, 2023 20:36

tmshort approved these changes Jun 10, 2023

View reviewed changes

tmshort added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 10, 2023

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jun 12, 2023

t8m closed this Jun 12, 2023

openssl-machine pushed a commit that referenced this pull request Jun 12, 2023

Fix build on cygwin

9b9c42d

Fixes #19531 Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21006)

openssl-machine pushed a commit that referenced this pull request Jun 12, 2023

add cygwin CI

e3b01eb

Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21006)

bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Aug 31, 2023

add cygwin CI

90616d5

Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#21006) (cherry picked from commit e3b01eb)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cygwin buildfix #21006

cygwin buildfix #21006

baentsch commented May 20, 2023

t8m commented May 22, 2023

baentsch commented May 22, 2023

t8m commented May 23, 2023

baentsch commented May 23, 2023

baentsch commented May 29, 2023

t8m commented May 29, 2023

tmshort left a comment

tmshort Jun 1, 2023

baentsch Jun 4, 2023

tmshort Jun 8, 2023

tmshort Jun 1, 2023 •

edited

baentsch Jun 4, 2023

openssl-machine commented Jun 2, 2023

baentsch commented Jun 4, 2023

baentsch commented Jun 4, 2023

tmshort commented Jun 8, 2023 •

edited

baentsch commented Jun 8, 2023

baentsch commented Jun 8, 2023

t8m commented Jun 8, 2023

baentsch commented Jun 10, 2023 •

edited

t8m Jun 10, 2023

baentsch Jun 10, 2023

t8m left a comment

tmshort left a comment •

edited

openssl-machine commented Jun 12, 2023

t8m commented Jun 12, 2023

cygwin buildfix #21006

cygwin buildfix #21006

Conversation

baentsch commented May 20, 2023

t8m commented May 22, 2023

baentsch commented May 22, 2023

t8m commented May 23, 2023

baentsch commented May 23, 2023

baentsch commented May 29, 2023

t8m commented May 29, 2023

tmshort left a comment

Choose a reason for hiding this comment

tmshort Jun 1, 2023

Choose a reason for hiding this comment

baentsch Jun 4, 2023

Choose a reason for hiding this comment

tmshort Jun 8, 2023

Choose a reason for hiding this comment

tmshort Jun 1, 2023 • edited

Choose a reason for hiding this comment

baentsch Jun 4, 2023

Choose a reason for hiding this comment

openssl-machine commented Jun 2, 2023

baentsch commented Jun 4, 2023

baentsch commented Jun 4, 2023

tmshort commented Jun 8, 2023 • edited

baentsch commented Jun 8, 2023

baentsch commented Jun 8, 2023

t8m commented Jun 8, 2023

baentsch commented Jun 10, 2023 • edited

t8m Jun 10, 2023

Choose a reason for hiding this comment

baentsch Jun 10, 2023

Choose a reason for hiding this comment

t8m left a comment

Choose a reason for hiding this comment

tmshort left a comment • edited

Choose a reason for hiding this comment

openssl-machine commented Jun 12, 2023

t8m commented Jun 12, 2023

tmshort Jun 1, 2023 •

edited

tmshort commented Jun 8, 2023 •

edited

baentsch commented Jun 10, 2023 •

edited

tmshort left a comment •

edited