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 build and test. #303

Merged
merged 12 commits into from
Oct 11, 2017
Merged

CMake build and test. #303

merged 12 commits into from
Oct 11, 2017

Conversation

spencels
Copy link
Contributor

@spencels spencels commented Feb 27, 2017

Add support for building and testing with CMake. This is necessary for future MSVC support (#300).

Not supported yet:

  • Windows
  • Emscripten
  • Python
  • Compilers other than GCC and Clang.

Not supported yet:
* Windows
* Emscripten
* Compilers other than GCC and Clang.
@@ -0,0 +1,67 @@
# libjsonnet

# Remember to update Bazel and Makefile builds when updating this list!
Copy link
Contributor

Choose a reason for hiding this comment

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

and MANIFEST.in and setup.py :(

@sparkprime
Copy link
Contributor

This looks ok, except it seems your build test is failing on mac?

I'd like to try it out myself, but haven't got time right now.

In particular, is there enough feature parity to remove the handwritten makefiles...

@sparkprime
Copy link
Contributor

Oh there is emscripten, as you said.

@spencels
Copy link
Contributor Author

CMake could easily take over everything the handwritten makefile does now. The Python build be a CMake target, where CMake generates setup.py/MANIFEST.in via a template using the LIB_SOURCES variable in core/CMakeLists.txt. I believe there are CMake libraries out there for finding Python installations. I'm pretty sure CMake could also do the Emscripten build as well, but I'm less certain on how that would work.

I'd prefer to implement Python and Emscripten builds in a follow-up PR to keep the size manageable, but I could build it into this one if you'd like.

I think Travis is failing on Linux. I developed this on a Mac so at least the Mac build is green :). I have access to all three systems so I will troubleshoot it when I get a chance.

* Download googletest at CMake generation time. This makes linking
simpler and should fix the Linux Travis build.

* Build the jsonnet standard library header (std.jsonnet.h) via CMake.
Doing this in a cross-platform fashion required a small C++ utility to
generate the code.
@spencels
Copy link
Contributor Author

spencels commented Mar 1, 2017

I fixed the issue with Linux, Travis should work now (I'm not sure why it hasn't re-run). I also added a cross-platform std.jsonnet.h generator.

I noticed some functionality in run_tests.sh. I think I can implement that in CMake. Shell scripts won't run on Windows so I'll want to port that eventually.

CMake will compile targets without header files listed as files, but it won't recompile when changes are made unless the files are listed as dependencies.
@sparkprime
Copy link
Contributor

There is one script that makes sure jsonnet outputs "true" (or matches the golden file if provided) and another script that makes sure jsonnet fmt does not change the jsonnet file (or matches the golden file if provided).

@sparkprime
Copy link
Contributor

Hey, should I have merged this?

@spencels
Copy link
Contributor Author

Yes, I think this was ready to go out. I'll resolve merge conflicts and give it a quick test in the next week and let you know when it's ready.

@sparkprime
Copy link
Contributor

Awesome, thanks for coming back to it after all this time.

@spencels
Copy link
Contributor Author

spencels commented Oct 6, 2017

The code looks good overall, but tests are failing after the pulling in 8 months-worth of changes. I'll ping this thread when I have it working again.

@spencels
Copy link
Contributor Author

spencels commented Oct 6, 2017

The error was on my end. This is ready to go.

@sparkprime sparkprime merged commit 30f4739 into google:master Oct 11, 2017
@sparkprime
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants