Initial commit of CMake build system #992

dnakamura · 2017-04-07T21:16:39Z

Adds experimental support for building with cmake

Issue: #933
Signed-off-by: Robert Young [email protected]
Signed-off-by: Devin Nakamura [email protected]

rwy7 · 2017-04-07T21:58:09Z

If you take a look at the CI, you'll see some changes to the way things are being built. The big one is an experimental build with CMake on x86-64, which is allowed to fail. We've also moved to the "Trusty" Ubuntu distribution. Since the OMRChecker isn't building on Trusty (yet), there is a new build on Precise for running the linter.

As well, there is a new Appveyor configuration using CMake.

mgaudet · 2017-04-08T15:12:00Z

Would you be able to send me a link to one of the logs that failed with the linter on Trusty so I can get a fix in?

Leonardo2718 · 2017-04-08T15:18:16Z

fvtest/CMakeLists.txt

+add_subdirectory(omrGtestGlue)
+add_subdirectory(algotest)
+# add_subdirectory(compilertest)
+add_subdirectory(gctest)


Would it be possible to also add jitbuildertest to the list?

Yep! We can't build the test yet, but we can leave a note.

rwy7 · 2017-04-08T17:21:34Z

@mgaudet here is a link to a failed Trusty linter build.

The clang and llvm versions are output in the build-on-travis.sh script:

+llvm-config --version
3.5.0
+clang++ --version
clang version 3.5.0 (tags/RELEASE_350/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
+make lint

fjeremic · 2017-04-10T00:51:52Z

omrversionstrings.CMakeTemplate.h

@@ -0,0 +1,29 @@
+/*******************************************************************************
+ *
+ * (c) Copyright IBM Corp. 2015


This needs a copyright update. Is this a tool-generated file?

This is a written file that is used to generate omrversionstrings.h. Will update the copyright.

rwy7 · 2017-04-12T17:31:22Z

This is good to go.

fjeremic · 2017-04-12T17:37:38Z

General question: Are the two build systems going to be running concurrently for a while?

charliegracie

I think this is a great starting point. Thanks! This will likely need an Eclipse CQ before it gets merged.

dnakamura · 2017-04-12T18:00:05Z

@fjeremic Yes the two build systems will run side by side for the time being. This pr adds an allowed failure cmake build to the travis config.

fjeremic · 2017-04-18T15:42:46Z

Anything blocking this to be merged? Are we waiting for additional reviews from @youngar and @mstoodle?

dnakamura · 2017-04-18T16:11:24Z

@fjeremic It's my understanding that we are waiting on @mstoodle and the eclipse CQ process

mstoodle · 2017-04-18T16:18:21Z

Thanks for the extra @ mention, I had missed the original request to review. Will review by EOD.

mstoodle · 2017-04-18T16:19:18Z

First observation: the commit message is pretty terse :) .

mstoodle

Two general comments (which you'll find embedded in the review comments as well):

Please make sure every file has a correct copyright header. Please make sure every new file is marked copyright 2017, and please update the "rightmost" copyright date in every existing file to 2017.
There are many blocks of commented out statements in this commit. Some have clear TODO comments, some have unclear TODO comments, and many have no comment to explain why the statements are commented out. You'll probably never understand these blocks of statements better than you do right now, so please write reminders to yourself/informative descriptions for future developers who happen across it. If the code is just vestigial stuff from trying to get this working, then please remove those statements completely to avoid confusion.

It seems like a lot of comments to work through, but those two categories probably capture at least 95% of the spots I marked.

mstoodle · 2017-04-18T16:20:03Z

.appveyor.yml

@@ -0,0 +1,36 @@
+###############################################################################
+#
+# (c) Copyright IBM Corp. 2016


new file, we're 4 months into 2017 already :)

mstoodle · 2017-04-18T16:21:12Z

.gitignore

@@ -16,6 +16,15 @@
 #    Multiple authors (IBM Corp.) - initial implementation and documentation
 ###############################################################################


general comment: modified files should have their "rightmost" copyright date updated to the current year

mstoodle · 2017-04-18T16:23:42Z

CMakeLists.txt

@@ -0,0 +1,115 @@
+###############################################################################
+#
+# (c) Copyright IBM Corp. 2016


should be 2017

mstoodle · 2017-04-18T16:25:07Z

CMakeLists.txt

+
+cmake_minimum_required(VERSION 3.2 FATAL_ERROR)
+
+set(OMR_VERSION_MAJOR 0)


would it be better for this version identification piece to live in a separate file, isolated from the rest of the cmake machinery?

It would appear this is how LLVM does it. I find they are usually a good standard for accepted practice.
https://github.com/llvm-mirror/llvm/blob/master/CMakeLists.txt

Also, now that I think about it, this is really more of a place holder. Ideally we need figure out a system where we can store version information once and consume in both build systems

mstoodle · 2017-04-18T16:28:34Z

cmake/config.cmake

@@ -0,0 +1,102 @@
+


is this a generated file? is that why there is no copyright header?

mstoodle · 2017-04-18T16:55:14Z

cmake/platform.cmake

+
+	set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${linker_common} /INCREMENTAL:NO /NOLOGO /LARGEADDRESSAWARE wsetargv.obj")
+	if(OMR_ENV_DATA64)
+		#TODO: makefile has this but it seems to break shit


please remove the profanity

mstoodle · 2017-04-18T16:55:32Z

cmake/platform.cmake

+	else()
+		set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -entry:_DllMainCRTStartup@12")
+	endif()
+	#set(CMAKE_C_FLAGS "${common_flags}")


please document why commented out

mstoodle · 2017-04-18T16:56:00Z

cmake/platform.cmake

+
+	# # This is the first option applied to the C++ linking command.
+	# # It is not applied to the C linking command.
+	# OMR_MK_CXXLINKFLAGS=-Wc,"langlvl(extended)" -+


please document why commented out

mstoodle · 2017-04-18T16:56:41Z

fvtest/gctest/CMakeLists.txt

+	omrutil
+	omrglue
+)
+#	./omrgctest -configListFile=fvtest/gctest/configuration/fvConfigListFile.txt


please document why these lines are commented out

mstoodle · 2017-04-18T16:57:14Z

fvtest/omrGtestGlue/CMakeLists.txt

+
+target_link_libraries(omrGtestGlue INTERFACE omrGtest)
+
+#TODO there is platform specific stuff here


doesn't sound like an action to me, could you please make this comment more clear?

mstoodle · 2017-04-18T17:04:39Z

Lest it be lost in the massive dump of review comments, I think this PR represents some amazing work, and I'm really happy to see it. Thanks @dnakamura and @rwy0717 for putting it all together!

mstoodle · 2017-04-18T17:05:46Z

Just curious, what motivated the switch to use Ubuntu Trusty?

dnakamura · 2017-04-18T17:15:46Z

@mstoodle thanks for the feedback

Most of the commented out blocks are chunks of the source makefile which have not yet been ported over (will update to reflect this)
If I recall correctly the main reason to move to trusty was that precise has some horridly out of date version of cmake (2.8.7)

dnakamura · 2017-04-18T18:30:12Z

@mstoodle I've addressed the comments you have made. I've left it as a separate commit in order to make it easier to track changes, but will squash it done before we merge in

0xdaryl · 2017-04-18T19:00:08Z

@marti4d : FYI, you may want to take a peek at this

mstoodle

much better, thanks @dnakamura ! only one remaining comment to fix, please.

mstoodle · 2017-04-18T19:16:09Z

thread/CMakeLists.txt

@@ -42,6 +42,7 @@ list(APPEND OBJECTS
 	rwmutex.c
 )

+#TODO need to add te


te? either a typo or please explain a bit more

dnakamura · 2017-04-18T20:35:52Z

@mstoodle should be good to go

youngar · 2017-04-19T19:02:59Z

I opened an Eclipse CQ for this pull request here. I'll update here with any CQ status updates.

Adds experimental support for building with cmake. To test the cmake build system: ```sh mkdir -p build/ && cd build cmake .. cmake --build . ctest ``` The cmake build system is incomplete. Currently building: * gc * omr (core/misc) * fvtest * thread * port Not yet done: * jitbuilder * Compiler Issue: eclipse-omr#933 Also-by: Robert Young <[email protected]> Signed-off-by: Robert Young <[email protected]> Signed-off-by: Devin Nakamura <[email protected]>

rwy7 · 2017-04-19T19:13:08Z

I've rebased against latest, and added a new tracegen definition file, j9hook.tdf, to the build. The cmake build should be passing again.

youngar · 2017-04-20T23:34:31Z

The Eclipse CQ has received "PMC approval", but we are still waiting for a preliminary legal review before we can check in this code. I'm guessing the review will be completed sometime tomorrow. You can read more about this process here.

charliegracie · 2017-04-24T15:04:57Z

The CQ has not been approved for release yet. I will keep monitoring it

charliegracie · 2017-04-25T14:36:07Z

CQ has been completely approved :).

charliegracie · 2017-04-25T14:39:06Z

Discussed with @youngar that if it got approved while he was away I would handle merging.

dnakamura force-pushed the cmake-final branch 3 times, most recently from 67fa8cb to e9e2cc8 Compare April 7, 2017 21:21

Leonardo2718 reviewed Apr 8, 2017

View reviewed changes

fjeremic reviewed Apr 10, 2017

View reviewed changes

dnakamura force-pushed the cmake-final branch 3 times, most recently from f3a0d8e to 17e10f0 Compare April 10, 2017 15:42

charliegracie added build / configure discussion labels Apr 10, 2017

dnakamura force-pushed the cmake-final branch 2 times, most recently from a222017 to 8797117 Compare April 12, 2017 16:38

charliegracie requested review from youngar and mstoodle April 12, 2017 17:35

charliegracie assigned charliegracie and youngar and unassigned charliegracie Apr 12, 2017

charliegracie self-requested a review April 12, 2017 17:37

charliegracie approved these changes Apr 12, 2017

View reviewed changes

mstoodle requested changes Apr 18, 2017

View reviewed changes

mstoodle approved these changes Apr 19, 2017

View reviewed changes

dnakamura force-pushed the cmake-final branch from b8cd2b0 to a69e2aa Compare April 19, 2017 18:34

rwy7 force-pushed the cmake-final branch from a69e2aa to 3488ac3 Compare April 19, 2017 18:56

youngar approved these changes Apr 19, 2017

View reviewed changes

rwy7 force-pushed the cmake-final branch from 3488ac3 to bab4f1a Compare April 19, 2017 19:09

rwy7 force-pushed the cmake-final branch from bab4f1a to 389f1e3 Compare April 19, 2017 19:10

charliegracie self-assigned this Apr 25, 2017

charliegracie merged commit 17ef73f into eclipse-omr:master Apr 25, 2017

dnakamura deleted the cmake-final branch August 2, 2017 16:24

		@@ -16,6 +16,15 @@
		# Multiple authors (IBM Corp.) - initial implementation and documentation
		###############################################################################


		cmake_minimum_required(VERSION 3.2 FATAL_ERROR)

		set(OMR_VERSION_MAJOR 0)


		target_link_libraries(omrGtestGlue INTERFACE omrGtest)

		#TODO there is platform specific stuff here

Initial commit of CMake build system #992

Initial commit of CMake build system #992

Conversation

dnakamura commented Apr 7, 2017 • edited Loading

rwy7 commented Apr 7, 2017

mgaudet commented Apr 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwy7 commented Apr 8, 2017

fjeremic Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwy7 commented Apr 12, 2017

fjeremic commented Apr 12, 2017

charliegracie left a comment

Choose a reason for hiding this comment

dnakamura commented Apr 12, 2017

fjeremic commented Apr 18, 2017

dnakamura commented Apr 18, 2017

mstoodle commented Apr 18, 2017

mstoodle commented Apr 18, 2017

mstoodle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle commented Apr 18, 2017

mstoodle commented Apr 18, 2017

dnakamura commented Apr 18, 2017

dnakamura commented Apr 18, 2017

0xdaryl commented Apr 18, 2017

mstoodle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnakamura commented Apr 18, 2017

youngar commented Apr 19, 2017

rwy7 commented Apr 19, 2017

youngar commented Apr 20, 2017

charliegracie commented Apr 24, 2017

charliegracie commented Apr 25, 2017

charliegracie commented Apr 25, 2017

dnakamura commented Apr 7, 2017 •

edited

Loading

fjeremic Apr 10, 2017 •

edited

Loading