jurplel / install-qt-action

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

target: android failing with v3 #161

Closed m-kuhn closed 1 year ago

m-kuhn commented 1 year ago
      - name: 💐 Install Qt
        uses: jurplel/install-qt-action@v3
        with:
          target: android
/opt/hostedtoolcache/Python/3.10.6/x64/bin/python3 -m aqt install-qt linux android 5.15.2 android_armv7 --outputdir /home/runner/work/Qt
aqtinstall(aqt) v2.1.0 on Python 3.10.6 [CPython GCC 9.4.0]
The packages ['qt_base'] were not found while parsing XML of package information!
==============================Suggested follow-up:==============================
* Please use 'aqt list-qt linux android --arch 5.15.2' to show architectures available.
* Please use 'aqt list-qt linux android --modules 5.15.2 <arch>' to show modules available.
Error: Error: The process '/opt/hostedtoolcache/Python/3.10.6/x64/bin/python3' failed with exit code 1

Likely caused by: https://github.com/jurplel/install-qt-action/commit/8358dd9cdba87272a800a5567000bdc8cc647dd6#diff-ad2b60fd0fe67776e1dcc7801bcaeed4934823e3103a9d2ae656073002304eb1R115-R117

The following command works in a local test

python3 -m aqt install-qt linux android 5.15.2 android --outputdir qt
m-kuhn commented 1 year ago

Specifying arch helps

      - name: 💐 Install Qt
        uses: jurplel/install-qt-action@v3
        with:
          target: android
          arch: android
ddalcino commented 1 year ago

Likely caused by: 8358dd9#diff-ad2b60fd0fe67776e1dcc7801bcaeed4934823e3103a9d2ae656073002304eb1R115-R117

Yes, you are correct. The offending code is here:

https://github.com/jurplel/install-qt-action/blob/2b22cf195fa48dc1e494764aa3327355adee76eb/action/src/main.ts#L103-L105

The architecture android_armv7 exists for most versions of Qt, but it does not exist for Qt 5.15. and 5.14.; for these versions, the only valid architecture is android. There may be other exceptions that I am not aware of. If you want to find the rest of them, you can try using aqt list-qt, as suggested by the error message above.

m-kuhn commented 1 year ago

Does it make sense to fall back to android_armv7? Or would it be more useful just to throw a message that "arch" is missing and needs to be specified?

ddalcino commented 1 year ago

Good question. I would lean towards using android for the special cases of Qt 5.14 & 5.15, and android_armv7 for all other cases. However, you could make the argument that choice of architecture is so important to get right that all builds should fail if it is left unspecified.

I could be wrong, but I thought that if you’re trying to put an Android app on the app store, you need to bundle binaries for all architectures into your APK. Maybe the right answer is to install every architecture available?

m-kuhn commented 1 year ago

I think you need at least 64 bit arches (i know you cannot ship armv7 alone, possibly you can ship arm64 alone). Installing all would be an option too. That's what happens for 5.14 and 5.15 too.

ddalcino commented 1 year ago

On second thought, maybe it would be more appropriate to have the action run aqt list-qt to find out what architectures are available, and pick something from that list.

There’s definitely a use case for installing all of the architectures, or a handful of architectures, but it probably should not be the default case. It could be problematic to have the defaults use so much unnecessary net traffic and CPU time.

m-kuhn commented 1 year ago

On second thought, maybe it would be more appropriate to have the action run aqt list-qt to find out what architectures are available, and pick something from that list.

How would you choose what to pick?

ddalcino commented 1 year ago

How would you choose what to pick?

aqt list-qt <host> <target> --arch <version> prints a list of architectures, separated by spaces, directly to stdout. I would run the command, split on spaces, filter out empty strings, and choose based on what comes back:

This decision tree should give you a usable default value in most cases, without surprising anyone with novel behavior. The one case where it would not work is if the download.qt.io servers are down, in which case this action is not going to work anyway.

m-kuhn commented 1 year ago

Hmm... I may be missing some historical reasons, but I would be surprised to get armv7 by default. What I do is build for 4 arches in a matrix. One of them would "randomly" build. For me either installing all or failing with instructions what's missing would be most intuitive.

ddalcino commented 1 year ago

For android, armv7 has been the default architecture at least since this commit: https://github.com/jurplel/install-qt-action/commit/dc5462a6bf9b7f8a1825ea4328b4844b679a76fd That was two years ago. I would expect that anyone using this action for a long time would be surprised if the default behavior changed without warning. I thought we were talking about a bug-fix, not a breaking interface change.

For me either installing all or failing with instructions what's missing would be most intuitive.

I agree that these behaviors are more intuitive than just choosing armv7, and are good ideas for a future version. Maybe this could be part of v4?

m-kuhn commented 1 year ago

Good point. The "bug" in this case would be for 5.14 / 5.15, where your proposal would fix it back to the old behaviour and the adjustment I was talking about could be for v4 :+1: .

jurplel commented 1 year ago

I think for now I will just check for qt >= 5.14 and have it use android in that case. I think the approach using list-qt is valuable but it kind of implies other changes and I think the simplest approach is good enough for the present.

ddalcino commented 1 year ago

https://github.com/jurplel/install-qt-action/blob/c8ebe28639c7adaa809f4902266be0ce9a433e44/action/src/main.ts#L104

I think this is the wrong version comparison. android is a valid architecture for Qt 5.14.0 through 5.15.2, but not for Qt 6.0.0+. android_armv7 is still a valid architecture for all versions of Qt since 6.0.0.

- if (compareVersions(this.version, ">=", "5.14.0")) { 
+ if (compareVersions(this.version, ">=", "5.14.0") && compareVersions(this.version, "<", "6.0.0")) { 
jurplel commented 1 year ago

Oh, I misunderstood! Thank you!