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

CMake support for Windows #322

Merged
merged 35 commits into from Sep 29, 2019

Conversation

@elliottslaughter
Copy link
Collaborator

commented Nov 22, 2018

This pull request adds (or will add, it isn't quite there yet) Windows support for the CMake build.

At the moment it fails because some of the dependencies aren't being set up correctly.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

I'm hoping to solicit some help from people familiar with CMake. (CC: @OvermindDL1 @Clyybber) For some reason the dependencies in the LuaJIT build aren't being properly respected, and I can't for the life of me figure out what's wrong.

The structure is basically:

add_custom_command(
    OUTPUT ${LUAJIT_EXECUTABLE}
    COMMAND ...
)
add_custom_command(
      OUTPUT "${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME}"
      DEPENDS ${LUAJIT_EXECUTABLE}
      COMMAND ...
)
add_custom_target(
  LuaJIT
  DEPENDS
    ${LUAJIT_EXECUTABLE}
    "${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME}"
)
add_custom_command(
  OUTPUT ${PROJECT_BINARY_DIR}/clangpaths.h
  DEPENDS LuaJIT
  COMMAND ...
)
add_custom_target(
  TerraGeneratedFiles
  DEPENDS
    ${PROJECT_BINARY_DIR}/clangpaths.h
)

On the first build, LUAJIT_EXECUTABLE gets built, but not "${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME}". So the later command, which depends on the LuaJIT target, fails. But on my second build, the "${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME}" file gets created properly, and everything builds.

I understand there is some subtlety about what kind of things are allowed to depend on what, but I think I'm following all the rules here. At least I can't see anything wrong.

If you want to see the actual sources, you can find them here:

https://github.com/elliottslaughter/terra/blob/0e61b66e2d044dadbeba9c833b2bc13d400eda76/cmake/Modules/GetLuaJIT.cmake

https://github.com/elliottslaughter/terra/blob/0e61b66e2d044dadbeba9c833b2bc13d400eda76/src/CMakeLists.txt

Thanks in advance for any guidance.

@Clyybber

This comment has been minimized.

Copy link

commented Nov 23, 2018

This is really wierd. It looks correct to me. Perhaps we should ask some CMake guru's on stackoverflow or IRC?

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 24, 2018

StackOverflow post here: https://stackoverflow.com/q/53435714/188046

I've worked around it for the moment by issuing the problematic commands at configure time, which is ugly, but seems to get the job done. I would still like to know why the original version didn't work though, since I don't know when I might hit this in the future.

@OvermindDL1

This comment has been minimized.

Copy link

commented Nov 26, 2018

As I recall (it's been a long long time since I dealt with VS) Visual Studio has an oddity about how dependencies can be mapped, essentially it has two types of dependencies, and the 'main' dependency can only be a singular dependency, so I'd change this:

add_custom_target(
  LuaJIT
  DEPENDS
    ${LUAJIT_EXECUTABLE}
    "${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME}"
)

To this:

add_custom_target(
  LuaJIT
  DEPENDS "${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME}"
)

As ${LUAJIT_INSTALL_PREFIX}/lua/jit/${LUAJIT_SOURCE_NAME} depends on ${LUAJIT_EXECUTABLE} anyway (Visual Studio's "main" command dependency can only be a single dependency, not multiple, it's a VS annoyance...).

However, this whole luajit setup looks odd... Why not use the luadist luajit CMake setup at https://github.com/LuaDist/luajit instead?

@elliottslaughter elliottslaughter force-pushed the elliottslaughter:cmake-windows branch 3 times, most recently from 3a00fbc to 5d25c8b Aug 22, 2019
@elliottslaughter

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

Ok, got this to the point where it builds, links, and runs most of the test suite. (Currently 8 failing tests.)

@elliottslaughter elliottslaughter force-pushed the elliottslaughter:cmake-windows branch from 62d037c to 517ef5a Sep 24, 2019
@blackhole12

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

This is still failing all the Visual Studio tests, and it is failing each one for unique reasons. Visual Studio 2013 seems impressively broken in ways I cannot begin to fathom. Visual Studio 2015 can't decide if there's a linker error or if half the tests should just fail for absolutely no reason. I will have to attempt building this with Visual Studio 2019 to see which errors are not simply environment issues.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 24, 2019

For CMake, I'd be willing to drop Visual Studio 2013 support altogether and focus only on the more recent versions. I think we're stuck with 2015 for the moment since that was what LLVM was built with, but even there we could rebuild with 2017 or 2019 if it would make things easier.

@blackhole12

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Re: the LLVM build issue, I feel obligated to point out that vcpkg #387 can, in fact, build LLVM from source on windows, but it's LLVM 8. A proper vcpkg implementation would create dependencies on both LLVM and LuaJIT and build them from source using the correct triplets.

It would also be possible to statically compile the runtime instead of manually copying msvc140.dll, which you aren't supposed to do in the first place. If you want Terra to be portable, the correct way is to use /MT on everything, which removes the dependency entirely. I already figured out how to convince LLVM to compile this way, so I could use the same method for Terra.

@blackhole12

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

After I managed to recreate these tests on my machine, multiterra.t is actually working but has spurious output from LLD, so I've added it to the skip list.

dynlib.t explodes with over 350 unresolved references in a spectacular failure probably resulting from not linking something properly

All 5 other tests are a result of not being able to find std.t or parsing.t, which normally lie in the lib folder. For whatever reason, this build of terra doesn't seem to be searching through those folders and I have no idea why. Moving std.t and parsing.t to the include directory lets all these tests pass.

@elliottslaughter elliottslaughter merged commit b5b8826 into zdevito:master Sep 29, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@elliottslaughter elliottslaughter deleted the elliottslaughter:cmake-windows branch Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.