[scripts-audit] QMake buildsystem#20322
Conversation
dg0yt
left a comment
There was a problem hiding this comment.
Just when I submitted a PR touching one of these files...
| 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") |
There was a problem hiding this comment.
release_libs cannot use double quotes because it may be a list, and file(COPY) will treat release_libs as one item.
strega-nil-ms
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
strega-nil-ms
left a comment
There was a problem hiding this comment.
some stuff to change :)
|
Does |
|
@Osyotr probably |
|
@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 |
|
Turns out qt5-base has its own relocating mechanism and the only binary that has no rpath is |
…jack/script-audit-qmake
I can do nothing about this part because I'm not familiar with qmake. I wish anyone could help me to implement it. |
strega-nil-ms
left a comment
There was a problem hiding this comment.
Continue marking changes to make
| ``` | ||
| #]===] | ||
|
|
||
| function(z_run_jom_build targets log_prefix log_suffix) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…jack/script-audit-qmake
…BoosY/vcpkg into dev/jack/script-audit-qmake
|
Ping @strega-nil-ms for review again. |
Audit the QMake build system according to the documentation.
Related: #17691