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 qt gcc 6 #187

Merged
merged 6 commits into from
Aug 23, 2017
Merged

Fix qt gcc 6 #187

merged 6 commits into from
Aug 23, 2017

Conversation

dstoup
Copy link
Collaborator

@dstoup dstoup commented Aug 15, 2017

Qt won't build with gcc 6. This patch is a bit heavy handed since it disables the scripts module. I don't believe we use it anywhere and I don't think this patch is a permanent need since we'll likely switch over to Qt 5 at some point.

@kwcvrobot
Copy link
Collaborator

Windows build succeeded: https://open.cdash.org/buildSummary.php?buildid=5020576

@kwcvrobot
Copy link
Collaborator

Mac build succeeded: https://open.cdash.org/buildSummary.php?buildid=5020594

@kwcvrobot
Copy link
Collaborator

Mac build succeeded: https://open.cdash.org/buildSummary.php?buildid=5020622

@kwcvrobot
Copy link
Collaborator

Linux build succeeded: https://open.cdash.org/buildSummary.php?buildid=5021587

@dstoup
Copy link
Collaborator Author

dstoup commented Aug 16, 2017

@mwoehlke-kitware @mleotta

It would be great to get your opinions on this branch. I have been bumping my nose against the gcc 6 issue for some time and would like to get Qt working. As I stated above, it is a bit heavy-handed by disabling script. It occurred to me that another approach which might be more clean is, add the -no-script and -no-scripttools flags when we disable Javascript since much of what is built by -script is in Qt/src/3rdparty/webkit/Source/JavaScriptCore. One could also argue that these items should be disabled with the -no-webkit flag we have. If we moved to that option, which I am starting to prefer, I would enforce it when we are using gcc >=6

@@ -393,7 +393,7 @@ bool QAccessibleTable2::unselectColumn(int column)
QModelIndex index = view()->model()->index(0, column, view()->rootIndex());
if (!index.isValid() || view()->selectionMode() & QAbstractItemView::NoSelection)
return false;
view()->selectionModel()->select(index, QItemSelectionModel::Columns & QItemSelectionModel::Deselect);
view()->selectionModel()->select(index, static_cast<QItemSelectionModel::SelectionFlags> (QItemSelectionModel::Columns & QItemSelectionModel::Deselect));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whee! This is actually a bug in Qt; the & should be a |. (This change is just hiding the actual bug.)

Interestingly, ViViA had an instance of this exact same bug 😄.

@@ -817,7 +817,7 @@ QWidget *PreviewManager::raise(const QDesignerFormWindowInterface *fw, const Pre
{
typedef PreviewManagerPrivate::PreviewDataList PreviewDataList;
if (d->m_previews.empty())
return false;
return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh.... also a bug in Qt, but the correct fix this time.

@mleotta
Copy link
Member

mleotta commented Aug 16, 2017

I'm not going to have much to add here other than a suggestion: Can we upgrade to a newer Qt that already works with GCC 6? That is a question for @mwoehlke-kitware. I don't know how much effort is needed to upgrade our current applications that use Qt. Also, I don't know what licensing issues the may introduce (e.g. if Qt changed it's license).

I'm not strongly pushing to upgrade, only pointing out that the longer we hang on to the old version the harder it will be to support.

@mwoehlke-kitware
Copy link
Member

another approach which might be more clean is, add the -no-script and -no-scripttools flags when we disable Javascript

That seems plausible; I don't think we (at least, neither ViViA nor MapTk) are directly using QtScript anywhere...

@mwoehlke-kitware
Copy link
Member

Can we upgrade to a newer Qt that already works with GCC 6?

Not really; I believe we are already on latest Qt4, and upgrading to Qt5 requires at least some porting, and figuring out if PySide2 is usable yet (or losing the web bits of ViViA until it is).

Eventually we need to find out what's up with Qt5/PySide2 (or decide we just don't care about ViViA web), but I think that's believed to be sufficiently non-trivial that it is blocked by lack of tasking.

@dstoup
Copy link
Collaborator Author

dstoup commented Aug 16, 2017

+1 to @mwoehlke-kitware .. that was my reasoning for not looking into Qt5 further. It would be good to get there, but it is non-trivial at the moment.

I didn't think anyone was using script. I will make the change to move it under the disable javascript option and repush. Disabling these flags should decrease the Qt build time substantially as well.

@dstoup
Copy link
Collaborator Author

dstoup commented Aug 16, 2017

... all that said, perhaps this is a great opportunity for one of those 'version' branches which we have been making. That will let us experiment with Qt 5 while defaulting to Qt 4.

@mleotta
Copy link
Member

mleotta commented Aug 16, 2017

Yes, I agree. It would be good to add an option for Qt 5 to allow for easier experimentation and to see what Fletch related issues we may run into.

After talking with @mwoehlke-kitware I think the plan is to land VTK 8.0 as the official Fletch version before we land Qt 5. There may be some issues with using newer Qt and older VTK together.

@dstoup
Copy link
Collaborator Author

dstoup commented Aug 16, 2017 via email

@mwoehlke-kitware
Copy link
Member

There may be some issues with using newer Qt and older VTK together.

Indeed; another project I was working on transitioned from VTK5/Qt4 to VTK6+/Qt5 and ultimately needed VTK8 in order to fully work correctly with Qt5. (I believe it even drove some changes to land in VTK8 just before it was released.)

Previously we were selecting webkit on/off and javascript on/off. With the build failures with gcc >= 6 we were faced with another removal, script and scripttools. It makes more sense to turn off everything or all all to build. Turning those packages on was a use case that was almost never activated.
@kwcvrobot
Copy link
Collaborator

Mac build succeeded: https://open.cdash.org/buildSummary.php?buildid=5022791

@kwcvrobot
Copy link
Collaborator

Windows build failure: https://open.cdash.org/buildSummary.php?buildid=5022798

@kwcvrobot
Copy link
Collaborator

Linux build succeeded: https://open.cdash.org/buildSummary.php?buildid=5023215

@dstoup
Copy link
Collaborator Author

dstoup commented Aug 21, 2017

Jenkins test this please

@kwcvrobot
Copy link
Collaborator

Windows build failure: https://open.cdash.org/buildSummary.php?buildid=5028093

@kwcvrobot
Copy link
Collaborator

Mac build succeeded: https://open.cdash.org/buildSummary.php?buildid=5028151

@kwcvrobot
Copy link
Collaborator

Linux build succeeded: https://open.cdash.org/buildSummary.php?buildid=5028276

@kwcvrobot
Copy link
Collaborator

Windows build succeeded: https://open.cdash.org/buildSummary.php?buildid=5030272

@kwcvrobot
Copy link
Collaborator

Mac build succeeded: https://open.cdash.org/buildSummary.php?buildid=5030293

@kwcvrobot
Copy link
Collaborator

Linux build succeeded: https://open.cdash.org/buildSummary.php?buildid=5031085

@dstoup dstoup merged commit fe25563 into master Aug 23, 2017
@dstoup dstoup deleted the fix-qt-gcc-6 branch August 23, 2017 12:12
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.

4 participants