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

Feature/solaris2 #867

Merged
merged 3 commits into from Jan 22, 2017

Conversation

Projects
None yet
9 participants
@mropert
Contributor

mropert commented Jan 16, 2017

(almost) Complete Solaris / SunStudio support.
Fixes unit tests and adapts generators.

Only one failing unit test (see below)

mropert added some commits Jan 13, 2017

Enhanced support for Solaris
Unified 32/64 support on x86
Added more unit tests on arch/library settings
@CLAassistant

This comment has been minimized.

CLAassistant commented Jan 16, 2017

CLA assistant check
All committers have signed the CLA.

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 16, 2017

@madduci, @memsharded,
As said in the PR only one test fails: conan.conans.test.integration.diamond_test
This is due to the fact that the "gcc" builder expects the compiler to work with CFLAGS and the like in environment variables.
This is not the case with SunStudio (sun-cc) which ignores all environment variable except LD_LIBRARY_FLAGS.

I didn't see a proper way to adapt this cleanly unless the ConfigureEnvironment returns an object with the various flags as different variables (c_flags, cxx_flags, cpp_flags, ld_flags...) that are then used by conanfiles.
As this is not a trivial change I prefered to left it our for now.

A quicker way could be to disable this test if CONAN_COMPILER is set to "sun-cc" :)

@codecov-io

This comment has been minimized.

codecov-io commented Jan 16, 2017

Current coverage is 94.68% (diff: 74.30%)

Merging #867 into develop will decrease coverage by 0.12%

@@            develop       #867   diff @@
==========================================
  Files           236        236          
  Lines         17200      17295    +95   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          16307      16375    +68   
- Misses          893        920    +27   
  Partials          0          0          

Powered by Codecov. Last update 136f147...624ce49

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 16, 2017

