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

control C++11 and castxml selection #95

Merged
merged 3 commits into from Dec 30, 2016
Merged

control C++11 and castxml selection #95

merged 3 commits into from Dec 30, 2016

Conversation

doudou
Copy link
Member

@doudou doudou commented Aug 24, 2016

This commit enables C++11 unconditionally on 16.04,
enables castxml unconditionally on gcc5 OSes (15.10, 16.04)
disables them unconditionally on other (OSes where castxml
is not available), and offers a choice to the user for the rest.

Once the C++11 support is added to orogen, it should be added
to the overrides as well.

Depends on orocos-toolchain/autoproj#24

This "competes" with

@jmachowinski
Copy link
Contributor

The logic is wrong. One can enable Cxx11 even with gccxml systems. You just need to look out, to
not put c++11 code into the transport types.

Also I don't see how the c++11 is enabled for the orogen packages.
Finally, what would be the way for a package, to say it needs cxx11 ?

@doudou
Copy link
Member Author

doudou commented Aug 25, 2016

One can enable Cxx11 even with gccxml systems. You just need to look out, to
not put c++11 code into the transport types.

Fair enough. So, how far back should we "allow" C++11 ?

Also I don't see how the c++11 is enabled for the orogen packages.

Read the PR description.

Finally, what would be the way for a package, to say it needs cxx11 ?

It either does it directly in its CMake code, or adds it explicitely in the autobuild files. If the package needs C++11, it should ensure that it is built with C++11 no matter what. That's not autoproj's job.

@doudou
Copy link
Member Author

doudou commented Aug 25, 2016

The logic is wrong. One can enable Cxx11 even with gccxml systems.

Getting back on that:

I actually believe that trying to officially allow mixed systems like this is a bad idea. It would add complexity on the orogen solution (need one option for the codegen and one for the build) for, as far as I can see, very little benefit - we never officially supported C++11 on 14.04.

If one knows what he/she is doing, he/she can forcefully enable cxx11 support by adding Autoproj.config.set 'cxx11', true to its init.rb. The package set will explicitely ignore these variables if they are already set. People that were explicitely setting the ROCK_USE_CXX11 in their overrides.rb should see no change with this PR applied.

@jmachowinski
Copy link
Contributor

A lot of external libs switched to Cxx11 now (e.g. URDF). So the question if we allow mixed systems is answered for us : yes. Also I would prefer to switch C++11 on by default, because there is basically no harm in it and as libraries migrate to c++11 in the near future almost all packages will turn on C++11 anyway.

The only thing we need to watch out for, is that until the next release, we do not allow C++11 in the base / transport types (at least in the header sections).

@doudou
Copy link
Member Author

doudou commented Aug 30, 2016

With this PR

  • C++11 will be on by default for anyone running on 16.04, which is the target of the next release
  • can be turned on for earlier Ubuntu releases, thus not changing existing installations.

I made the careful choice here since I actually see often Rock users still running older ubuntu releases. I don't see any downside in not forcing C++11 down their collective throat, while still migrating the official master to C++11.

@jmachowinski
Copy link
Contributor

This will lead to annoying build failures...
The 16.04 guys will not care about enabling c++11 in their CMakelists,
as it is on by default. The 14.04 guys will then get the build failures...

@doudou
Copy link
Member Author

doudou commented Sep 6, 2016

This will lead to annoying build failures...

Yes. As does 1/2 of the packages because their manifest.xml is not done properly, or because their CMakeLists.txt does not properly list dependencies.

The bottom line is that it won't happen overnight. Enabling C++11 to all packages, even the ones you don't control will cause build failures overnight for people using C++98 syntax / features not supported in C++11.

@jmachowinski
Copy link
Contributor

I don't get your point, you said c++11 is on for 16.04 by default. So they should build.

@doudou
Copy link
Member Author

doudou commented Sep 6, 2016

Enabling C++11 in autoproij, the cmake macros and/or orogen enables it for everyone and every package. Even the private ones used only on pre-16.04

@jmachowinski
Copy link
Contributor

Yes... And people can still 'downgrade' by override if the really need to. Having a different behaviour depending on the ubuntu version is where I see the problem.
We'll get a lot of compile errors on 14.04, if people develop on 16.04 and forget to enable cxx11 explicitly.

@doudou
Copy link
Member Author

doudou commented Sep 6, 2016

And people can still 'downgrade' by override if the really need to

People can still upgrade by override if they really need to ... How is that a point ?

Again, a difference in philosophy:

  • you: I want the workflow to work for my projects. I switch between 16.04 and older ubuntus, and I want that to work.
  • me: I want to avoid breaking existing installations without warning -- the possible breakage for 16.04 is an OK tradeoff for me as 16.04 installations are "new", and still not straightforward. In addition, I think that making sure (by having the build broken) that C++11 packages explicitely ask for C++11 is a good thing as it makes their successful build independent of autoproj. You don't know if the accidental introduction of C++11 in a package is a conscious choice by the developer. I only two weeks ago created a PR about "you accidentally introduced C++11 constructs" in a package, and it got accepted.

This commit enables C++11 and castxml unconditionally on some OSes
(for now, ubuntu 16.04), disables them unconditionally on other
(OSes where castxml is not available), and offers a choice to the
user for the rest.
@doudou
Copy link
Member Author

doudou commented Dec 16, 2016

OK. Removed the automatic setting of C++11 by default on 16.04 onwards, to have the same build environment on <16.04 and >=16.04. Which means that the only thing the PR does now is:

  • enable castxml on 16.04 onwards which is required to actually build
  • create the cxx11 configuration option to let the user enable C++11, and select castxml as importer if he does so

Is that acceptable ?

@jmachowinski
Copy link
Contributor

yep

@doudou doudou merged commit 727e11e into master Dec 30, 2016
@doudou doudou deleted the default_cxx11_castxml branch December 30, 2016 17:22
pierrewillenbrockdfki pushed a commit to pierrewillenbrockdfki/rock_core_package_set that referenced this pull request Feb 2, 2023
fix installing osg-collada on autoproj 1.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants