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

Use path library for path manipulation #266

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ddalcino
Copy link
Contributor

@ddalcino ddalcino commented Nov 11, 2024

Fix #259.

Currently, this action relies heavily on string manipulation techniques to compose paths. This approach is prone to error, particularly in the context of Windows and inconsistent use of native path delimiters. This PR uses the standard Node path library to manipulate paths.

ddalcino and others added 3 commits November 11, 2024 12:58
Previously, this action relied heavily on string manipulation techniques
to compose paths. This approach is prone to error, particularly in the
context of Windows and inconsistent use of native path delimiters.
@p0358
Copy link

p0358 commented Nov 11, 2024

This is definitely a big improvement and the right path. But I can't help but point out that passing output of path.join to nativePath aka path.normalize is pretty redundant now, since the output of path.join is already normalized. Note that this is not the same as path.resolve for example, but that one is not used...

Btw if poking at paths already, I think that this: const nativePath = process.platform === "win32" ? path.win32.normalize : path.normalize; is also redundant. Afaik Path API is gonna already behave with win32 behavior on win32 platform, I think this path.win32 implementations are explicitly only exposed if win32-specific behavior is desired for example during tests, otherwise it's kinda unnecessary, while making it a bit more cryptic what "nativePath" is, compared to just using path.normalize directly...

@jurplel
Copy link
Owner

jurplel commented Nov 11, 2024

If there's still value in unit testing feel free to pull in the jurplel/fix-qt-root-dir changes. It looks like it is covered in CI now, though.

Looks like the failed test is just flakiness?

@ddalcino
Copy link
Contributor Author

ddalcino commented Nov 12, 2024

If there's still value in unit testing feel free to pull in the jurplel/fix-qt-root-dir changes. It looks like it is covered in CI now, though.

I think unit testing is a really good idea, but I'd like to keep that separate from this PR for now.

Looks like the failed test is just flakiness?

The failed test does not appear related to the changes in this PR. Relevant code:

if ([version]$env:QT_VERSION -ge [version]"6.5.3") {
# GitHub macOS 13/14 runners use Xcode 15.0.x by default which has a known linker issue causing crashes if the artifact is run on macOS <= 12
sudo xcode-select --switch /Applications/Xcode_15.2.app
} else {
# Keep older Qt versions on Xcode 14 due to concern over QTBUG-117484
sudo xcode-select --switch /Applications/Xcode_14.3.1.app
}

The failed runs indicate that on some MacOS test runners, /Applications/Xcode_14.3.1.app does not exist:

xcode-select: error: invalid developer directory '/Applications/Xcode_14.3.1.app'

Maybe we could get around it with a glob star, like sudo xcode-select --switch /Applications/Xcode_14.*.app? That could get messy if it picks the wrong one, or the glob star returns multiple apps.

According to the bug report, this was fixed in Qt 6.5.3, so maybe the fix is no longer needed? [edit] no, that doesn't work for older versions of Qt ...

`path.resolve` joins and normalizes paths, returning an absolute path.
We want all environment variables to be set to absolute paths, with all
directory separators fixed and root `/` turned to `C:\\` or `D:\\` on
Windows. `path.resolve` does that.

I thought it was important to use `path.resolve` after any use of
`glob.sync`, since those necessarily introduce new `/` into the paths.
I left two instances of `path.join` in places where I thought the
absolute path didn't matter, because they are either not used to build a
path (in `locateQtArchDir`) or upstream of a `path.resolve`
(in `toolsPaths`). Perhaps this is the wrong approach, and will lead to
future inconsistency; perhaps not. Hopefully the tests will be enough.
@jdpurcell
Copy link

That Xcode failure is related to actions/runner-images#10703 and I don't think it's possible to build with Qt < 6.5.3 on GitHub Actions with the newer runners (macOS 14/15) anymore; only macOS 13 and the soon-to-be-retired macOS 12 runners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad QT_ROOT_DIR on Windows
4 participants