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

Added support to Solaris #862

Merged
merged 4 commits into from Jan 13, 2017

Conversation

Projects
None yet
6 participants
@madduci
Contributor

madduci commented Jan 12, 2017

Added extra informations to enable conan to work with Solaris x86

Michele Adduci
@CLAassistant

This comment has been minimized.

CLAassistant commented Jan 12, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

memsharded
madduci
Michele Adduci


Michele Adduci seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

arch: [x86, x86_64, ppc64le, ppc64, armv6, armv7, armv7hf, armv8]
compiler:
cc:

This comment has been minimized.

@memsharded

memsharded Jan 12, 2017

Contributor

maybe rename it to sun-cc? Just so people do not assume that this would be a generic C++ compiler

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

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 12, 2017

Looks good, thanks very much for this contribution.
Some tests are broken though, no worries, if you can have a look, great, otherwise, we will have a look asap.

I think there is nothing that should be a problem, so I suggest planning it for next release 0.19.

Have you managed to sign the CLA? For some reason it appears as not signed yet. Thanks!

if self.os == "Linux" or self.os == "Macos" or self.os == "FreeBSD":
if self.compiler == "gcc" or "clang" in str(self.compiler):
if self.os == "Linux" or self.os == "Macos" or self.os == "FreeBSD" or self.os == "SunOS":
if self.compiler == "gcc" or "clang" in str(self.compiler) or "cc" in str(self.compiler):

This comment has been minimized.

@lasote

lasote Jan 12, 2017

Contributor

"cc" == self.compiler better than "cc" in str(self.compiler)
Could be error prone.
But I agree with @memsharded calling it sun-cc or something like that.

@madduci

This comment has been minimized.

Contributor

madduci commented Jan 12, 2017

sure, we can change it, but do we map the Solaris Compiler ("cc") to that name then?

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 12, 2017

@madduci I am not sure if I understood your question. It is only necessary to change it in the settings you are checking, it is not necessary to do anything in the system, it is not "connected" to the real "cc" compiler in the machine. It is assumed that for that setting, the build system will manage to use it properly. Was that the question?

@madduci

This comment has been minimized.

Contributor

madduci commented Jan 12, 2017

Got it, I've renamed the compiler to sun-cc as suggested ;-)

memsharded and others added some commits Jan 12, 2017

Merge pull request #1 from memsharded/feature/sun_support
tests passing in Win for SunOS support
@codecov-io

This comment has been minimized.

codecov-io commented Jan 13, 2017

Current coverage is 94.80% (diff: 52.17%)

Merging #862 into develop will decrease coverage by 0.04%

@@            develop       #862   diff @@
==========================================
  Files           235        236     +1   
  Lines         17103      17175    +72   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          16221      16282    +61   
- Misses          882        893    +11   
  Partials          0          0          

Powered by Codecov. Last update d8490b2...dd78b53

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 13, 2017

The codecov is very picky, but as we don't have CI in SunOS, this will decrease the coverage a little, but not problem at all. Anyone knows a free service like travis-ci but for SunOS?. In any case, I guess that @madduci will be using those settings, so for me it is good enough to validate it.

Merging it, thanks very much for your contribution!

@memsharded memsharded merged commit 806bdcf into conan-io:develop Jan 13, 2017

2 of 5 checks passed

codecov/patch 52.17% of diff hit (target 94.84%)
Details
codecov/project 94.80% (-0.05%) compared to d8490b2
Details
licence/cla Contributor License Agreement is not signed yet.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@madduci

This comment has been minimized.

Contributor

madduci commented Jan 13, 2017

Yes, I've managed to run conan on Solaris 10 x86 and I've built some libraries such as OpenSSL (for which, only 64 bit support is available) and it works really good. It solved the issues of setting another build process for that.
The unique change for me was introducing "cc" instead of "sun-cc" in the configuration setting (as now in this patch). Thanks again for accepting it

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 13, 2017

Great, thanks for the feedback!

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 13, 2017

Hello guys,

I was about to submit my own pull request for Solaris support when the rebase conflict shown me someone already done it :)

I've tried to use this version and noticed a few things that were missing for me:

  • Arch flags (Solaris convention is to build -m32 by default even on x86_64 cpu, so -m64 needs to be added in some cases)
  • Support for libstlport4

Since I have those working on my branch, I can submit a pull request as soon as I've resolved the conflicts (hopefully during next week) if you're OK with it.

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 13, 2017

Yes, Mathieu, it seems that Michele was faster :)

Sure, please send a PR with your changes, so @madduci can review it too and validate.

There is still time for next release, no hurry, the milestone has not a date yet, so it should be at least 2 weeks from today.

@madduci

This comment has been minimized.

Contributor

madduci commented Jan 13, 2017

@mropert actually the flag -m32/-m64 has to be set as CFLAGS / CPPFLAGS for some libraries and for other ones you have to set CC="cc -m32" or CC="cc -m64". It's up to your implementation.
For libstlport4, I see it is specific for gcc, not for Oracle Compiler

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 13, 2017

@madduci,

  • By arch flags I meant that today conan expects all OS to follow the Linux convention on x86_64: build with -m64 by default. That's why if you select "x86" as architecture conan adds -m32 to your compile flags but nothing if it's set to "x86_64". On Solaris compilers follow a different convention: everything is built with -m32 by default, unless specified. Hence conan generators need a little adaptation.
  • stlport4 can be used with sun-cc (using -library=stlport4). In fact we have been using it in production for some time here because it has better C++ standard support (for a C++03 library that is). Oracle has a documentation on libCstd's shortcoming: oracle.com
@madduci

This comment has been minimized.

Contributor

madduci commented Jan 14, 2017

@mropert Ive already experienced that. I've adjusted my "build" phase by doing

if platform.system() == "SunOS": if self.settings.arch == "x86": compiler = 'CC="cc -m32' flags = 'CFLAGS="-m32"' else: compiler = 'CC="cc -m64"' flags = 'CFLAGS="-m64"'

For stlport4, I haven't tested it, I was compiling C libraries such as OpenSSL and libcurl. I think you can submit the PR, since there's room to extend the support to stlport4

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 14, 2017

I think that Conan should take care of that (as it does for the Linux convention) instead of asking each maintainer to have a special case for Solaris in its recipe.
The rule seem generic enough to be handled by the package manager and not by each invidual package.
@memsharded, what do you think?

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 14, 2017

Yes, the best if possible is to have the different cases modeled in settings and have those settings automatically translated to the compiler by some helper, like the CMake one. So, even in that case, the package manager is not in charge of handling that! :) It is the individual package recipes that handle that (that is the only way the package manager will not get in the middle if someone pretends to do different things or adapt to their very own processes and tools), the only thing is that it somewhat hidden in the CMake helper. Don't use the helper, and you can do any other flags you want.

But, I agree, having them handled by the helper (probably also by the ConfigureEnvironment helper, if using autotools/make) would be the easiest for package creators if possible. Is it possible to add stlport4 to the list of available libstdcxx settings?

@madduci

This comment has been minimized.

Contributor

madduci commented Jan 14, 2017

I think stlport4 can be added to the settings file, next to libCstd and select it explicitly, resolving the issue.

Beware, as by official Oracle website itself, stlport4 isn't properly suggested and might be also removed in future in favour of newer versions (which leads then to include different versions of it in conan)
Oracle C++ Compiler.
Why not using then libstdcxx?

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 15, 2017

Yes that was my idea.
The default if conan detects Solaris with sun-cc should be to select libCstd in the defaults generated in conan.conf but it should be possible to override it through config or profiles.
With sun-cc 5.14 I would even recommend selecting libstdc++ by default since it's required for C++11 but that's just an idea.
As far libstdcxx I can't remember why we prefered to select stlport, that was a few years ago already.

@mropert

This comment has been minimized.

Contributor

mropert commented Jan 15, 2017

@memsharded, @madduci,
I have rebased my work on top of @madduci's feature and got the tests working on my home setup (FreeBSD).
Tomorrow I should be able to check on our Solaris machines at work if everything is fine and issue the pull request.
After this one is validated I'll take a look at Solaris support on SPARC ;)

@madduci

This comment has been minimized.

Contributor

madduci commented Jan 16, 2017

@mropert that sounds a great news!

@memsharded

This comment has been minimized.

Contributor

memsharded commented Jan 16, 2017

Thanks @mropert and @madduci, sounds a good plan: I'll add your PR when issued to the next release milestone: https://github.com/conan-io/conan/milestone/12 (date not assigned yet, but expect a couple of weeks, we'll try to assign it tomorrow).

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