Refactor the PAL build configuration to detect at configure time

[kangaroo]

The current PAL has binary check-ins of the config.h for Linux and MAC, long
term this is not sustainable. This is a first pass at cleaning this up.
Please note, there are some static platform independent defines as I tried to
keep consistency with the current outputs of the not-checked-in configure.in.
As we clean up the PAL the CMakeLists should be revisited and redundant pieces
removed.

[dnfclas]

Hi @kangaroo, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[janvorli]

Thanks you very much for making this change! It is great that it can be done using the cmake.
One thing I'd like to change though is to move all of the config stuff into a separate configure.cmake file that would be just included from the root CMakeLists.txt.

[kangaroo]

So the linux build is failing because the check_type_size and check_struct_has_member calls are building with C and clang -std=c++ doesn't set the linux flag needed to pull in stack_t.

CMake 3 fixes this by adding a [LANGUAGE CXX] footer to those two calls, but its not directly available in ubuntu 14.04, we'd need to find a ppa.

Alternately we could bring in the CheckTypeSize.cmake and CheckStructHasMember.cmake files from cmake3 to support this, or look at some other way of making these checks pass (Add -D_XOPEN_SOURCE?)

Thoughts?

[janvorli]

Bringing the sources from the CMake 3.0 sounds like at least a delaying factor due to a needed legal approval. If we can add the -D_XOPEN_SOURCE flag and it works as expected, it would seem like a good workaround.
Yet another alternative way might be to force the C++ compiler for .c files before the tests using
set( CMAKE_C_COMPILER "${CMAKE_CXX_COMPILER}" )
set( CMAKE_C_COMPILE_OBJECT "${CMAKE_CXX_COMPILE_OBJECT}" )
and possibly CMAKE_C_LINK_EXECUTABLE / CMAKE_CXX_LINK_EXECUTABLE too.
And restoring them after.

[kangaroo]

To clarify, we don't need to import cmake source code, just the following two .cmake files:

CheckStructHasMember.cmake
CheckTypeSize.cmake

They wouldn't get included in any build output. Is that still a legal challenge? If so I'll investigate other options.

[janvorli]

It seems to me that the fact that we would include these scripts would be seen the same way as including other sources. @sergiy-k What do you think?

[kangaroo]

@janvorli I ended up just fixing it to not require that, less sticky that way.

[janvorli]

LGTM
@sergiy-k Could you please take a look too?

[kangaroo]

1 mistake with ungetc tests on linux, so I just repushed

[janvorli]

@kangaroo Is there a way to see diff between the two changes on github? I was told there is, but I cannot find it.

[shahid-pk]

@janvorli
This is small read for just that i guess
https://help.github.com/articles/differences-between-commit-views/

[janvorli]

Thanks @shahid-pk. I was looking for something else though. @kangaroo said: "1 mistake with ungetc tests on linux, so I just repushed", so I wanted to see the diff between those two iteration (before and after he has repushed).

[kangaroo]

@janvorli I squashed it so it can't be seen anymore. Seems there is still a failure tho. I'll take a look in a bit

[sergiy-k]

Guys, I’m sorry for the delay with a response. I wanted to double check a few things first. Including these cmake files is OK if: a) their content does not show up in the binaries that we will distribute (which should be OK in this case); b) we don’t check them in into our source (checking in OSS code requires special approval).

Making CMake 3 a requirement is an option. As a matter of fact, we want to make CMake requirements to be the same across all platforms (for example, upgrade required version in our cmake files to 3.0.2 which is the version that we currently support on Windows). There is a binary distribution of cmake package which we can manually install on our Linux machines (I have just installed the latest version on Ubuntu 14.04 and it seems to work). Personally, I don’t like that we cannot use ‘apt-get’ to do the upgrade automatically but most people that I’ve talked to don’t think that downloading and installing a binary package manually would not be a problem. What’s your opinion?

[sergiy-k]

LGTM. Thanks a lot for this!

[kangaroo]

@sergiy-k I managed to work around it with CMake 2.8 for now. I think we should jump to 3, when we have a need to in the future tho.

[sergiy-k]

@kangaroo I'm curious what did you have to do to make it work? I wish I could diff your your changes.

[kangaroo]

@sergiy-k Basically what I said to @janvorli. Long story short, check_struct_has_member and check_type_size default to using the C compiler, and go to std=c11 mode. When in std=c11 mode, stack_t for instance isn't defined, leading to a compiler error when probing for ucontext_t. When we're in std=c++11 mode it works.

This is because of how Linux gates things on STD modes. The work around was simple enough, if you see:

set(CMAKE_REQUIRED_DEFINITIONS "-D_DEFAULT_SOURCE")
...
set(CMAKE_REQUIRED_DEFINITIONS)

Make sense?

[sergiy-k]

@kangaroo Thank you.

[kangaroo]

@sergiy-k in the future would you rather I didn't rebase and squash commits from the original PR? Do we want a PR to end up as a single merge, or have the review history encapsulated?

[richlander]

This is what @terrajobst had to say about that: https://twitter.com/terrajobst/status/537004701124796416.

1 is certainly fine if it's meaningful.

[kangaroo]

@richlander While I agree with the sentiment, its very 'open to interpretation' from an implementors perspective. I felt in this case I felt that the 'response to feedback' was noise. Cleary @sergiy-k appreciated the context from a review perspective. So I have a simple suggestion (as a policy):

• Incoming PRs should not be squashed so long as feedback remains unresolved
• After all concerns are addressed the framework team should advise for a squash or not

Basically this boils down to:

"No squash until merge sign-off, and the core team can decide if squash is needed before merge to preserve signal:noise"

Thoughts?

[richlander]

Ya, I think that's right. In most cases, this is what is already happening. The folks in corefx land now have a bit more experience are requesting squashes just before merge. coreclr folks are not quite there yet, but will end up there.

@terrajobst, @jkotas How's that sound?

Nice writeup. We might just run with it.

[kangaroo]

Super happy to defer to corefx team on this. I'm jumping in late to the overall game.

[jkotas]

Sounds good to me.

[terrajobst]

"No squash until merge sign-off, and the core team can decide if squash is needed before merge to preserve signal:noise"

👍