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 some build issues with mingw #1680

Merged
merged 4 commits into from
Aug 20, 2021
Merged

Fix some build issues with mingw #1680

merged 4 commits into from
Aug 20, 2021

Conversation

Biswa96
Copy link
Contributor

@Biswa96 Biswa96 commented Jul 29, 2021

What does this PR do?

Fixes some build issues with mingw

How does this PR change Premake's behavior?

None.

Are there any breaking changes? Will any existing behavior change?

Nope.

Anything else we should know?

Don't know what to add here.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

This fixes conflict of pollfd structure declaration with winsock2.h header file
@nickclark2016
Copy link
Member

Please follow the PR format provided.

@nickclark2016
Copy link
Member

May resolve #1675

@Lemiort
Copy link

Lemiort commented Jul 30, 2021

Please take a look at that patch, that is used in conan-center:
https://github.com/conan-io/conan-center-index/blob/41d4fbb8c218f9d938ccb8b9d888bff52002e62e/recipes/premake/5.x/patches/0001-5.0.0-alpha15-mingw.patch
It's quite simular, but u don't have changes in linking commands, and their patch doesn't contain changes in contrib/curl/lib/select.h

Warnings are:
  * Check _MSC_VER for MSVC specific pragma directive.
  * Use lowercase header names for case sensitive file system.
  * GetFileAttributesW returns DWORD type instead of int.
@Biswa96
Copy link
Contributor Author

Biswa96 commented Jul 30, 2021

Please take a look at that patch, that is used in conan-center

I am not familiar with that project and never used it.

It's quite similar, but u don't have changes in linking commands

This pull request does not change any linking commands. That patch file in conano-io adds -lcrypt32 but it has already been defined in premake5.lua file.

and their patch doesn't contain changes in contrib/curl/lib/select.h

Without that change, this error is shown in mingw gcc:

In file included from ../../contrib/curl/lib/connect.c:64:
../../contrib/curl/lib/select.h:48:8: error: redefinition of 'struct pollfd'
   48 | struct pollfd
      |        ^~~~~~
In file included from ../../contrib/curl/lib/curl_setup.h:273,
                 from ../../contrib/curl/lib/connect.c:23:
x86_64-w64-mingw32/include/winsock2.h:1185:16: note: originally defined here
 1185 | typedef struct pollfd {
      |                ^~~~~~

BTW, I am building premake5 using this recipe msys2/MINGW-packages#9245. Feel free to comment on any mistake there.

src/host/os_getversion.c Show resolved Hide resolved
src/host/os_isdir.c Show resolved Hide resolved
src/host/os_isdir.c Show resolved Hide resolved
src/host/os_uuid.c Show resolved Hide resolved
@nickclark2016
Copy link
Member

So based on comments, I think the course of action is to have curl updated in one commit, then fix the build issues in a follow on commit, then submit that as a single PR.

@Biswa96
Copy link
Contributor Author

Biswa96 commented Aug 1, 2021

Updating curl would be a huge change and out of scope of this pull request. I have edited curl in separate commit here.

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

I'm going to go ahead and approve this as is. I agree that it would be better to update Curl (and disagree that it is a "a huge change and out of scope of this pull request"), but the changes are fairly small and self-contained. If it buys us better MingGW support in the meantime it feels worth it. I'll hold off on merging for a bit though in case someone else wants to object.

@nickclark2016
Copy link
Member

I'll go ahead and approve, but I'll write an issue for updating curl so this isn't lost.

@nickclark2016 nickclark2016 mentioned this pull request Aug 3, 2021
@nickclark2016
Copy link
Member

Just gonna poke on this @starkos

@starkos starkos merged commit b3de107 into premake:master Aug 20, 2021
@starkos
Copy link
Member

starkos commented Aug 20, 2021

Thanks for the contribution!

@Biswa96 Biswa96 deleted the fix-mingw branch August 20, 2021 13:01
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