-
-
Notifications
You must be signed in to change notification settings - Fork 621
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 building Premake using mingw #1111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, if nobody speaks up in by the end of the week, we should just merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me…I don't have MinGW installed here to test.
Well, I installed both MinGW32 and 64 and it works with MinGW64. MinGW32 has an error related to curl if I remember correctly. I expected a review on this part:
I think it is pretty safe to assume that Makefiles on Windows are used with MinGW but just wanted to confirm it. |
Could be Cygwin too, but I would still expect those libraries to be present. I think this should be okay. |
I though Cygwin used MinGW, I'm really confused about all these things. Let me test Cygwin to be sure; If everything works fine, I will merge this myself if that's ok with you. |
Well, I can't build curl with the cygwin gcc but I think it is another issue than what this PR does. I think this is something for curl to fix and not us. I also have an issue with mingw64 gcc in cygwin. Here is the log:
Not sure what cause this, I will look into it some more. |
Well, it seems to be an issue on cygwin side. If someone want to build with MinGW, they should get MinGW not from cygwin. If someone want to look into cygwin, I had to add change premake-core/src/host/premake.h Line 20 in d6dfaa3
to
Anyway, that still fix an issue so I'm inclined to merge as is. I will merge this tomorrow to let the opportunity for people to disagree. |
Disregard this... I just forgot to add PLATFORM=x64 so it was building and looking for 32bit libraries. I just added a fix in os_stat for MinGW32 if the curl problem is ever fixed and the Cygwin identification. I tried to build with Cygwin for a correctly (with the PLATFORM=x64) and I currently have a seg fault on linking. Otherwise everything build. |
I'd like to think that's not a reasonable assumption; msc should work with makefiles too, but it's definitely not used often. |
contrib/libzip/premake5.lua
Outdated
@@ -16,6 +16,8 @@ project "zip-lib" | |||
|
|||
filter "system:windows" | |||
defines { "_WINDOWS" } | |||
filter { "system:windows", "action:gmake**" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't like this strategy... I feel like mingw/cygwin might be systems, but I'm not entirely sure :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like mingw/cygwin might be systems
I disagree, is there anything stopping you from using either of these to target Android or Linux? I'd be more inclined to say that these should fall under toolset
, or a subtype of toolset
/action
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe... the distinction between mingw/cygwin and 'windows' is that they have a different runtime, different ABI, different libs. They are definitely a different platform than 'windows' (assuming an MSVC windows world).
I have often wanted to generate a makefile that uses MSVC because I want to share the same build system across platforms. I don't usually do it, but I feel like it should be possible to produce a makefile that supports the normal 'windows' ecosystem, in addition to mingw/cygwin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah nar. It's definitely not a toolset thinking on it. Mingw/cygwin are undeniably different platforms... or 'systems'?
Building mingw from windows is no different than building android from windows. The entire pipeline is incompatible with 'windows' by premakes conventional assumptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't know enough about either of them to push for anything in particular, they just don't seem like system
s to me.
src/host/premake.h
Outdated
@@ -17,7 +17,7 @@ | |||
|
|||
/* Identify the current platform I'm not sure how to reliably detect | |||
* Windows but since it is the most common I use it as the default */ | |||
#if defined(__linux__) | |||
#if defined(__linux__) || defined(__CYGWIN__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want PLATFORM_STRING
to be "linux"
under cygwin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cygwin == linux is kinda like saying bsd == linux no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't... but the problem for example is that curl uses autoconf to generate most of it config header files. Since premake does not have such a facility, we pre-generated these header files. and checked them in.. and well, obviously they are wrong for some platforms. That's why for example mingw32 doesn't seem to work, because most of those header files have been generated on 64-bit linux variants....
it's a common recurring issue with 3rd party libraries that do this kind of thing, and we basically have to hand craft a header file that satisfies all platforms. It's why I created an autoconf module to try and somewhat remedy this. It was mostly a prototype proof of concept, and it's certainly not impossible to add such facilities to premake. I just don't like it. I feel like if you have to resort to autoconf tools, your doing something wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why for example mingw32 doesn't seem to work
Not sure this is the cause but I should check it out. I think I saw a GitHub issue somewhere where someone had the same issue and the cause was MinGW32.
Why do you want
PLATFORM_STRING
to be"linux"
under cygwin?
Building with Cygwin uses posix libraries. But this is an awful solution and should absolutely not be merged into master.
I had second though about this PR yesterday and definitely want to take another look at it now.
One thing to consider is that Premake generated Makefiles cannot be used with NMake. So we would be talking about using a MinGW/Cygwin makefile binary to run a msc Makefile. However, I agree that it's a somewhat bold assumption but I didn't see an alternative. |
That's entirely feasible. Although supporting nmake is something I'd like to clean up too one day! Yeah, I'm not happy with the assumption. It's rarely used, so I have no doubt that when it's committed, it'll 'stick', and it'll be that way forever :P |
Finally some time to look into this. After some thoughts, I'd say that MinGW should be handled as a toolset. In the same way you can build either with gcc or clang on Linux, you should be able to build with cl or MinGW on Windows with a Makefile. Well not really in the same way as MinGW and cl can't use the same options like gcc and clang does. This way, the bootstrap can just use |
So, the issue with MinGW32 come from luasocket.
The related github issue I was talking about last time: curl/curl#2361. I'm giving up on MinGW32, so I think this PR is complete as is, just waiting on an approval (especially from @TurkeyMan). If someone ever take up to make MinGW32 work, there is an issue with how the I also removed the Cygwin change as it was bothering me and I feel (if someone ever want to make Cygwin work) that it should be a system so should be on a separate PR. |
@tdesveauxPKFX @TurkeyMan are we good to merge this? |
I rebased this to master, made some additional fixes, and created PR #1420. It isn't perfect, but it does provide a build path for MinGW users. Thanks to @tdesveaux for doing the hard parts. |
Only works with MinGW64 due to an error in luasocket.
I'd appreciate if someone that have used MinGW before could take a look.