-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[qt5-base] Remove duplicate flags qt #31634
[qt5-base] Remove duplicate flags qt #31634
Conversation
This idea is interesting. I have tested the build here: The qmodule.pri file still contains a lot more redundant information collected during the qt5-base build:
|
All these flags are passed again from vcpkg here:
|
But there are still duplicates like This would allow cross building with vcpkg which is supported by QT but broken: https://github.com/daschuer/vcpkg/actions/runs/5131095086/jobs/9230730990 See a resulting compiler invocations using this branch:
|
# Remove duplicate flags from qmodule.pri issue -> https://github.com/microsoft/vcpkg/issues/28835 | ||
if(EXISTS "${CURRENT_INSTALLED_DIR}/tools/qt5/mkspecs/qmodule.pri") | ||
file(READ "${CURRENT_INSTALLED_DIR}/tools/qt5/mkspecs/qmodule.pri" QMODULE_PRI_CONTENT) | ||
string(REGEX REPLACE "QMAKE_CXXFLAGS_RELEASE\\+=[^\n]*\n" "QMAKE_CXXFLAGS_RELEASE=\n" QMODULE_PRI_CONTENT ${QMODULE_PRI_CONTENT}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file always \n
or could it be \r\n
?
Should we be asserting that this is at the beginning of a line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string(REGEX REPLACE "^QMAKE_CXXFLAGS_RELEASE\\+=[^\r\n]*[\r\n]" "QMAKE_CXXFLAGS_RELEASE=\r\n" QMODULE_PRI_CONTENT ${QMODULE_PRI_CONTENT})
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think you need to defend against it, just curious if we checked on a \r\n platform
It's also not reasonable to just ignore a user setting. If the user's triplet says 'build with these flags', qt needs to be built with those flags. #29052 breaks an explicitly documented knob. |
Yes, of cause. #29052 does not break it. The triplet settings are respected and verified in the build process of qt5-base. The issue is that Qt stores the results in qmodule.pri and re-uses if for other modules. If we, like proposed here delete the stored flags, and apply the triplet flags again. The verification step of qt-base is missing. This still leads to double flags and breaks cross builds. Fixing the approach here likely leads to ripping of the Qt build system. I don't think that we have a real world problem that rectifies the works. But anyway, I have no interest to spend more work here, because with Mixxx, I have cross build of QT5 working as an overlay. |
@@ -123,4 +123,11 @@ function(qt_build_submodule SOURCE_PATH) | |||
endif() | |||
endif() | |||
|
|||
# Remove duplicate flags from qmodule.pri issue -> https://github.com/microsoft/vcpkg/issues/28835 | |||
if(EXISTS "${CURRENT_INSTALLED_DIR}/tools/qt5/mkspecs/qmodule.pri") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this exists check.. exist? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's safe to assume that qmodule.pri
will exist. :)
# Conflicts: # ports/qt5-base/vcpkg.json # versions/baseline.json # versions/q-/qt5-base.json
Attempts to resolve this #28835 by removing duplicate flags from
qmodule.pri
Hi @daschuer, I don't have a way to repro your issue, but please feel free to try out these changes.