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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

target: android failing with v3 #161

Closed
m-kuhn opened this issue Aug 13, 2022 · 14 comments
Closed

target: android failing with v3 #161

m-kuhn opened this issue Aug 13, 2022 · 14 comments
Labels
enhancement New feature or request

Comments

@m-kuhn
Copy link

m-kuhn commented Aug 13, 2022

      - 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: 8358dd9#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 m-kuhn changed the title android failing with v3 target: android failing with v3 Aug 13, 2022
@m-kuhn
Copy link
Author

m-kuhn commented Aug 13, 2022

Specifying arch helps

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

@ddalcino
Copy link
Contributor

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

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

} else if (this.target === "android") {
this.arch = "android_armv7";
}

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
Copy link
Author

m-kuhn commented Aug 20, 2022

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
Copy link
Contributor

ddalcino commented Aug 20, 2022

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鈥檙e 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
Copy link
Author

m-kuhn commented Aug 20, 2022

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
Copy link
Contributor

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鈥檚 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
Copy link
Author

m-kuhn commented Aug 21, 2022

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
Copy link
Contributor

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.

@m-kuhn
Copy link
Author

m-kuhn commented Aug 21, 2022

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
Copy link
Contributor

For android, armv7 has been the default architecture at least since this commit: dc5462a 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
Copy link
Author

m-kuhn commented Aug 22, 2022

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

@jurplel
Copy link
Owner

jurplel commented Aug 24, 2022

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 added a commit that referenced this issue Aug 24, 2022
@jurplel jurplel added the enhancement New feature or request label Aug 25, 2022
@jurplel jurplel closed this as completed Aug 25, 2022
@ddalcino
Copy link
Contributor

if (compareVersions(this.version, ">=", "5.14.0")) {

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

fredowski added a commit to fredowski/qgroundcontrol that referenced this issue Aug 25, 2022
@jurplel
Copy link
Owner

jurplel commented Aug 25, 2022

Oh, I misunderstood! Thank you!

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

No branches or pull requests

3 participants