-
Notifications
You must be signed in to change notification settings - Fork 89
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
Bad QT_ROOT_DIR on Windows #259
Comments
Sharing a lazy workaround for the issue in case it helps anyone (it did fix the CMake erroring out about inability to locate Qt): - name: Fix env vars if needed
uses: actions/github-script@v7
with:
script: |
if (process.env.QT_ROOT_DIR) {
core.exportVariable('QT_ROOT_DIR', process.env.QT_ROOT_DIR.replace(/[/\\]bin[/\\]qmake[^/\\]*/i, ""));
core.exportVariable('Qt6_DIR', process.env.QT_ROOT_DIR + "/lib/cmake/Qt6");
core.addPath(process.env.QT_ROOT_DIR + "/bin");
}
if (process.env.QT_PLUGIN_PATH)
core.exportVariable('QT_PLUGIN_PATH', process.env.QT_PLUGIN_PATH.replace(/[/\\]bin[/\\]qmake[^/\\]*/i, ""));
if (process.env.QML2_IMPORT_PATH)
core.exportVariable('QML2_IMPORT_PATH', process.env.QML2_IMPORT_PATH.replace(/[/\\]bin[/\\]qmake[^/\\]*/i, "")); |
Can we standardize the path (i.e., always use slashes) during processing instead of playing with regex? |
Hmm, maybe cleaner than regex would be to do:
Well, at least glob always requires slashes as input, but default output is what's standard for given OS.
Well the UNC part kinda sucks. I'd say just keep it system-native. It's not really a problem of paths using bashslashes, but a broken regex on a string that assumed they wouldn't... |
Whoops, I wrote that bug; sorry about that. Guess I forgot about Windows there. To whoever winds up fixing this: I strongly suggest that you add a regression test for this. This is the kind of bug that is very easy to re-introduce during a refactor. |
I tried to write a test to reproduce it, but I wasn't able to: Any advice? |
I'm not sure... I can paste my workflow run of github actions to show that I'm not crazy that it happens https://github.com/harmonytf/UnCSO2/actions/runs/11509181511/job/32038804019 Unless something changed between |
v4 branch was just updated to match master, so i guess let me know if the issue still occurs |
seems still borked: https://github.com/harmonytf/UnCSO2/actions/runs/11744517999/job/32719812809 I have no idea why the tests on this repo don't reproduce here, could be some intricacy about the environment somewhere |
Have you tried these lines here? I think install-qt-action/.github/workflows/test.yml Lines 150 to 152 in 5d50465
|
I mean I think they run it already? Generally CMake works just fine, as do MSBuild and cl.exe, but if you expand the command in the run, you'll see the env vars passed related to Qt are invalid. And just fixing these vars made it compile flawlessly. And the fact that \bin\qmake.exe is at the end points towards that piece of code mentioned at the top as probably the only possible direct culprit. The only way it doesn't happen in here is if somehow glob here returns Unix paths rather than Windows paths I think...? |
There are two major differences between they way your workflow is set up and the way install-qt-action is set up:
If you want to find out why your test runs don't match install-qt-action's, then you need to control for these two conditions.
Not by default.
Correct. The vars are definitely broken, both in install-qt-action workflows and your own. Despite this, the install-qt-action workflows are able to locate Qt properly; yours are not. |
@p0358 you may be interested to know that if you remove the line I'm not sure why this results in QT_ROOT_DIR being broken. I'm not so interested in investigating further; I've tried setting |
Oh, this is a very good discovery, I'll keep poking around this.
I kinda am ngl, since this is some witchcraft, so it would feel wack to just leave it at that. Plus
This is really weird now, since you have an error there on Windows, despite that alone working for me perfectly fine. This means something is REALLY weird with the environment between the actions, it shouldn't be inconsistent like this. I mean it did partially install at least, but then perhaps it's But I see one possible reason now, but gotta test it yet, it seems that glob 9.0 introduced this:
Tests are run with 7.2.3, based on action/package-lock.json and that it uses |
There we go, glob 7.2.3:
solution: update glob version and fix the bug that got masked by its former inconsistent behavior and I couldn't repro it at first this way, since I used the latest installable glob... and of course without setting @jurplel I guess reproduction test could just use |
This:
Ref: how to change directory using Windows command line - Stack Overflow. |
#266 should fix this; it uses the standard Node path library for path manipulation, rather than ad-hoc string manipulations. Updating glob sounds like a good idea, but it wasn't necessary. FYI, setting |
It seems to be because of this (introduced in #173):
install-qt-action/action/src/main.ts
Lines 75 to 80 in 08a6bd0
The glob detects as it should, but the regex is wrong, as it only matches forward slashes, but the path on windows has backslashes...
EDIT: With that said it seems like the tests are passing and path isn't wrong on this repo. But in my case on
@v4
version and custom install dir, this behavior was exhibited...EDIT 2: If fixing the regex, I'd probably also make it case insensitive, maybe do something like
s.replace(/[\/\\]bin[\/\\]qmake[^/\\]*$/i, "")
, seems to work if we test with:'D:\\Qt\\6.8.0\\msvc2022_64\\bin\\qmake.exe'.replace(/[\/\\]bin[\/\\]qmake[^/\\]*$/i, "") === "D:\\Qt\\6.8.0\\msvc2022_64"
EDIT 3: Actually why was forward slash escaped? Make that
.replace(/[/\\]bin[/\\]qmake[^/\\]*$/i, "")
, a bit more readableThe text was updated successfully, but these errors were encountered: