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

[scripts-audit] QMake buildsystem #20322

Merged
merged 21 commits into from
Nov 5, 2021

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Sep 24, 2021

Audit the QMake build system according to the documentation.

Related: #17691

@JackBoosY JackBoosY added info:internal This PR or Issue was filed by the vcpkg team. category:scripts-audit Part of the scripts audit effort. labels Sep 24, 2021
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Just when I submitted a PR touching one of these files...

@JackBoosY JackBoosY marked this pull request as ready for review September 27, 2021 10:01
file(COPY ${RELEASE_LIBS} DESTINATION ${CURRENT_PACKAGES_DIR}/lib)
if(release_libs)
file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/lib")
file(COPY ${release_libs} DESTINATION "${CURRENT_PACKAGES_DIR}/lib")
Copy link
Contributor Author

@JackBoosY JackBoosY Sep 27, 2021

Choose a reason for hiding this comment

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

release_libs cannot use double quotes because it may be a list, and file(COPY) will treat release_libs as one item.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

started reviewing :)

The options passed to qmake to the configure step.

### BUILD\_OPTIONS, BUILD\_OPTIONS\_RELEASE, BUILD\_OPTIONS\_DEBUG
The options passed to qmake to the build step.
Copy link
Contributor

Choose a reason for hiding this comment

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

This may have been the intention by name, but does it make sense for qmake? There is no "build step" in vcpkg_configure_qmake and also no documented effect of -- in https://doc.qt.io/qt-5/qmake-manual.html.
CC @Neumann-A

Copy link
Contributor

Choose a reason for hiding this comment

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

It just means non parametric options after the -- . BUILD_OPTIONS might be the wrong name here and it was a bit influenced by qt5-base bootstrap script where it is needed to pass other variables to the next build/configure step. BUILD_OPTIONS is used in in e.g. qt5-webengine.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

some stuff to change :)

@Osyotr
Copy link
Contributor

Osyotr commented Oct 20, 2021

Does vcpkg_configure_qmake respect VCPKG_C/CXX/LINKER_FLAGS triplet variables?
I was trying to build qt5-base with set(VCPKG_LINKER_FLAGS "-Wl,-rpath,'$ORIGIN':'$ORIGIN/../lib'") and resulting binaries did not contain anything related to rpath, so I am wondering whether it's qt5-base that ignores these flags or vcpkg_configure_qmake.

@strega-nil-ms
Copy link
Contributor

@Osyotr probably vcpkg_configure_qmake.

@Neumann-A
Copy link
Contributor

@Osyotr neither respect the cmake toolchain. Qt5-base runs a bootstrap script and the flags are not passed and the rest uses the script here

@dg0yt
Copy link
Contributor

dg0yt commented Oct 21, 2021

@Osyotr Related work started in #20309, for enabling mingw builds. Now it is effectively blocked by this scripts-audit, due to unavoidable merge conflicts (and the lack of mingw CI).

@Osyotr
Copy link
Contributor

Osyotr commented Oct 21, 2021

Turns out qt5-base has its own relocating mechanism and the only binary that has no rpath is libQt5Core.so. Others have $ORIGIN, tools have $ORIGIN/../../../lib.
So, in order to workaround that one can remove core the check here

@JackBoosY
Copy link
Contributor Author

Turns out qt5-base has its own relocating mechanism and the only binary that has no rpath is libQt5Core.so. Others have $ORIGIN, tools have $ORIGIN/../../../lib. So, in order to workaround that one can remove core the check here

I can do nothing about this part because I'm not familiar with qmake. I wish anyone could help me to implement it.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

Continue marking changes to make

@@ -8,102 +8,79 @@ vcpkg_build_qmake()
```
#]===]

function(z_run_jom_build targets log_prefix log_suffix)
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 I mentioned this before:
This function uses jom only on a single platform so the name isn't good fit.
I also wonder if invoke_command should become a formal parameter - or a global, upper-case variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this function has the prefix z_, it will only be used in this file, so I think this is also acceptable.
invoke_command is not a tool, so I think it should be lowercase.

Copy link
Contributor

Choose a reason for hiding this comment

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

z_run_jom_build is exposed in the stack trace on port build failures. In this situation, it is inviting to be misunderstood as "Oh no, jom is wrong on POSIX, vcpkg [!] broke my port again."

I know invoke_command is a variable. I complain that it is passed to z_run_jom_build by context, not as a proper formal parameter. Ideally, a function depends only on its input parameters, and has no side effects.

@JackBoosY
Copy link
Contributor Author

Ping @strega-nil-ms for review again.

@strega-nil-ms strega-nil-ms merged commit 66c39e1 into microsoft:master Nov 5, 2021
@JackBoosY JackBoosY deleted the dev/jack/script-audit-qmake branch November 8, 2021 01:59
@daschuer daschuer mentioned this pull request Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:scripts-audit Part of the scripts audit effort. info:internal This PR or Issue was filed by the vcpkg team. requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants