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

Only include Android.mk of selected gapps #222

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

Conversation

cwhuang
Copy link

@cwhuang cwhuang commented Jul 2, 2019

Some apps don't exist in all archs. Only include makefiles of selected
apps to avoid such errors:

[570/645] including vendor/opengapps/build/modules/ActionsServices/Android.mk ...
error: ActionsServices: No source files specified

Some apps don't exist in all archs. Only include makefiles of selected
apps to avoid such errors:

[570/645] including vendor/opengapps/build/modules/ActionsServices/Android.mk ...
error: ActionsServices: No source files specified
To avoid conflicts between opengapps and AOSP's default apps.
Note the clean steps are only executed once.
@@ -0,0 +1,2 @@
$(call add-clean-step, rm -rf $(PRODUCT_OUT)/system/app/*)
$(call add-clean-step, rm -rf $(PRODUCT_OUT)/system/priv-app/*)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these clean rules seem too broad.

what is the use-case where they are needed?

if you already have an aosp build completed and then decide to add opengapps to your build, then I think you can just run make installclean and the system-app and system-priv-app will be cleaned up.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use-case: make an incremental build after adding repos of opengapps

Of course manually doing make installclean would solve that.
But the CleanStep.mk is faster and smarter. It just removes apps in /system instead of whole /system. Besides, people usually forget them need to do so (after adding opengapps) that results in a broken build. The CleanStep.mk automates it.

Note this is the standard way to handle such a problem in AOSP build system.
AOSP often adds new clean steps. For example, see

cd build/make; git log CleanSpec.mk

@@ -0,0 +1 @@
include $(call all-named-subdir-makefiles,$(GAPPS_PRODUCT_PACKAGES))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this requires that the module names exactly match the directory names. I think this true now, although it wasn't always like that.

@jamuir
Copy link
Collaborator

jamuir commented Jul 10, 2019

It looks like you are building for x86 or x86_64.

You can work-around errors like this using GAPPS_EXCLUDED_PACKAGES:

GAPPS_EXCLUDED_PACKAGES += ActionsServices

If the apk really does not exist for x86, x86_64, then a better fix is to update the logic in opengapps-packages.mk.

@cwhuang
Copy link
Author

cwhuang commented Jul 11, 2019

OK, agree. Then could you fix that? Since you are more familiar with what apps don't exist for x86/ x86_64. I just invent a quick and lazy way to work around it...

@jamuir
Copy link
Collaborator

jamuir commented Jul 11, 2019

This should work:

$ git diff
diff --git a/modules/ActionsServices/Android.mk b/modules/ActionsServices/Android.mk
index 94ff562..8b5fcb5 100644
--- a/modules/ActionsServices/Android.mk
+++ b/modules/ActionsServices/Android.mk
@@ -1,3 +1,4 @@
+ifneq ($(filter arm64 arm, $(TARGET_ARCH)),)
 LOCAL_PATH := .
 include $(CLEAR_VARS)
 include $(GAPPS_CLEAR_VARS)
@@ -5,3 +6,4 @@ LOCAL_MODULE := ActionsServices
 LOCAL_PACKAGE_NAME := com.google.android.as
 
 include $(BUILD_GAPPS_PREBUILT_APK)
+endif

Could you try this and let me know? I actually stopped using aosp_build in my trees; I have some scripts that process opengapps zip packages and generate the necessary aosp makefiles (this avoids long sync times).

@greyltc
Copy link

greyltc commented Apr 8, 2020

@stefanhh0 This explains my build issue! (re discussion here: f8bd617#comments)

I have GAPPS_EXCLUDED_PACKAGES += ActionsServices but /ActionsServices/Android.mk still gets included and that breaks my build with

[642/718] including vendor/opengapps/build/modules/ActionsServices/Android.mk ...
zipinfo:  cannot find or open , .zip or .ZIP.
tools/test/connectivity/Android.mk: error: ActionsServices: No source files specified 
build/make/core/prebuilt_internal.mk:35: error: done.
16:07:25 ckati failed with: exit status 1

#### failed to build some targets (46 seconds) ####

because there is no ActionsServices blob for all, x86 or x86_64

Why are Android.mk files being included for packages in GAPPS_EXCLUDED_PACKAGES?

@MarijnS95
Copy link
Contributor

@greyltc This error commonly occurs when git lfs pull has not been done.

Android parses these build files in sorted order (which is why ActionsServices always shows up first), looks up the apk for com.google.android.as but encounters an lfs pointer instead of a zip file.
GAPPS_EXCLUDED_PACKAGES only determines whether that parsed package is subsequently included or excluded from the build.

Despite github being utterly broken when searching for ActionsServices, searching for lfs does the trick:
#254
#253
And a couple more.

@greyltc
Copy link

greyltc commented Apr 8, 2020

@MarijnS95

A lot of people are having trouble with git lfs and so this issue is getting conflated with those, I'm not having any issues with git lfs pull:

$ zipinfo vendor/opengapps/sources/x86_64/app/com.google.android.webview/21/nodpi/398716260.apk | head -n 10
Archive:  vendor/opengapps/sources/x86_64/app/com.google.android.webview/21/nodpi/398716260.apk
Zip file size: 72940880 bytes, number of entries: 506
-rw----     1.0 fat    22031 bx stor 09-Jan-01 00:00 assets/chrome_100_percent.pak
-rw----     1.0 fat  6412768 bx stor 09-Jan-01 00:00 assets/icudtl.dat
-rw----     1.0 fat   409736 bx stor 09-Jan-01 00:00 assets/resources.pak
-rw----     1.0 fat   237188 bx stor 09-Jan-01 00:00 assets/snapshot_blob_32.bin
-rw----     1.0 fat   240440 bx stor 09-Jan-01 00:00 assets/snapshot_blob_64.bin
-rw----     1.0 fat    16049 bx stor 09-Jan-01 00:00 assets/stored-locales/am.pak
-rw----     1.0 fat    14429 bx stor 09-Jan-01 00:00 assets/stored-locales/ar.pak
-rw----     1.0 fat    17945 bx stor 09-Jan-01 00:00 assets/stored-locales/bg.pak

Last time I checked (30 seconds ago), the ActionsServices blob is not available for x86, x86_64 or all so I can't git lfs pull it to solve this problem.

@MarijnS95
Copy link
Contributor

@greyltc Thanks for the heads-up, that's the topic of this PR which I completely ignored. The suggestion by @jamuir is the way to go, IMO.

@greyltc
Copy link

greyltc commented Apr 8, 2020

I've applied @jamuir's suggestion but I still have this issue...

I disagree that his is the right approach though. Why should we be hardcoding archetecture depencancies into the Android.mk files? What if com.google.android.as appears in https://gitlab.opengapps.org/opengapps/x86_64/-/tree/master/priv-app at some point in the future? There shouldn't be an ifneq here that makes it impossible to use!

I'm going to try @cwhuang's PR here now to see if my build can complete with that...

@greyltc
Copy link

greyltc commented Apr 8, 2020

Yep. I can confirm that my problem is solved today by:

curl https://github.com/cwhuang/aosp_build/commit/384cdac7930e7a2b67fd287cfae943fdaf7e5ca3.patch | git -C vendor/opengapps/build apply -v --index
curl https://github.com/cwhuang/aosp_build/commit/3bb6f0804fe5d516b6b0bc68d8a45a2e57f147d5.patch | git -C vendor/opengapps/build apply -v --index

I suggest you folks merge this PR! 👍

@MarijnS95
Copy link
Contributor

MarijnS95 commented Apr 8, 2020

Desync between opengapps and aosp_build is an ongoing issue already; I wouldn't be too afraid for it to appear on x86 as it doesn't break anything; someone needs to go in and undo the ifneq.

The same will happen with this PR, except that the ifneq is moved into the package list (or - from what it seems - into personal/device specific makefiles which'll obviously not solve this issue for other users). In addition, this will not work for transitive dependencies. Want Chrome or WebViewChrome on API 29? The build now fails due to the missing explicit dependency on TrichromeLibrary. That could be fixed by patching BUILD_GAPPS_PREBUILT_APK to load anything from LOCAL_REQUIRED_MODULES, keeping recursive/double includes in mind (which should not be an issue at this time).

An alternative solution has been discussed here which prevents the module from being added to the build-system altogether if the file is not found. That in turn relies on the build system to complain (or silently continue) when a missing module is specified in
LOCAL_REQUIRED_MODULES and PRODUCT_PACKAGES, respectively.

@greyltc
Copy link

greyltc commented Apr 8, 2020

Yeah, I can imagine the dependancy problem is not a simple one and there's a ton of intricacies I'm not familar with here.

Anyway, now you all are aware that x86_64 builds are broken and this PR solves that for me.

Unfortunately, it looks like jamuir's suggestion above does not solve that for me (for whatever reason, I'd guess that's somehow related to cwhuang's CleanSpec.mk patch here.). I'm happy to help test other solutions to the problem here though.

I'll also make I've also made a proper issue about this so it doesn't get lost in all the git lfs pull stuff: #255

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.

4 participants