jurplel / install-qt-action

Install Qt on your Github Actions workflows with just one simple action
MIT License
478 stars 83 forks source link

Bad QT_ROOT_DIR on Windows #259

Open p0358 opened 3 weeks ago

p0358 commented 3 weeks ago
    QT_ROOT_DIR: D:\Qt\6.8.0\msvc2022_64\bin\qmake.exe
    QT_PLUGIN_PATH: D:\Qt\6.8.0\msvc2022_64\bin\qmake.exe\plugins
    QML2_IMPORT_PATH: D:\Qt\6.8.0\msvc2022_64\bin\qmake.exe\qml

It seems to be because of this (introduced in #173): https://github.com/jurplel/install-qt-action/blob/08a6bd0b9fa49af9090ec35e68ce61dea976ed9f/action/src/main.ts#L75-L80

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 readable

p0358 commented 3 weeks ago

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, ""));
pzhlkj6612 commented 3 weeks ago

Can we standardize the path (i.e., always use slashes) during processing instead of playing with regex?

p0358 commented 3 weeks ago

Hmm, maybe cleaner than regex would be to do: .map((s) => path.resolve(s, "../../"));

always use slashes

Well, at least glob always requires slashes as input, but default output is what's standard for given OS.

posix Set to true to use / as the path separator in returned results. On posix systems, this has no effect. On Windows systems, this will return / delimited path results, and absolute paths will be returned in their full resolved UNC path form, eg insted of 'C:\\foo\\bar', it will return //?/C:/foo/bar.

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...

ddalcino commented 2 weeks ago

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.

jurplel commented 1 week ago

I tried to write a test to reproduce it, but I wasn't able to: https://github.com/jurplel/install-qt-action/blob/jurplel/fix-qt-root-dir/action/tests/locateQtArchDir.test.ts

Any advice?

p0358 commented 1 week ago

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

obraz

Unless something changed between @v4 tag from that time and master that changed the behavior by some coincidence?

jurplel commented 1 week ago

v4 branch was just updated to match master, so i guess let me know if the issue still occurs

p0358 commented 6 days ago

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

ddalcino commented 6 days ago

Have you tried these lines here? I think vcvars64.bat makes a big difference in setting up CMake to run properly on Windows. You'd need to run it in cmd or powershell rather than bash, though. https://github.com/jurplel/install-qt-action/blob/5d50465bcd509eb41c9bc309cd4ed800e0a80e82/.github/workflows/test.yml#L150-L152

p0358 commented 6 days ago

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...?

ddalcino commented 6 days ago

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.

I mean I think they run it already?

Not by default.

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.

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.

ddalcino commented 5 days ago

@p0358 you may be interested to know that if you remove the line dir: '/' from your workflow, the QT_ROOT_DIR is no longer broken, and your workflow completes successfully: https://github.com/ddalcino/UnCSO2/actions/runs/11758620357/job/32757179719

I'm not sure why this results in QT_ROOT_DIR being broken. I'm not so interested in investigating further; / is a dangerous place to try to install anything to begin with.

I've tried setting dir: '/' in the install-qt-action workflow, and I get lots of errors that prevent Qt installation at all: https://github.com/ddalcino/install-qt-action/actions/runs/11758682208/job/32757320056 The action tries to install at \\Qt or //Qt, which is either not user-writable, or subject to Error: Error: UNKNOWN: unknown error, stat '\\Qt\6.8.0\'.

p0358 commented 4 days ago

@p0358 you may be interested to know that if you remove the line dir: '/' from your workflow, the QT_ROOT_DIR is no longer broken, and your workflow completes successfully: https://github.com/ddalcino/UnCSO2/actions/runs/11758620357/job/32757179719

Oh, this is a very good discovery, I'll keep poking around this.

I'm not sure why this results in QT_ROOT_DIR being broken. I'm not so interested in investigating further; / is a dangerous place to try to install anything to begin with.

I kinda am ngl, since this is some witchcraft, so it would feel wack to just leave it at that. Plus / aka C:/ aka C:\Qt is the default path of Qt installation on Windows, so I can't agree with that statement. Plus it's an easily predictable path, and any user can create paths under that by default. Although on GitHub Actions it's gonna resolve to D:\...

I've tried setting dir: '/' in the install-qt-action workflow, and I get lots of errors that prevent Qt installation at all: https://github.com/ddalcino/install-qt-action/actions/runs/11758682208/job/32757320056 The action tries to install at \Qt or //Qt, which is either not user-writable, or subject to Error: Error: UNKNOWN: unknown error, stat '\Qt\6.8.0\'.

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 this.dir =${dir}/Qt; that got resolved to //Qt and then thus \\Qt, which maybe somewhere got interpreted as UNC path then somewhere? But generally on Windows / should resolve to just C:\ (unless it resolves to current drive, I'm not honestly sure).

But I see one possible reason now, but gotta test it yet, it seems that glob 9.0 introduced this:

Paths are returned in the canonical formatting for the platform in question.

Tests are run with 7.2.3, based on action/package-lock.json and that it uses npm ci to install, but packaged action might be using latest? I'll have to play around with outdated glob again to see if I can reproduce it...

p0358 commented 4 days ago

There we go, glob 7.2.3:

> console.log(glob.default.sync("C:/Qt/[0-9]*/*/bin/qmake*"))
[
  'C:/Qt/5.13.0/msvc2017_64/bin/qmake.exe',
  'C:/Qt/5.15.2/msvc2019_64/bin/qmake.exe',
  'C:/Qt/6.8.0/msvc2022_64/bin/qmake.exe',
  'C:/Qt/6.8.0/msvc2022_64/bin/qmake6.exe'
]
undefined
> console.log(glob.default.sync("/Qt/[0-9]*/*/bin/qmake*"))
[
  'C:\\Qt\\5.13.0\\msvc2017_64\\bin\\qmake.exe',
  'C:\\Qt\\5.15.2\\msvc2019_64\\bin\\qmake.exe',
  'C:\\Qt\\6.8.0\\msvc2022_64\\bin\\qmake.exe',
  'C:\\Qt\\6.8.0\\msvc2022_64\\bin\\qmake6.exe'
]

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 dir it works on GitHub Actions, since it defaults to the current running workflow dir env var, which is set to an absolute path within D:

@jurplel I guess reproduction test could just use / as the install dir to trigger it before any fixes then. And then also add a condition to ensure that the resulting path on Windows always ends up with \\ separators and then also that it doesn't end with qmake.exe both.

pzhlkj6612 commented 3 days ago

Plus / aka C:/ aka C:\Qt is the default path of Qt installation on Windows, so I can't agree with that statement. Plus it's an easily predictable path, and any user can create paths under that by default. Although on GitHub Actions it's gonna resolve to D:\...

This:

PS C:\> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.22621.4249
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.22621.4249
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

PS C:\> cd '/System Volume Information'

PS C:\System Volume Information> D:

PS D:\> cd '/System Volume Information'

PS D:\System Volume Information>

Ref: how to change directory using Windows command line - Stack Overflow.

ddalcino commented 3 days ago

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 dir: '/', host: 'windows-latest' was sufficient to reproduce your CMake error almost exactly; see the test runs linked in the comment for #266.