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

add support for qt6 #14

Merged
merged 2 commits into from
Oct 17, 2021
Merged

add support for qt6 #14

merged 2 commits into from
Oct 17, 2021

Conversation

TheEdward162
Copy link
Contributor

@TheEdward162 TheEdward162 commented Oct 2, 2021

the exact changes are:

  • Assigning an int to QChar fails to compile due to type ambiguity. Changed occurences to either assign uchar or to use the default constructor.
  • [qt5.14] QLineF::intersect replaced by QLineF::intersects
  • [qt6.0] QTableView::viewOptions replaced by QTableView::initViewItemOption with a different interface - covered by preprocessor #if check
  • [qt5.11] QFontMetrics::width replaced by QFontMetrics::horizontalAdvance
  • [N/A] QFileDialog::setFileMode(QFileDialog::DirectoryOnly) replaced by QFileDialog::setFileMode(QFileDialog::Directory) and QFileDialog::setOption(QFileDialog::ShowDirsOnly, true)
  • [qt4.3] QLayout::margin replaced by QLayout::getContentsMargins
  • [N/A] Multiple calls to QString(int) replaced by QString::number(int) (I belive this was a bug in the original implementation and was uncovered thanks to the update)
    • This is probably the same problem as the one that popped up in the first change (QChar), not sure why it didn't complain sooner
  • There is a problem with Offset type not being the same size as size_t on macos, so a static_cast<size_t>(Offset) was added to fix the warning

Everything still builds with qt5, most of the changes were replacing functions which are already deprecated in qt5 and have defined alternatives.

These changes are sufficient to build on Linux with qt6, however, to build macos I've had to bump the QMAKE_MACOSX_DEPLOYMENT_TARGET found in build-macos.sh from 10.14 to at least 10.15.

Also I've had to manually copy the elf.h header from another libelf since the one provided by brew does not contain this header (it is not maintained anymore, but I'm not sure why the header is missing in the first place). This means that even with these changes the project won't build for macos with qt6 in the environment described in azure-pipelines.yml file.

closes #13

the exact changes are:
* Assigning an int to QChar fails to compile due to type ambiguity. Changed occurences to either assign uchar or to use the default constructor.
* QLineF::intersect replaced by QLineF::intersects
* QTableView::viewOptions replaced by QTableView::initViewItemOption with a different interface - covered by preprocessor #if check
* QFontMetrics::width replaced by QFontMetrics::horizontalAdvance
* QFileDialog::setFileMode(QFileDialog::DirectoryOnly) replaced by QFileDialog::setFileMode(QFileDialog::Directory) and QFileDialog::setOption(QFileDialog::ShowDirsOnly, true)
* QLayout::margin replaced by QLayout::getContentsMargins
* Multiple calls to QString(int) replaced by QString::number(int) (I belive this was a bug in the original implementation and was uncovered thanks to the update)
	- This is probably the same problem as the one that popped up in the first change (QChar), not sure why it didn't complain sooner
* There is a problem with Offset type not being the same size as size_t on macos, so a static_cast<size_t>(Offset) was added to fix the warning
@TheEdward162 TheEdward162 mentioned this pull request Oct 2, 2021
@jdupak
Copy link
Collaborator

jdupak commented Oct 2, 2021

There problem here is, that we want to support Debian stable - qt5.9. I am fairly confident, that your changes will not build there (5.10 is the boundary, I believe), as the new functions are not present (I know about intersects).

Given that those incompatible changes have mostly one to one correspondence, I would change the codebase to conform to qt6 and create a polyfill header file defining the new functions using the old ones.

@ppisa
Copy link
Member

ppisa commented Oct 2, 2021

The Debian distribution Qt support

  • actual stable 11 bullseye Qt 5.15.2
  • old stable 10 buster Qt 5.11.3
  • old old stable 9 stretch Qt 5.7.1
  • actual LTS 20.04 focal 5.12.8
  • prev LTS 18.04 bionic 5.9.5

Regarding Debian, I agree with dropping support for old old stable and requirement for Qt 5.10 is acceptable. The Ubuntu can be a bigger problem, bionic support is announced to April 2028.

You can check Qt support for other distributions in Distrowatch

Another problem is official AppImage build, because its base system contains libQt5Core5-5.6.2, at least on Open Suse Build service....

So I am not sure what to do, may be leave pre Qt 5.10 base distributions to stuck on the last successfully build QtMips package.

@jdupak
Copy link
Collaborator

jdupak commented Oct 2, 2021 via email

@ppisa
Copy link
Member

ppisa commented Oct 3, 2021

To move forward, I am for accepting all changes which still builds with Qt 5.9. If there stays some which are blocker for 5.9, then start next round of discussion what to do with the rest. It is possible to use ifdefs, but they are far from beeing ideal.

As for assembler literals, I am not happy by assigning character constants to final variable. Because character constants are specific to the host system encoding, it can be even EBCDIC, on the other hand QtMisp emulated MIPS architecture Linux kernel API is for sure ASCII based. So at the end correct solution is to separate source character and target value. The final value to write should be defined as
uint8_t byte;
Case should be based on Qchar directly in host system encoding. When sequence "\n" is recognized in host system encoding then it should be converted to ASCII byte = 0x0a; If the character should end in the final string without special control sequence expansion then it byte = ch.toLatin1() should be used. Final write should change to
mem->write_u8(address, byte, ae::INTERNAL);
So please update to this pattern. I think that it is more readable and clean than original one. The corresponding code in QtRvSim should be revisited as well.

As for CMake, may it be it worth to open issue and collect there all pointers to work done in QtRvSim and @kchasialis QtMips repos and prepare switch of QtMips devel branch to CMake. Integration of branch predictor changes there is another step which worth some time in longer perspective.

@TheEdward162
Copy link
Contributor Author

I've updated the PR description to contain qt versions in which the replacement symbol was introduced. The ones with N/A don't specify version in qt docs, so I'm assuming they were "always" supported.

The symbols not available in qt <= 5.9:

  • QLineF::intersects, appeared in qt5.14
  • QTableView::initViewItemOption, appeared in qt6.0 (right now behind #if check)
  • QFontMetrics::horizontalAdvance, appeared in qt.5.11

I'm not sure how a polyfill header @jdupak is describing would look like, but given some pointers I'd be happy to implement it. Otherwise I think it'd have to be covered by ifdefs, which I don't really like either.

I'll change the character handling as per your request.

@ppisa ppisa merged commit 223704e into cvut:devel Oct 17, 2021
@ppisa
Copy link
Member

ppisa commented Oct 17, 2021

Thanks @TheEdward162 for contribution and updates.

I have checked that code builds on Debian 10 with Qt 5.11 and minor adjustment has been necessary to keep to use QLineF::intersect for Qt older than 5.14 because QLineF::intersects has been introduced by Qt 5.14.

I have corrected one spacing error and converted Qt version check to use QT_VERSION_CHECK(6, 0, 0).

I am not sure if the use of

fm.horizontalAdvance("+99")

is the right change when @jdupak changed it to

fm.width("+99")

in the 774757c Qt: compatibility changes with qt5.9.6 to keep code buildable for Qt 5.9.6.

It would be great if you can check compatibility with Qt 6.0 version.

@TheEdward162 TheEdward162 deleted the feature/qt6-support branch October 18, 2021 10:31
@TheEdward162
Copy link
Contributor Author

@ppisa I confirm that I can build and lanuch QtMips GUI app built from the latest devel on Qt6.2.0

@ppisa
Copy link
Member

ppisa commented Oct 18, 2021

Thanks. In the longer term, we need to switch project build to CMake. This has been already done for the RISC-V simulator variant (QtRvSim) by @jdupak and propagated to the fork of the QtMips started by @kchasialis which includes branch predictor visualization. I have even some small funding/scholarship available for CTU students for this year to cooperate on QtRvSim and possibly even QtMips consolidation.

@jdupak
Copy link
Collaborator

jdupak commented Nov 21, 2021

For future reference:
I have moved all the ifdefs away from the project to src/common/polyfills/qt5. It is my intention to have all interfaces unified and use ifdefs only in polyfills. I have also added polyfill for horizontalAdvance discussed above.

I also found problems with cmakelists, where qt5 was hardcoded - now qt major version is detected. (Based on https://www.steinzone.de/wordpress/how-to-support-both-qt5-and-qt6-using-cmake/).

Finally, I have added a CI test run for Qt6 (6.2.1 on Ubuntu Latest) to prevent future compatibility breakages.

@jdupak jdupak mentioned this pull request Nov 21, 2021
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.

3 participants