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

Tickets/dm 1361 #2

Merged
merged 4 commits into from Oct 13, 2014
Merged

Tickets/dm 1361 #2

merged 4 commits into from Oct 13, 2014

Conversation

mjuric
Copy link
Contributor

@mjuric mjuric commented Oct 10, 2014

No description provided.

@mjuric
Copy link
Contributor Author

mjuric commented Oct 10, 2014

I'd propose to move this functionality to EUPS' detect_compiler() -- e.g., maybe return a variable called CXX11_FLAG -- and just use it here. See https://github.com/RobertLuptonTheGood/eups/blob/master/lib/eupspkg.sh#L93 .

n.b.: these two commits should have been squashed.

@ktlim
Copy link

ktlim commented Oct 10, 2014

Nope, not squashed -- the latter should be rebased onto master.

@r-owen
Copy link
Contributor

r-owen commented Oct 10, 2014

Note: if we do move this to eups then it ought to return "-std=c++11 -Wno-deprecated-register". Unfortunately, if we do that, I don't know how to get bjam to use it. Aside from that, I think it's a great idea, especially if sconsUtils can also use the information.

Are you proposing that the eups test work like the boost test (based on trying to compile, rather than parsing the name and version of the compiler)? I hope so. I strongly prefer a test based on trying to compile to one based on knowledge of the compiler, since we know so little about certain compilers (e.g. icc).

@ktlim
Copy link

ktlim commented Oct 10, 2014

I'm worried about building in too much config testing in eups/eupspkg. This is what scons and configure are supposed to do; we shouldn't be usurping that, I would think.

@mjuric
Copy link
Contributor Author

mjuric commented Oct 10, 2014

Agree w. K-T; we shouldn't make sconsUtils-buildable products depend on eupspkg in any way.

This is just about external packages being built with EUPSpkg.

@r-owen
Copy link
Contributor

r-owen commented Oct 10, 2014

Can we not find one central location to compute CXXFLAGS that works for config-based build, bjam-based builds and scons?

I don't expect this central location to APPLY the flags, just tell us what they should be.

@r-owen
Copy link
Contributor

r-owen commented Oct 10, 2014

Also, if this is not going to apply to scons then what is the rush? boost is our only non-scons compiled C++ library (as opposed to Eigen, which is pure headers) at the moment. Still...I'd rather do it right the first time.

@RobertLuptonTheGood
Copy link

+1 on not putting tests into eupspkg (or taking them out).

I don't think that we should add the -Wno-deprecated-register to the compiler detection, they are different things. For example, you can turn off all the compiler warning filtering on the command line

I'm not convinced that we should be trying -std=c++11 and -std=c++0x a la autoconf. We know the compiler version and can do it once and for all. Sure, it sounds more general but if we get a compiler with some other notion it'll still stop working.

@mjuric
Copy link
Contributor Author

mjuric commented Oct 11, 2014

The problem with enumerated versions is that you need to either assume what the future versions will do, or keep updating the enumerated list. Also, distros still occasionally fiddle with the compiler version strings (nothing as bad as gcc 2.95 fiasco, but Apple is (for example) changing the version string on all its clang builds). That makes it difficult to come up with a reasonably complete list, I think, so this very limited detection test may work better.

@RobertLuptonTheGood
Copy link

In theory I agree with Mario, but in this case (looking for the old historical cases that don't understand -std=c++11) I think a version test would suffice. I am nervous about reinventing scons' configure contexts or autoconf.

@mjuric
Copy link
Contributor Author

mjuric commented Oct 11, 2014

So the argument is to just test if we're running w. gcc < 4.7 and set -std=c++0x, and default to -std=c++11 otherwise? I guess that would work.

@RobertLuptonTheGood
Copy link

Yes, that's the proposal. It's simple.

@mjuric
Copy link
Contributor Author

mjuric commented Oct 11, 2014

Thinking more about it, the thing we're really doing here is trying to get the code built with a compiler for a different language. The annoying thing is that that compiler's binary is still called 'c++' (instead of, say, 'c++11'). If that wasn't the case, enabling C++11 support would be as easy as exporting "CXX=c++11".

@mjuric
Copy link
Contributor Author

mjuric commented Oct 11, 2014

I just prototyped the code that checks for the version and, honestly, I'm not sure it's a major win in terms of simplicity or readability compared to autodetecting the flag:

build()
{
        cxxflags=-std=c++11

        detect_compiler
        if [[ "$COMPILER_TYPE" == gcc ]]; then
                CXX1=${CXX:-c++}
                IFS='.' read -a gccversion <<< "$($CXX1 -dumpversion)"
                if [[ ${gccversion[0]} -eq 4 && ${gccversion[1]} -lt 7 ]]; then
                        cxxflags=-std=c++0x
                fi
        fi

        ./b2 -j $NJOBS cxxflags=$cxxflags
}

@r-owen
Copy link
Contributor

r-owen commented Oct 11, 2014

I would prefer to auto-detect the flag because I have no idea when clang and ICC switched to using --c++11. I could look it up, but it's simpler to just run the test. I care about this because I want to change sconsUtils to use the same pattern, for the same reason.

@mjuric
Copy link
Contributor Author

mjuric commented Oct 11, 2014

I think Robert is arguing that the only compiler we should support with a flag other than -std=c++11 is gcc (versions less than 4.7). For everything else, just default to c++11. And once we up the required gcc to 4.7 (a few years from now), remove the special case all together.

@r-owen
Copy link
Contributor

r-owen commented Oct 11, 2014

If we only support -std=c++0x on gcc then I am concerned that it will leave out older (but quite usable) versions of clang, e.g. on linux.

@RobertLuptonTheGood
Copy link

We don't need to support old clangs. We only support g++ 4.4 because of RH 6.

@r-owen
Copy link
Contributor

r-owen commented Oct 11, 2014

The anaconda 1.8.0 on Linux comes with clang 3. Even if this is too old to be useful, I hope you see my point: we may very well want to support a newer anaconda's clang without having to parse version #s.

@RobertLuptonTheGood
Copy link

Why would we want to use it? We have access to modern versions of clang, and the default on linux boxes is g++. I do not want to be in the business of supporting old compilers just because we could.

@r-owen
Copy link
Contributor

r-owen commented Oct 11, 2014

As a heads up, my changes to sconsUtils on DM-1361 follow the same pattern (test for -std=c++11 first, then -std=c++0x, then complain). I just pushed the fully cleaned up version. If you strongly object to it, then I will ask you, Mario and K-T to come to some agreement. Once we get the release of eups and can merge boost (which relies on this eups ticket) and sconsUtils (which does not), LSST will be compiling with C++11 enabled.

@mjuric mjuric merged commit a76a01c into master Oct 13, 2014
@ktlim ktlim deleted the tickets/DM-1361 branch August 25, 2018 06:43
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

4 participants