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.

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

Add a new CI step for windows environments #2979

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Add a new CI step for windows environments #2979

wants to merge 4 commits into from

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented May 4, 2024 •
edited
Loading

Add a new CI step on Windows to ensure the project compiles on this platform with mingw.

Motivation: During upstreaming process we discovered some dlsym code was incorrectly disabled, the project does not compile using mingw anymore.

@P-E-P P-E-P force-pushed the add_windows_ci branch 8 times, most recently from 08143f7 to 76059e5 Compare May 4, 2024 18:00
@P-E-P P-E-P marked this pull request as ready for review May 4, 2024 18:01
@P-E-P P-E-P marked this pull request as draft May 4, 2024 18:01
@P-E-P P-E-P force-pushed the add_windows_ci branch 6 times, most recently from 7bb7c81 to c7842c7 Compare May 4, 2024 19:40
@P-E-P P-E-P added the CI label May 4, 2024
@P-E-P P-E-P force-pushed the add_windows_ci branch from c7842c7 to b8b4055 Compare May 5, 2024 16:50
@P-E-P P-E-P mentioned this pull request May 6, 2024
Open
@P-E-P P-E-P force-pushed the add_windows_ci branch 11 times, most recently from 2549f88 to 17ed2a8 Compare May 7, 2024 18:49
@P-E-P P-E-P force-pushed the add_windows_ci branch from 17ed2a8 to 766c2cd Compare May 10, 2024 11:28
@P-E-P P-E-P force-pushed the add_windows_ci branch from 766c2cd to c344d31 Compare May 21, 2024 12:27
@P-E-P P-E-P force-pushed the add_windows_ci branch 2 times, most recently from aded3aa to 6cf78da Compare May 28, 2024 08:53
@P-E-P
Copy link
Member Author

P-E-P commented May 28, 2024

We'll need #2949

@P-E-P P-E-P force-pushed the add_windows_ci branch 3 times, most recently from 3b70b8b to ad6b741 Compare September 27, 2024 13:50
@P-E-P
Copy link
Member Author

P-E-P commented Oct 2, 2024

@MehdiChinoune Any idea about what is going on here ?

From what I understand the standard library requires some win32 functions, I tought linking with -lkernel32 would do the trick but I was proven wrong.

@MehdiChinoune
Copy link

Looks like those functions come from ws2_32 library.

@mati865
Copy link

mati865 commented Oct 2, 2024

In case you need a list of Win32 libraries that official compiler links check https://github.com/rust-lang/rust/blob/360f7d7b731c508c88b7b56921a92d281c05dbf7/compiler/rustc_target/src/spec/base/windows_gnu.rs#L25 and https://github.com/rust-lang/rust/blob/360f7d7b731c508c88b7b56921a92d281c05dbf7/library/windows_targets/src/lib.rs#L37-L41

@P-E-P P-E-P mentioned this pull request Oct 10, 2024
Open
P-E-P added 2 commits October 10, 2024 14:19
@P-E-P
Add a new CI for windows environments
6d69c81 
ChangeLog:

	* .github/workflows/ccpp.yml:

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
@P-E-P
Prevent configuration fail on windows
455efe6 
Windows platform do not have libpthread and libdl, they instead rely
on win32 to provide similar functionnality.

ChangeLog:

	* configure: Regenerate.
	* configure.ac: Do not error out when missing library for windows
	target.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
thesamesam
thesamesam reviewed Oct 10, 2024
View reviewed changes
Copy link
Contributor

@thesamesam thesamesam left a comment

Choose a reason for hiding this comment

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

Looks good otherwise from that nit.

configure.ac Outdated
AC_SEARCH_LIBS([pthread_create], [pthread])
CRAB1_LIBS="$LIBS"
LIBS="$save_LIBS"
;;
Copy link
Contributor

Choose a reason for hiding this comment

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

The ;; indentation is inconsistent with the block above.

Copy link
Member Author

@P-E-P P-E-P Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't too sure how to indent that. Should the semicolon be aligned with the case ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, either style is fine (people do a mix) but just have it be consistent. I personally have it aligned with the last statement inside the case usually (so the 1st one).

GCC seems to usually do that too (first variant).

@P-E-P P-E-P force-pushed the add_windows_ci branch 2 times, most recently from 7a47638 to e656602 Compare October 10, 2024 15:25
thesamesam
thesamesam reviewed Oct 10, 2024
View reviewed changes
configure.ac Outdated
LIBS="$save_LIBS"
case "${target}" in
*-*-mingw*)
CRAB1_LIBS="-lkernel32 -luser32 -lmsvcrt -lmingwex -lmingw32 -lmingwex -lws2_32"
Copy link
Contributor

Choose a reason for hiding this comment

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

This line uses spaces while the line below use tabs.

P-E-P added 2 commits October 14, 2024 13:10
@P-E-P
[DME]: Temp. disable 32 bit target.
d1a3697
@P-E-P
Link with win32 API library on windows
8aaf6a9 
ChangeLog:

	* configure: Regenerate.
	* configure.ac: Link with multiple libs on
	windows.

Signed-off-by: Pierre-Emmanuel Patry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MehdiChinoune MehdiChinoune MehdiChinoune left review comments

@thesamesam thesamesam thesamesam left review comments

Assignees
No one assigned
Labels
CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@P-E-P @MehdiChinoune @mati865 @thesamesam