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

port of STP to the visual studio 2012 c-compiler #64

Closed
wants to merge 29 commits into from

Conversation

fmerz
Copy link
Contributor

@fmerz fmerz commented Dec 10, 2013

STP currently doesn't build with visual studio's 2012 c-compiler. These are all the necessary changes to make it work on my machine. Your mileage my vary.

when setting the CMAKE_*_OUTPUT_DIRECTORY. These are only used in install
commands. We want to install to the system, not to the build directory.

This commit also affects the unix build. It should always make sense,
unless cmake's install feature is abused in strange ways...
MSVC allows adding comments to object files via "#pragma comment(...)".
This feature also allows to add comments to the object files that tell
the linker which libraries to link in additionally, whenever such an
object file is linked into a library.
Boost uses this in its headers. Any code using boost headers that require
boost libraries therefore will fail when linked if the library cannot be
found. STP's build system already takes care of linking boost, so this is
not needed in our case. If this automatic linking fails for some reason
it is not readily apparent why this failed.

This patch tells boost not to insert these pragmas into the object files.
MSVC seems to require this.
if this is not added the lexer will generate a call to isatty(), which is
not supported on windows.

Not entirely sure if this is good for unix? An alternative solution would
be to add a always-false isatty() somewhere for windows.
instead of hardcoding it in the script.

Affects unix, too, but should be good.
if I understand the code correctly this is just an optimization. Should
be safe not to do this at all on windows.
and can likely not be fixed cleanly. This is a workaround, though not a
particularly good one.
though I haven't tried making it work for very long.
MSVC seems to require variable declarations at the beginning of functions.
MSVC doesn't like this.
variable length arrys were only added for C99. Support is optional for C11.
At least MSVC knows alloca, though this is also not supported by all
compilers.
log2 is not provided by MSVC.
in various headers in the cryptominisat2 directory. msvc/ is a folder local
to cryptominisat. This implementation of stdint.h is necessary because
MSVC doesn't provide this header at all.

Note that there is another implementaion of stdin.h in STP's windows/
directory. I have no idea what happens if these differ.
though this isn't used by stp. Could also comment the code out as an
alternative solution.
MSVC gets confused by this. Renamed it to _free. Not a good choice either,
but it solves the problem...
this is non-trivial to port to windows. If you need this you're on your
own...
so we need to comment this out. Now we can't have timeouts on windows. If
you need this do it yourself ;-)
but fortunately these irritatingly strange defines fix this problem for
MSVC.
so we include compdep.h, which typedefs it to stroui64. Good enough for us.
the yy stuff is broken. If you need this, fix it yourself.
this is odd: On the one hand, inline is not a keyword in visual studio so
it doesn't support it. On the other hand it doesn't allow you to redefine
it because it is a keyword. Fortunately we can just tell visual studio to
allow keyword macros and it shuts up about this.
this is gcc specific and doesn't work with msvc. Its used like
- __attribute__((noreturn)), which is safe to ignore, and
- __attribute_-((mode(TI)), which messes with the bitwidths of the types.

The second one is suspicious and might not be safe to ignore. No idea if
this is necessary on windows, too, or if this is safe to ignore. Let's just
do it and keep it in mind if STP on windows behaves strangly.
uncomment these lines to actually disable the warnings. There's a lot of
these, so you might actually want to do this...
this slipped in without the necessary ifdef.
@fmerz
Copy link
Contributor Author

fmerz commented Dec 10, 2013

The pull request breaks Travis. I believe d5b9335' is the culprit. Not sure how this went wrong. STP seems to use CMake in strange ways. I'll leave my hands off this.

fmerz and others added 3 commits December 11, 2013 11:11
and added comment that
CMAKE_ARCHIVE_OUTPUT_DIRECTORY and CMAKE_LIBRARY_OUTPUT_DIRECTORY
are build locations **not** install locations.
@delcypher
Copy link
Member

@fmerz Thanks for looking at this, I'm not a Windows person but it's nice to see some trying to make this work. I'm not that very familiar with STP's code base so a lot of stuff I can't really comment on. The only bit I've really worked on is the CMake build system. I also don't have commit access so I can't push any of these changes.

I'm not quite sure what to do about the merge static libraries issue. I don't have time to properly look at it right now but I will look at this at some point.

If we can get the commits in a state where nothing gets broken and we have a clean history (use git rebase --interactive and then do a forced push to this branch) then hopefully @rgov or @msoos can commit it.

@fmerz
Copy link
Contributor Author

fmerz commented Dec 11, 2013

I'm not a Windows person either so I can't give any guarantees...

I'll do the rebase though I'd rather push the result to a separate branch and send another pull request than force the push on this branch.

@delcypher
Copy link
Member

@fmerz You don't have to do any rebasing yet.

It's just a suggestion of something to do once everyone is happy with commits (we're not really there yet) so we can have a better history (e.g. not having d0e4ec0 or d5b9335 is the history).

@fmerz
Copy link
Contributor Author

fmerz commented Dec 11, 2013

There's a cleaned up branch at https://github.com/fmerz/stp/tree/windows_clean. Mind having a look at it?

@delcypher
Copy link
Member

@fmerz I've had a quick look. I've left a few comments. As I said before I only really know STP's build system and not how the code itself actually works. So I'd rather leave @rgov @msoos @khooyp @hellovijay to take a look to check this is okay.

@delcypher
Copy link
Member

@fmerz I've removed MERGE_STATIC_LIBRARIES in #70 . You should try your port again and see how well things work.

@fmerz fmerz closed this May 22, 2014
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.

None yet

2 participants