install-qt-action icon indicating copy to clipboard operation
install-qt-action copied to clipboard

target: android failing with v3

Open m-kuhn opened this issue 2 years ago • 1 comments

      - 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 avatar Aug 13 '22 09:08 m-kuhn

Specifying arch helps

      - name: 💐 Install Qt
        uses: jurplel/install-qt-action@v3
        with:
          target: android
          arch: android

m-kuhn avatar Aug 13 '22 10:08 m-kuhn

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.

ddalcino avatar Aug 20 '22 17:08 ddalcino

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?

m-kuhn avatar Aug 20 '22 17:08 m-kuhn

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?

ddalcino avatar Aug 20 '22 17:08 ddalcino

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.

m-kuhn avatar Aug 20 '22 18:08 m-kuhn

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.

ddalcino avatar Aug 20 '22 19:08 ddalcino

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?

m-kuhn avatar Aug 21 '22 06:08 m-kuhn

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:

  • If there are no strings in the output, raise an exception
  • If there's only one string, select that one
  • Else:
    • If any item in the list includes the substring armv7, choose the first such item. (if not for the backwards compatibility implications, I would choose arm64 instead)
    • If any item in the list includes the substring arm, choose the first such item
    • Otherwise, just return the first item in the list

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.

ddalcino avatar Aug 21 '22 22:08 ddalcino

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.

m-kuhn avatar Aug 21 '22 23:08 m-kuhn

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?

ddalcino avatar Aug 22 '22 00:08 ddalcino

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

m-kuhn avatar Aug 22 '22 11:08 m-kuhn

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.

jurplel avatar Aug 24 '22 21:08 jurplel

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")) { 

ddalcino avatar Aug 25 '22 04:08 ddalcino

Oh, I misunderstood! Thank you!

jurplel avatar Aug 25 '22 04:08 jurplel