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 includes for framework libs #630

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jacquetc
Copy link
Contributor

Hello,

This is a tentative fix for using Qt's "framework" libs on MacOS. See #450

@jacquetc
Copy link
Contributor Author

It seems that I have bit more work to do on it

@jacquetc jacquetc marked this pull request as draft July 27, 2023 08:18
.gitignore Outdated Show resolved Hide resolved
@ahayzen-kdab
Copy link
Collaborator

Also note how previously we battled adding support for frameworks from homebrew installs here c6ac928

Which route of Qt install are you trying to improve here ? As i thought the remaining issues were around using the Qt official binaries ? Or have you found another scenario of frameworks that doesn't work ? 😅

Maybe it would be useful if you could post the error that occurs on your machine with main so that we know which of the various combinations you are improving here :-)

@jacquetc
Copy link
Contributor Author

jacquetc commented Jul 27, 2023

I was trying to fix primarily the use of Qt 6.5 from the official installer. So, the use of framework libs, so adding a few specific args to the compiler.

So, my method was :

  • detect if the targeted Qt use framework libs
  • if so, adapt the args.

I'll be able to recreate the initial problem at home tonight. Yet, it was the same error than in #450

@ahayzen-kdab
Copy link
Collaborator

I was trying to fix primarily the use of Qt 6.5 from the official installer. So, the use of framework libs, so adding a few specific args to the compiler.

Awesome thanks ! There are just so many different combinations it's useful to know which is being investigated :-) I find it fun that we solved use of frameworks from a homebrew install but not from a the Qt official installer, so it will be interesting to see the difference :-) Any help on macOS or Windows builds is much appreciated as we are mostly Linux devs 😅

@jacquetc
Copy link
Contributor Author

I was trying to fix primarily the use of Qt 6.5 from the official installer. So, the use of framework libs, so adding a few specific args to the compiler.

Awesome thanks ! There are just so many different combinations it's useful to know which is being investigated :-) I find it fun that we solved use of frameworks from a homebrew install but not from a the Qt official installer, so it will be interesting to see the difference :-) Any help on macOS or Windows builds is much appreciated as we are mostly Linux devs 😅

I'm a cross-platform Qt dev and DevOps, so I know a little bit about such issues. I'll try to help, but must be clear about the platforms and installs I can play with:

  • Apple M1 - homebrew
  • Apple M1 - official installer
  • Apple M1 - vcpkg (a cxx-qt project consuming qt from vcpkg)
  • Windows ARM - official installer
  • Windows ARM - vcpkg (a cxx-qt project consuming qt from vcpkg)
  • Linux Ubuntu / Fedora ARM - system Qt
  • Linux Ubuntu / Fedora ARM - official installer
  • Linux Ubuntu / Fedora ARM - vcpkg (a cxx-qt project consuming qt from vcpkg)

I found out reading the code, that my first problem was to make qmake visible. So adding its folder to the PATH is a prerequise before using cxx-qt.

@ahayzen-kdab
Copy link
Collaborator

ahayzen-kdab commented Jul 27, 2023

I'm a cross-platform Qt dev and DevOps, so I know a little bit about such issues.

Awesome :-)

... make qmake visible. So adding its folder to the PATH is a prerequise before using cxx-qt.

Right, we also have a QMAKE env var you can set to point to a custom qmake, either qmake needs to be in your PATH or you set QMAKE to point to it.

@jacquetc
Copy link
Contributor Author

jacquetc commented Jul 27, 2023

I would use another name for the variable like CXX_QT_QMAKE. Adding a prefix is to not interfere with QT's future possible env variable. It's your choice of course.

@jacquetc jacquetc marked this pull request as ready for review July 27, 2023 12:42
@jacquetc
Copy link
Contributor Author

jacquetc commented Jul 27, 2023

State of local tests:

Do not merge before the completion of this list !

  • on Apple M1 - homebrew
  • on Apple M1 - official installer Qt6
  • on Apple M1 - official installer Qt5
  • Apple M1 - vcpkg (a cxx-qt project consuming qt from vcpkg)
  • Windows ARM - official installer Qt6
  • Windows ARM - official installer Qt5
  • Windows ARM - vcpkg (a cxx-qt project consuming qt from vcpkg)
  • Linux Ubuntu / Fedora ARM - system Qt
  • Linux Ubuntu / Fedora ARM - official installer
  • Linux Ubuntu / Fedora ARM - vcpkg (a cxx-qt project consuming qt from vcpkg)