@codecov-io, just when I thought I was actually improving the test coverage :(

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 16, 2017

Dont worry about the test coverage, the current threshold is too restrictive!

@memsharded memsharded added this to the 0.19 milestone Jan 17, 2017

@memsharded

This is excellent work, looks really good.

The main question is my ignorance about -m64 flag, and not breaking things to existing users because of making different builds due to subtle differences.

flags.extend(["-DCONAN_CXX_FLAGS=-m32",
"-DCONAN_SHARED_LINKER_FLAGS=-m32",
"-DCONAN_C_FLAGS=-m32"])
elif op_system == "Macos":
if arch == "x86_64":
flags.extend(["-DCONAN_CXX_FLAGS=-m64",

This comment has been minimized.

@memsharded

memsharded Jan 19, 2017

Contributor

quick question, those flags were not defined before, so sure they will not change behavior to some users? Is the "-m64" the default value if not specified?

This comment has been minimized.

@mropert

mropert Jan 19, 2017

Contributor

That depends on your platform, actually.
On BSD, Linux (and I suspect, Mac too), the compiler defaults to 64 bit binaries.
On Solaris, it defaults to 32 bit event on 64 bit systems.
But then again, it shouldn't hurt since if you ask for a x86_64 build you do want a 64 bit binary.

"-DCONAN_SHARED_LINKER_FLAGS=-m64",
"-DCONAN_C_FLAGS=-m64"])
elif op_system == "Macos":
if arch == "x86":

This comment has been minimized.

@memsharded

memsharded Jan 19, 2017

Contributor

So, before the DCMAKE_OSX_ARCHITECTURES was set to i386, always, irrespective of architecture. It seemed to work fine in 64bits OSX. Now, nothing will be defined: Are you sure this won't be a problem?

This comment has been minimized.

@mropert

mropert Jan 19, 2017

Contributor

If I read the old code well, it added the -DCMAKE_OSX_ARCHITECTURES=i386 flag only if arch == "x86" and op_system == "Macos".
My version still does the same, just that instead of having a first switch by CPU arch then by OS it does a switch by OS then by CPU.

This comment has been minimized.

@memsharded

memsharded Jan 20, 2017

Contributor

Yes, perfect, sorry, I missed that.

@@ -53,10 +53,44 @@ def __init__(self, *args):
except:
self.libcxx = None
def _gcc_arch_flags(self):
if self.arch == "x86_64":
return "-m64"

This comment has been minimized.

@memsharded

memsharded Jan 19, 2017

Contributor

Same issue as in cmake.

This comment has been minimized.

@mropert

mropert Jan 19, 2017

Contributor

At least I'm consistent :)
Joking aside, as for CMake I don't think it can hurt to be explicit about bitness depending on arch.
The only regression would be if some existing package was not built on 64 bits with arch=x86_64, which I'd prefer to call a bug.

This comment has been minimized.

@memsharded

memsharded Jan 20, 2017

Contributor

Absolutely, you are quite right
For me it makes sense, but I wanted to make sure that people that consider this a feature won't get too annoyed. So far, it seems the feedback is consistent :)

ret.append(command_set + ' ' + name + '=' + value + ':$' + name)
# Standard UNIX "sh" does not allow "export VAR=xxx" on one line
# So for portability reasons we split into two commands
ret.append(name + '=' + value + ':$' + name)

This comment has been minimized.

@memsharded

memsharded Jan 19, 2017

Contributor

Maybe @lasote wants to have a look at this.

This comment has been minimized.

@mropert

mropert Jan 19, 2017

Contributor

I'm not fond of it but it turns out the system command in python invokes the default OS shell which is not guaranteed to handle Bash extensions.
For example on Solaris it's a vanilla sh that does not recognize export VAR=xxx in one line.

This comment has been minimized.

@lasote

lasote Jan 20, 2017

Contributor

Should be a valid way to do it.

self.assertIn("-library=stlport4", str(client.user_io.out))
else:
client = TestClient()

This comment has been minimized.

@memsharded

memsharded Jan 19, 2017

Contributor

Extract these 3 common lines

This comment has been minimized.

@mropert

mropert Jan 19, 2017

Contributor

Will do

This comment has been minimized.

@mropert

mropert Jan 20, 2017

Contributor

Fixed

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 19, 2017

I would appreciate feedback from @tru, @monsdar, @lasote, @madduci , about what could be the effects in gcc by adding by default the -m64 flag for x86_64 builds (as this PR proposes). So far, only -m32 was specified for 32 bits builds (arch=x86), but nothing was specified for x86_64, leaving the defaults.

@monsdar

This comment has been minimized.

monsdar commented Jan 19, 2017

Not much of a GCC guy myself, I'll see what our Linux experts say. My gut says that this won't be an issue.

@sztomi

This comment has been minimized.

Contributor

sztomi commented Jan 20, 2017

FWIW I'm using this flag in rpclib. It's purpose is to be able to cross-compile a full release package (that includes 64 bit and 32 bit) regardless of the arch running the compilation. Without -m32 and -m64, the compilation would default to the current arch. The only catch is that when cross-compiling, the compiler has to be build with multilib support. It will complain if it's not.

@madduci

This comment has been minimized.

Contributor

madduci commented Jan 20, 2017

So far as I know, on 64 bit architectures, in Linux the -m64 flag is ignored when compiling 64 bits targets, so it should be transparent and not influent. That's actually a Solaris/Apple thing

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 20, 2017

So @sztomi, sorry I didn't fully understand, would you be against the CMake and ConfigureEnvironment helpers adding the -m64 flag to CONAN_CXX_FLAGS and CONAN_C_FLAGS for arch=x86_64? Those CONAN flags will be lately appended to CMAKE_CXX_FLAGS if using macros like conan_basic_setup()

Or you meant that you are already adding that variable, and then, it will be an improvement, because it will be automatic now?

@sztomi

This comment has been minimized.

Contributor

sztomi commented Jan 20, 2017

No, I just wanted to provide insight into using this flag. I support adding it, but I noted that in the rare situations when you are cross-compiling (i.e. running one arch and compiling for the other) AND you are not running a multilib gcc, you will get an error. I think this is alright, because you need a multilib gcc to cross-compile and an explicit arch flag passed to conan is need anyway.

@tru

This comment has been minimized.

Contributor

tru commented Jan 20, 2017

Yeah I am not worried either.

@memsharded memsharded merged commit 6aff328 into conan-io:develop Jan 22, 2017

3 of 5 checks passed

codecov/patch 74.30% of diff hit (target 94.80%)
Details
codecov/project 94.68% (-0.13%) compared to 136f147
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 22, 2017

Excellent job @mropert , thanks very much, and everybody else for the feedback!

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 24, 2017

Glad I could help.
Next step, SPARC cpus :)

@mropert mropert deleted the mropert:feature/solaris2 branch Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment