Add WITH_GMS_ALT setting as an alternative to WITH_GMS

[Lanchon]

NOTE: this patch is long overdue. i did not post it before because, for reasons affecting LineageOS (not L4M), builds for my device were failing. this has now been fixed upstream so i could properly test everything before creating this PR.

hi @petefoth,

the issue of many L4M builds not having free space in the system partition for add-ons was widely discussed. the cause of the problem is known and fixes were proposed.

but the position of this project is that maintainers do not have the resources/motivation to test a change that affects all builds.

fine, i get that, and there is no possible counter-argument to that.

however...

as evidenced in previous, somewhat heated talks, many users are still in need of a solution.

here i am proposing one: do not change the way official builds are made, nor the default behavior of this docker, but still let users build their own images with free space for add-ons if they so choose. and let them do this without the hassle of having to fork and build their own docker.

if any such unsupported builds were to fail (they should not, but if) it is entirely their problem, and their time and effort to fix them. i think this is a reasonable solution that still respects the wishes of the project maintainers regarding their resources.

which devices are affected by this issue?

android 10+ devices that use dynamic partitions. because dynamic partitions seriously simplify the lives of OEMs and have no drawbacks, i think it is safe to say that virtually all newer devices will be affected.

how is this patch used?

you use WITH_GMS_ALT=true instead of WITH_GMS=true:

• WITH_GMS_ALT=true on devices using dynamic partitions causes some free space to be reserved in system. the exact reserved amount is device-specific, but in most if not all affected devices, it is 600MB. (this number will grow in future android versions when gapps growth requires it.)

• WITH_GMS_ALT=true on devices not using dynamic partitions behaves exactly like WITH_GMS=true.

example of use

on OnePlus 8 Pro, building with WITH_GMS=true produces:

OnePlus8Pro:/ # mount | grep '/ '                                                                                                                                                                                                            
/dev/block/dm-7 on / type ext4 (ro,seclabel,relatime,discard)
OnePlus8Pro:/ # df -h /                                                                                                                                                                                                                      
Filesystem      Size Used Avail Use% Mounted on
/dev/block/dm-7 824M 821M  2.5M 100% /

while building with WITH_GMS_ALT=true produces:

OnePlus8Pro:/ # mount | grep '/ ' 
/dev/block/dm-7 on / type ext4 (ro,seclabel,relatime,discard)
OnePlus8Pro:/ # df -h / 
Filesystem      Size Used Avail Use% Mounted on
/dev/block/dm-7 1.3G 821M  604M  58% /

showing the expected 600MB increment in free space.

one final note

this patch reports an error if both WITH_GMS_ALT=true and WITH_GMS=true are set because i have no better way to handle the condition:

>> [Sat Oct 14 06:27:49 UTC 2023] Copying '/srv/local_manifests/*.xml' to '.repo/local_manifests/'
>> [Sat Oct 14 06:27:49 UTC 2023] Syncing mirror repository
>> [Sat Oct 14 06:29:56 UTC 2023] Branch:  lineage-20.0
>> [Sat Oct 14 06:29:56 UTC 2023] Devices: instantnoodlep,
>> [Sat Oct 14 06:29:58 UTC 2023] (Re)initializing branch repository
>> [Sat Oct 14 06:29:59 UTC 2023] Copying '/srv/local_manifests/*.xml' to '.repo/local_manifests/'
>> [Sat Oct 14 06:29:59 UTC 2023] Syncing branch repository
>> [Sat Oct 14 06:30:44 UTC 2023] ERROR: WITH_GMS and WITH_GMS_ALT cannot be both true

to this effect, the script needs to check for WITH_GMS being true. naive checking causes an error if accessing an undefined variable, so the tidiest solution was to add these lines.

note that:

• this change cannot affect official builds, because all official builds are done with WITH_GMS=true.

• this change cannot affect unofficial builds in which WITH_GMS is undefined, because WITH_GMS is tested exclusively via ifeq ($(WITH_GMS),true) and ifneq ($(WITH_GMS),true) statements in LineageOS (which means that false behaves the same way as undefined).

however, if these lines are still considered an undue risk, the lines and the detection of concurrent use of WITH_GMS_ALT=true and WITH_GMS=true can simply be eliminated.

references

#358
#314
#486
#460

thank you for your help!

[petefoth]

@Lanchon This looks interesting. I'll look at it more closely, but we would need some additions to README.md

• text explaining how and why t use this flag
• an example of its use in the Examples section

[Lanchon]

I'll look at it more closely, but we would need some additions to README.md

sure, let me know after taking a look if you are still interested and i'll write the docs.

thank you!

[petefoth]

Having looked at your changes, and reviewed the previous discussions in the issues you reference, I'm sorry but I'm still not inclined to merge this change for a number of reasons