@jacquetc
Copy link
Contributor Author

Also note how previously we battled adding support for frameworks from homebrew installs here c6ac928

Which route of Qt install are you trying to improve here ? As i thought the remaining issues were around using the Qt official binaries ? Or have you found another scenario of frameworks that doesn't work ? 😅

Maybe it would be useful if you could post the error that occurs on your machine with main so that we know which of the various combinations you are improving here :-)

I compared the Qt installation from homebrew and Qt official installation.
I can be mistaken, but it seems that the work done in c6ac928 had an unintentional side effect to make it compile with homebrew's installation, but by adding support for the framework style libs.

Homebrew installs Qt quite traditionally, plus add links like /opt/homebrew/include/QtCore linking to /opt/Cellar/qt/6.5.1_2/include/QtCore . So, somewhere, certainly this line, the good "include" path from homebrew was added. Yet, the work done was a necessary step which I was able to build on.

In the Qt official installation, "packages" containing the libraries and the headers are found in [...]/Qt/6.5.2/macos/lib/[module name].framework
Ex:
[...]/Qt/6.5.2/macos/lib/QtCore.framework
contains the files:
QtCore (links to Versions/A/QtCore)
Headers (links to Versions/A/Headers)
Resources (links to Versions/A/Resources)
Versions/A/QtCore
Versions/A/Headers
Versions/A/Resources

I added the include paths like [...]/Qt/6.5.2/macos/lib/QtCore.framework/Headers
And I added the framework args -f , like -f[...]/Qt/6.5.2/macos/lib/QtCore.framework

@ahayzen-kdab
Copy link
Collaborator

I can be mistaken, but it seems that the work done in c6ac928 had an unintentional side effect to make it compile with homebrew's installation, but by adding support for the framework style libs.

Yes this change was to add support for framework style libs and specifically what homebrew was producing, but were not yet checked for what the Qt installer produces (which appears to be subtly different as you've stated).

If you get a chance, checking that Qt 5.15 works via the Qt installer on macOS could would be good in case that is different again.

@jacquetc
Copy link
Contributor Author

I can be mistaken, but it seems that the work done in c6ac928 had an unintentional side effect to make it compile with homebrew's installation, but by adding support for the framework style libs.

Yes this change was to add support for framework style libs and specifically what homebrew was producing, but were not yet checked for what the Qt installer produces (which appears to be subtly different as you've stated).

If you get a chance, checking that Qt 5.15 works via the Qt installer on macOS could would be good in case that is different again.

Tested against Qt 5.15.2 (Official install), it compiles fine.

.gitignore Outdated Show resolved Hide resolved
@ahayzen-kdab
Copy link
Collaborator

It passes CI now 🥳

I'll let @Be-ing comment on the changes to the build side as well as he has been working on this side the most.

@jacquetc
Copy link
Contributor Author

It passes CI now 🥳

I'll let @Be-ing comment on the changes to the build side as well as he has been working on this side the most.

Thank you for your help @ahayzen-kdab

crates/cxx-qt-lib/build.rs Outdated Show resolved Hide resolved
crates/cxx-qt-lib/build.rs Outdated Show resolved Hide resolved
crates/cxx-qt-lib/build.rs Outdated Show resolved Hide resolved
@jacquetc
Copy link
Contributor Author

It seems that there are problem with the VCPKG version of Qt (told here) with this branch. So please wait for proper testing.

I have troubles installing VCPKG's Qt on Linux & MacOS due to "ICU" not finding "autoconf-archive". Investigating... Sure, it's outside our scope.

Comment on lines 493 to 498
for qt_module in &self.qt_modules {
paths.push(format!("{root_path}/Qt{qt_module}"));
}
Copy link
Contributor

@Be-ing Be-ing Jul 31, 2023

Choose a reason for hiding this comment

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

Is this correct when Qt is installed as a framework? This allows using include paths like #include <QString> instead of #include <QtCore/QString>. Should we skip this part for frameworks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never included using #include <QtCore/QString>, always using #include <QString> like it's shown in the Qt documentation. So, I don't see the problem. Or I misunderstand your comment ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code should likely be moved into an else block of the if self.has_framework_libs statement. If it wasn't sufficient before to get the include paths with frameworks, it's probably okay to not add these include paths when using frameworks.

Copy link
Contributor Author

@jacquetc jacquetc Aug 4, 2023

Choose a reason for hiding this comment

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

I did a few researches :
The module-style includes (#include <QObject>)are essential in MacOS framework style. Using the old #include <QtCore/qobject.h> or #include <QtCore/QObject> is not possible any longer. The old style still runs well on Windows and Linux thought.

I was testing only on qml_minimal example project, so this problem didn't appear.
On a hunch, I ran QMAKE=/Users/cyril/Qt/6.5.2/macos/bin/qmake cargo run -p qml-minimal-no-cmake , so Qt's official install on MacOs with framework style

It gave me these errors:
cargo:warning=In file included from /Users/cyril/Devel/cxx-qt/target/debug/build/cxx-qt-lib-c19809693f328b69/out/cxxbridge/sources/cxx-qt-lib/src/core/qlist/qlist_i32.rs.cc:1:
cargo:warning=/Users/cyril/Devel/cxx-qt/target/debug/build/cxx-qt-lib-c19809693f328b69/out/cxx-qt-lib/qlist.h:11:10: fatal error: 'QtCore/QList' file not found
cargo:warning=#include <QtCore/QList>
cargo:warning= ^~~~~~~~~~~~~~
cargo:warning=1 error generated.
exit status: 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh curious, i found doing #include <QtCore/QObject> useful as it allows you to quickly determine which modules you need to include and link or why a library needs eg QtQuick as you can search for it quickly.

Guess we'll have to drop doing that if this doesn't work in macOS anymore (i can create a separate PR to do this), unless there is a different include folder that can be used that does include the module name + the header name ?

Copy link
Contributor

@Be-ing Be-ing Aug 7, 2023

Choose a reason for hiding this comment

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

I looked at the CI output from another projecting using the Qt official binary installer and the include paths are:

-isystem /Users/build/Qt/online/6.3.2/macos/lib/QtCore.framework/Headers
-isystem /Users/build/Qt/online/6.3.2/macos/lib/QtCore.framework/Headers/6.3.2
-isystem /Users/build/Qt/online/6.3.2/macos/lib/QtCore.framework/Headers/6.3.2/QtCore

So it seems we need:

        if self.has_framework_libs {
            let lib_root_path: String = self.qmake_query("QT_INSTALL_LIBS");

            let qt_version = &self.qt_version;
            for qt_module in &self.qt_modules {
                paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers"));
                paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers/{qt_version}"));
                paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers/{qt_version}/Qt{qt_module}"));
            }
        }

Please try that code with both #include <QString> and #include <QtCore/QString> style includes.

I also saw -isystem /Users/build/Qt/online/6.3.2/macos/include in the include paths, so forget what I was saying about moving the QT_INSTALL_HEADERS path into an else block; leave that code as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,
I'm back, I'll be able to test that this week.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jacquetc Have you had a chance to test the code I posted above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No yet, I'll take time tomorrow at noon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested, exactly the same error.

@jacquetc jacquetc force-pushed the fix/macos_qt_libs_as_framework branch from e61d693 to 0f3e6e8 Compare August 1, 2023 13:01
@Be-ing
Copy link
Contributor

Be-ing commented Aug 1, 2023

Please check that this branch also builds with Qt installed from Homebrew, which is also shipped as frameworks. You can test that by setting the QMAKE environment variable, for example QMAKE=/path/to/qmake/from/homebrew cargo run -p qml-minimal-no-cmake

@jacquetc
Copy link
Contributor Author

jacquetc commented Aug 4, 2023

QMAKE=/path/to/qmake/from/homebrew cargo run -p qml-minimal-no-cmake

Tested successfully with homebrew's Qt

@jacquetc jacquetc force-pushed the fix/macos_qt_libs_as_framework branch from 0f3e6e8 to cfff8bc Compare August 4, 2023 13: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.

3 participants