qt-build-utils can invoke qmake when it is a script #1284
Conversation
|
I only tested this so far on the "happy case" on Windows (that is to say: when using a qmake .bat wrapper). Will test on other platforms too. |
|
I marked as ready for review. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1284 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 75 75
Lines 12772 12772
=========================================
Hits 12772 12772 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SanderVocke
force-pushed
the
support_qmake_scripts
branch
4 times, most recently
from
f08b25c to
e4ef4fa
Compare
SanderVocke
force-pushed
the
support_qmake_scripts
branch
from
e4ef4fa to
031f080
Compare
|
In the refactored qt-build-utils, the change was a lot more straightforward. |
|
@ahayzen-kdab is this still on your radar? |
| /// Wrap a command in a native subshell | ||
| pub(crate) fn native_shell_command(command: &str) -> Command { | ||
| let mut result: Command; | ||
| if cfg!(target_os = "windows") { |
There was a problem hiding this comment.
can we do this at build time with #[cfg(...)] rather than at runtime with cfg! ?
There was a problem hiding this comment.
@ahayzen-kdab I actually prefer sticking with cfg! here.
AFAIK, the cfg! macro evaluates to a bool literal.
This should allow the compiler to optimize out the branches that are not used by the current cfg, but will still check that the alternative branches compile.
With #[cfg()] if you compile on Linux you will not notice if the windows part of the branch has an error that could be caught at compile-time.
There was a problem hiding this comment.
I guess, just elsewhere where we have similar things they are at compile time and in theory CI picks up any Windows failures :-) But sure we can keep it this way for now...
There was a problem hiding this comment.
Hi, @SanderVocke, overall I'm in favor of adding support for this, especially for the Windows case, where you can't just make a script an executable.
I'm somewhat concerned about possible regressions to invoking plain qmake binaries (especially on Unix where it is already possible to just make any script an executable).
We definitely need to be careful around handling of spaces, as I think that is now broken.
Maybe a good way to handle this could be:
Only wrap the executable in a native shell command on Windows, and only if it matches the .bat extension.
Checking the file extension would also allow us to add support for e.g. powershell instead of cmd via checking for the .ps1 extension.
Do you think this would fit the use case on Windows? As I think on Windows we can rely on these extensions to be there, which is not necessarily the case on Linux.
| @@ -14,3 +16,16 @@ pub(crate) fn is_apple_target() -> bool { | |||
| pub(crate) fn is_emscripten_target() -> bool { | |||
| std::env::var("CARGO_CFG_TARGET_OS") == Ok("emscripten".to_owned()) | |||
| } | |||
|
|
|||
| /// Wrap a command in a native subshell | |||
| pub(crate) fn native_shell_command(command: &str) -> Command { | |||
There was a problem hiding this comment.
I believe passing the command as a plain string here will fail if the path to qmake contains a space.
As this path is provided by the user, we must ensure this works as well.
Please make sure that we can handle paths with spaces.
I suggest adding a testcase for this by adding a test scripts/ folder that contains a qmake.sh and qmake.bat script that just echo a string or forward to qmake.
| if cfg!(target_os = "windows") { | ||
| result = Command::new("cmd"); | ||
| result.args(["/C", command]); | ||
| } else { |
There was a problem hiding this comment.
Please use an else if that actively lists the operating systems that we expect to have an sh binary and fall back to a plain Command in the other cases, as otherwise this will break on any OS that does not have an sh binary.
Ensure qmake is called either through
cmd(Windows) orsh(Linux/MacOS).The main change is this invocation. The rest of the code change is: