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

Fix issue #2600 #2628

Closed
wants to merge 1 commit into from
Closed

Conversation

EmperorArthur
Copy link
Contributor

Alternative to pull #2608

Builders no longer have to use qmake "CONFIG += c++11" to build,
like they've been forced to since e99b0c9.

The lessThan(QT_MAJOR_VERSION, 5) was removed because qmake was not setting QMAKE_CXXFLAGS. It still worked, because modern compilers default to c++11, but this makes it explicit.

Without removing lessThan(QT_MAJOR_VERSION, 5) mumble will not build with a modern version of QT on a compiler that defaults to c++98.

Builders no longer have to use qmake "CONFIG += c++11" to build,
like they've been forced to since e99b0c9.
@mkrautz mkrautz self-assigned this Nov 8, 2016
@mkrautz
Copy link
Contributor

mkrautz commented Nov 8, 2016

The lessThan(QT_MAJOR_VERSION, 5) was removed because qmake was not setting QMAKE_CXXFLAGS. It still worked, because modern compilers default to c++11, but this makes it explicit.

I'm not sure why you're focusing on lessThan(QT_MAJOR_VERSION, 5) ? That part of the PR was simply there for Qt 4 compatibility...

You say:

[...] Without removing lessThan(QT_MAJOR_VERSION, 5) mumble will not build with a modern version of QT on a compiler that defaults to c++98.

But surely, lessThan(QT_MAJOR_VERSION, 5) means the opposite of modern Qt? It only triggers for versions < 5.0.

I'm confused.

@mkrautz
Copy link
Contributor

mkrautz commented Nov 8, 2016

Ah, OK... Sorry. I read your PR in the context of PR #2608.
Same with your comment in #2608.
I now see that it's in the context of what's currently in the tree.

Now... You're saying that PR #2608 doesn't work for you on Sid?

@mkrautz
Copy link
Contributor

mkrautz commented Nov 8, 2016

Also, I believe this PR seems to assume that Qt doesn't automatically pass -std=c++XX in CXXFLAGS.

I believe I tested that it does in PR #2608, and from my reading of qtbase/mkspecs/features/default_post.prf it seems like it should?
See https://github.com/qt/qtbase/blob/1e97037ed97aade9eb107bbc595785c2de76f7dc/mkspecs/features/default_post.prf#L98-L117

(I'm not in a position to do build tests at the moment...)

@EmperorArthur
Copy link
Contributor Author

Now... You're saying that PR #2608 doesn't work for you on Sid?

PR #2608 does compile on Sid, and I'm happy for that.

Also, I believe this PR seems to assume that Qt doesn't automatically pass -std=c++XX in CXXFLAGS.

That's correct. I can't find anywhere in the generated makefiles or in the output when actually running make.

My concern is with the bottom of this page: https://gcc.gnu.org/projects/cxx-status.html

If we're using GCC 6.0 or below, it defaults to c++98. If we're using that to compile mumble with QT 5.7 or above it won't work.

My concerns go away if Qt does automatically pass -std=c++XX in CXXFLAGS, or if it turns out QT 5.7 and above can't be built on GCC 6.0 or below.

Side note:

Sid no longer requires QMAKE_LIBDIR *= /usr/lib/$${MULTIARCH_TRIPLE}/c++11. At this point, as long as QMAKE_CXXFLAGS *= -std=c++98 doesn't happen Sid is happy.

@mkrautz
Copy link
Contributor

mkrautz commented Nov 8, 2016

Yes, the idea of the change in the first place was to ensure that we explicitly pass a -std=c++XX option to the compiler.

And for Mumble 1.3.x, we want that to be -std=c++98, unless we're on Qt 5.7 or above -- or a dependency, like ZeroC Ice, requires C++11.

For Mumble 1.4.x, we'd drop C++98 support and go C++11 or later.

@mkrautz
Copy link
Contributor

mkrautz commented Nov 8, 2016

What Qt were you building against, BTW? It's possible that Qt will only pass -std=c++XX on 5.7, but not on any release below that.

@mkrautz
Copy link
Contributor

mkrautz commented Nov 8, 2016

WDYT about #2630?

@EmperorArthur
Copy link
Contributor Author

Withdrawing in favor of #2630

mkrautz added a commit that referenced this pull request Nov 13, 2016
This PR adds a cplusplus.pri file (which gets included by compiler.pri).

Its purpose is to handle selection of C++ standards at build time via qmake's CONFIG variable.

Builders can now specify, say, qmake -recursive -Wall main.pro CONFIG+="release c++11" to select C++ mode.

Besides the ability to explicitly select a C++ standard, cplusplus.pri will auto-detect an appropriate C++ standard for the current system.

Supercedes PRs #2608 and #2628
@EmperorArthur EmperorArthur deleted the c++11_fix branch November 14, 2016 19:19
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.

2 participants