• you say many users are still in need of a solution. Looking at the issues you reference, it's actually a pretty small number of people, There was a lot of noise, but from a small number of contributors, most of whom seem to have moved on;
• in the PRs you reference, the problem would be easier and better (IMHO) fixed in the upstream device-specific makefiles
• looking at your changes, your line in 216 src/build.sh would have the effect of replacing the upstream file vendor/lineage/config/partner_gms.mk with a single line of code. I'm not sure what testing you have performed on the proposed change, but I'm pretty sure that will have some consequences.

So, thanks for the time you have spent on this, but I reached the conclusion after earlier discussions that we do not need to make a change. Nothing in this PR persuades me to change that conclusion, and I feel I have already spent too much time on this issue which - from what I can make out - affects very few developers so I'm going to close the PR.

I'm happy to leave this branch here, so that if any developers do feel the need for the fix, they can build a docker image using your changes. In that case, it may be interesting for them to know what testing you have performed.

Thanks again.

[Lanchon]

answering your concerns:

looking at your changes, your line in 216 src/build.sh would have the effect of replacing the upstream file vendor/lineage/config/partner_gms.mk with a single line of code. I'm not sure what testing you have performed on the proposed change, but I'm pretty sure that will have some consequences.

as you can see, the file that i am replacing starts with ifeq ($(WITH_GMS),true) and ends with endif and is in fact an empty file when WITH_GMS is not true, which is a precondition for the replacing to occur, and the reason why i replace it. note that the file replacement only occurs if WITH_GMS_ALT is true and this patch errors the build if both settings are true, so WITH_GMS cannot be true.

yes, replacing the file when WITH_GMS_ALT is used has consequences: the consequences are replacing the way microG is included in the build, which is exactly what i need and want when WITH_GMS_ALT is true.

the file in question has always been effectively empty when WITH_GMS is not true and will always be, because it has to be a no-operation in those cases. and the right way to handle that file, since i am replacing the way microG is included, is to replace the file. but if you don't like that for any reason, any of these can be used instead (which i don't favor):

• simpler method that no longer handles WITH_GMS, which is not a problem since WITH_GMS cannot be true:

sed -i 's/$(WITH_GMS)/$(WITH_GMS_ALT)/g' "vendor/$vendor/config/partner_gms.mk"

• better option that handles both WITH_GMS and WITH_GMS_ALT, which is an unnecessary complication since WITH_GMS cannot be true:

sed -i 's/ifeq ($(WITH_GMS),true)/ifneq ($(filter true,$(WITH_GMS) $(WITH_GMS_ALT)),)/g' "vendor/$vendor/config/partner_gms.mk"

although these changes would answer your concern, note that i do not favor any of these. hacking the handling of WITH_GMS is not the right way of doing things: replacing WITH_GMS with an alternative way is. i cannot know what the file in question will be in the future, and thus i cannot produce a hack guaranteed to work in the future, but i can replace the whole file safely.

in the PRs you reference, the problem would be easier and better (IMHO) fixed in the upstream device-specific makefiles

there is nothing to fix upstream and nothing will be fixed. WITH_GMS is not used in LineageOS: it is an unsupported feature and one they have repeatedly warned against using.

the bug that needs fixing is in this project: this project uses WITH_GMS, which is not supported by LineageOS.

i tried to fix this bug before (#460), but it was established that this project does not want to fix this bug.

so the next best thing: introduce an alternative way to use this project without WITH_GMS. because WITH_GMS produces unusable builds for may devices, and more worryingly, most newer devices.

you say many users are still in need of a solution. Looking at the issues you reference, it's actually a pretty small number of people, There was a lot of noise, but from a small number of contributors, most of whom seem to have moved on;

this is not a technical concern and as such cannot be answered intelligently. but there are many, many users: just search online, on reddit, on xda. users find out the L4M builds are broken when they try to root their new installs, and just move on to other ROMs that support sig-spoof, and that's the end of it.

regarding your "most of whom seem to have moved on": you are exactly right.

So, thanks for the time you have spent on this, but I reached the conclusion after earlier discussions that we do not need to make a change. Nothing in this PR persuades me to change that conclusion

no, you had reached the conclusion that you didn't have the time/energy to test changes on all devices. this PR does address those concerns: it does not change current builds in any way. it just gives users the tools to do working builds for themselves without having to fork this repo.

I feel I have already spent too much time on this issue

so do i. your older concerns no longer apply (no time/resources). your newer concerns were addressed here.

i would like to know if your decision not to merge is based on further technical reasons that can be argued, or on emotional reasons that cannot.

also, i cannot see a reason to oppose a feature that does fix existing real problems, on the basis that it could trigger unspecified unknown and so far imagined issues, given that the proposed feature is unsupported and optional.

the only reasonable opposition would be: i oppose it because i want to wait for a supported solution. but you clearly stated you don't want such a solution.

my take on this: just merge it and keep it undocumented! please

in short: i need these builds. do you want to review your merge decision o have me fork your project?

thanks.