search

jurplel / install-qt-action

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

Add the tool's specific bin path when installing tools #157

Closed asztalosdani closed 9 months ago

asztalosdani commented 1 year ago

Using

- name: Install Qt
  uses: jurplel/install-qt-action@v2
  with:
    version: '5.15.2'
    tools: 'tools_ifw,4.4.1,qt.tools.ifw.44'
    tools-only: 'true'
    cache: true

I successfully installed the Qt Installer Framework. It set the IQTA_TOOLS environment variable, but it points to %GITHUB_WORKSPACE%/Qt/Tools, so trying to run e.g. binarycreator.exe in a subsequent step results in an error because it could not be found, because it is installed in %GITHUB_WORKSPACE%/Qt/Tools/QtInstallerFramework/4.4/bin.

Can we have the tool's bin path added to the environment variable?

jurplel commented 1 year ago

Is there a pattern that all tools conform to (i.e. would adding %GITHUB_WORKSPACE%/Qt/Tools/*/*/bin work for all *)?

asztalosdani commented 1 year ago

Unfortunately not... E.g. QtCreator's bin is %GITHUB_WORKSPACE%/Qt/Tools/QtCreator/bin. So this would need per-tool or at least per-tool-group treatment, which feels fishy... One thing can be done maybe, checking if %GITHUB_WORKSPACE%/Qt/Tools/{currently-installed-tool}/bin exists and adding it to the path, otherwise checking one folder deeper. Does this sound like something you would want in this tool? Do you have a better idea?

asztalosdani commented 1 year ago

Using glob and %GITHUB_WORKSPACE%/Qt/Tools/**/bin pattern would yield all bin folders in any level. At least that is the case in python, should be achievable in ts too.

import glob

for item in glob.glob(r"d:/temp/aqt_temp/Tools/**/bin", recursive=True):
    print(item)

If it is not available in ts, adding all bin folders 1 and 2 levels deep explicitly would do the trick.

ddalcino commented 1 year ago

If it is not available in ts, adding all bin folders 1 and 2 levels deep explicitly would do the trick.

This functionality is available in ts:

import * as glob from "glob";

console.log(glob.sync(`${install_dir}/Tools/**/bin`))  // prints a list of absolute paths

If you want to try it interactively in your local node REPL, imports work differently, so you could do it like this:

> const glob = (await import("glob")).default;

> glob.sync(`${install_dir}/Tools/**/bin`)

Caveat:

This approach appears to work well for many tools on Windows and Linux, but there are some exceptions on Mac.

When I install tools_qtcreator for desktop linux, glob.sync("install_dir/Tools/**/bin") returns QtCreator/bin, QtCreator/lib/Qt/bin, QtCreator/libexec/qtcreator/clang/bin, and 6 more directories in QtCreator/src. That's great.

When I install tools_qtcreator for desktop mac, glob.sync("install_dir/Tools/**/bin") returns an empty list. The trouble is, on Mac, tools_qtcreator installs two folders parallel to the Tools directory, Qt Creator.dSYM and an Mac app bundle called Qt Creator.app, not inside Tools. Investigating these directories, I can observe the following:

I know that tools_qtcreator is not the only Mac tool like this; I'm pretty sure that tools_maintenance ships in a Mac app bundle as well. I have not exhaustively tested every single tool for every platform that aqtinstall supports. Maybe these two tools are the only exceptions here; maybe not.

Opinion:

I like the idea of adding glob.sync("install_dir/Tools/**/bin") to the path, but I worry this could cause more problems than it solves. If a user installs tools_qtcreator and doesn't opt out, they will be adding 9 bin directories on Linux and none on Mac. On Win/Linux, if they call a binary, they could easily be calling the wrong one. The QtCreator/libexec/qtcreator/clang/bin directory could easily override other clang tools that the user has installed.

IMHO this kind of thing should be opt-in only, and added to the end of the path variable rather than the front.

asztalosdani commented 1 year ago

Thanks for the info.

This approach appears to work well for many tools on Windows and Linux, but there are some exceptions on Mac.

Can we add the bin folder if we are on win/linux and do something else on mac for the app bundles?

IMHO this kind of thing should be opt-in only, and added to the end of the path variable rather than the front.

I agree with adding it to the end of the PATH, but not the opt-in part. I am installing the tools because I want to use them. I don't see a use case where I would just want to install them, but please correct me if I'm wrong.

ddalcino commented 1 year ago

Thanks for the info.

No problem; thanks for the response!

This approach appears to work well for many tools on Windows and Linux, but there are some exceptions on Mac.

Can we add the bin folder if we are on win/linux and do something else on mac for the app bundles?

That sounds reasonable. Right now, I'm thinking we could add these paths:

IMHO this kind of thing should be opt-in only, and added to the end of the path variable rather than the front.

I agree with adding it to the end of the PATH, but not the opt-in part. I am installing the tools because I want to use them. I don't see a use case where I would just want to install them, but please correct me if I'm wrong.

Point taken. I am mainly concerned about adding an unexpectedly large number of paths by default, which could make your shell environment behave unpredictably. On further investigation, maybe the problem is not as bad as I had thought.

I think it would be simple enough to implement this.

ddalcino commented 1 year ago

IMHO this kind of thing should be opt-in only, and added to the end of the path variable rather than the front.

Maybe I have been thinking about this the wrong way.

GitHub actions/core provides an easy and robust means of adding new items to the front of the PATH variable, but not the end. There is an issue to track this (https://github.com/actions/toolkit/issues/270), but it doesn’t seem to have any forward progress. I think there’s a good reason for that: In a GitHub action, modifications to the path are meant to be ordered, so that later modifications will override changes that came before. Your yml file would not make a lot of sense if an earlier step added things to the path that can override later steps. So, if I have a step that adds the wrong clang-format to my path (for example, install-qt-action), I should override it with the one I want in a later step.

I am trying to make the case here that we should be adding tool paths to the front of the path, rather than the end. Unfortunately, this would be a breaking change that can easily break users’ gh workflows, and will require a major version bump (3 to 4).

180 implements adding tools to the front of the path, not the end, because there does not appear to be a robust way to add to the end of the path variable. Every attempt I have made has broken something, usually on Windows, because paths work differently there. I could be wrong, but right now I am convinced that adding to the end of the path is a mistake.

I will wait for further comment on this issue before I move #180 out of draft status.