-
Notifications
You must be signed in to change notification settings - Fork 197
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
MicroG should *not* set WITH_GMS = true #358
Comments
LineageOS does typically reserve quite a lot of space for the regular GApps addons, so you may want to set |
This is the reason it was originally added, or at least why I reccomended it be added (for blueline specifically). |
OK that makes a bit of sense but it has the side effect of breaking other addons. I'll leave it to the devs here to discuss the best way forward. |
Besides the partition sizes, we're also using |
Are you able to monkey-patch in extra definitions to the LOS+MicroG build? Setting the Alternatively, LineageOS devs seem to suggest to me that |
I second that this should be changed. Currently, the free space is extremely limited, and for example, Lygisk is not able to use |
If you can find a better way to adjust the partition size so that our system apps fit on all supported devices, I would be very happy to accept a pull request. |
I am a user, but what is wrong to set the reserved size to be, say, 50MB? |
The fix would be to set |
If I interpret the relevant makefiles correctly, it's actually Our current way of including the extra packages relies on partner_gms.mk, which will no longer activate if We could return to the old way of including them via Maybe this makes enough sense to you to try it out. If I find the time, I might also give it a shot. |
Disregard that, the files I was looking at are only relevant for a few devices. Looking through the actual places where e.g. BOARD_SYSTEMIMAGE_PARTITION_RESERVED_SIZE is defined, the value is different from device to device and only conditional on |
Geez... took me ages to figure out that this is the issue why I cannot install AuroraServices. There're only 2.5MB on system and 0.9MB on system_ext available, whilst plain lineage provides the 1+GB on system. Anyhow, since the builds already patch the version in lineage's vendor (config/common.mk or config/version.mk) [1], wouldn't it be possible to just patch away the On the other hand, the relevant thing in config/partner_gms.mk is probably
Wouldn't it be possible to add that one line from the Best regards [1] https://github.com/lineageos4microg/docker-lineage-cicd/blob/master/src/build.sh#L206 |
Introduce 'WITH_MICROG' to replace use of 'WITH_GMS'. Fixes lineageos4microg#358.
A search in the LIneageOS repos indicates that this flag is not used only to set partition sizes. If we decide to change how we use it, then the change would need to be fully tested. To fully test, we would need to check not only that builds for all ~220 devices do not break, but also that the functionality of those devices is not broken in other ways. Clearly that is beyond the capabilities of this project and is not going to happen. Given that this issue does not affect the core functionality of our ROMs, and only impacts the uses of some system addons for some devices, then I am not inclined to make any changes. I'm happy to leave this issue open for further discussion, but I won't be rushing to push any PRs |
i totally disagree with this claim. the builds without also, as @bananer says here, builds published by los4mg used to be created without also, microg's developers work and test using builds without but on the contrary, using the option indeed causes issues; which is why we are here.
yes, and it's used for stuff that we don't want:
as far as i can see, all the other uses revolve around not preparing extra spaces for addons. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Introduce 'WITH_MICROG' to replace use of 'WITH_GMS'. Fixes lineageos4microg#358.
@infinity0 you write that
Please can you specify which addons you know or believe to be are broken, and on which devices? This will give us a better idea of the scope of the problem, and help us decide on how to move forward with this issue . Thanks |
An example:.Lygisk on gta4xlwifi, cf. programminghoch10/Lygisk#17 (comment) |
Thanks. Do you remember offhand which los4mg buid the problem occurred in, and whether or not it still occurs for this add-on, on this device? No porblem if not - just trying to gather data :) |
I have posted in the XDA Forum thread asking for other users' experiences - good and bad - with installing add-ons. Let's see what comes out of that. I will try to maintain the following tables, based on responses here and in the forum thread, and on my own testing: Failed installs
Successful installs
|
I tested installing Lygisk on the devices I have access to. I installed by flashing from recovery, then opening the installed app and letting it do its stuff. No problems with insufficient space, on
Note that the unofficial builds were all built using the I also tried installing Magisk on the same devices, using the deprecated 'Rename and flash from recovery` method. No reported insufficient space problems, but the install didn't succeed - no sign of the app after rebooting. I'm not going to start messing with patching images, so I'm not going to count these attempts as valid tests |
The Lygisk installer does not handle the exceptions appropriately, thus it will show "success" even if there is no space. Furthermore, these spaces are not essential to (first) install Magisk, but essential to make root persist after updates. The point of Lygisk (being a fork of Magisk) is to make root persist after updates, and if there is not enough space, after updating (or even after reflashing the current ROM), the system might not boot. More technical details: in "modern" environments, the recovery can no longer access encrypted |
hi @petefoth, thanks for getting onto this and doing this investigation. i'll try to help sharing what i know. but first... magiskmagisk is a "system-less" mod, meaning it doesn't touch the the reason for its existence is maintainable rooting of stock firmwares. it used to be that, once you root, you couldn't accept OTAs anymore. now you can easily unroot (just restore the tiny boot or init_boot partition), apply the OTA, and reroot. magisk may also install the manager app, but this is complimentary. the boot runtime is the important piece, you can install the app from any source later. this is why i think your tests actually succeeded: you didn't get the app, but if you had manually installed it, it would have found the runtime and worked. btw, i think the app is always installed in /data to remain system-less. in your case, maybe recovery couldn't decrypt and mount back on-topic...in the beginning system was a regular ext4 partition and the OTA just wrote files to it. then came dm-verity and OTAs had to be block-level: the android build system now produced a filesystem image instead of just files. for this the build system had to know the filesystem type and the size of the system partition for each device. by upgrading old phones, cyanogenmod stumbled on a roadblock at the time: old devices had small system partitions, but android kept on getting bigger. users countered by installing smaller and smaller gapps, but eventually this became futile and android itself was getting chopped up to fit by device maintainers. so people started repartitioning their devices (see https://lanchon.github.io/REPIT/ - ahh, the good old days of unsigned partition tables!). this brought on a problem: aftermarket roms like cyanogenmod had system images of stock sizes, but people had physical system partitions that did not match. as a stopgap measure, TWRP added a manual option to grow the filesystem in system partition to match its partition size. but eventually cyanogemod and friends caught on and added the grow as a post-install step in their zips. (this meant that the build system didn't need to know the physical partition size anymore, but it was useful to have it anyway to know when repartitioning ceased to be optional and became mandatory.) now this is important: this was the status quo for all devices before android 10, and still is for devices that opt to launch with physical system or system_a/b partitions. for all these devices, the size of the filesystem image within the lineage zip is irrelevant, as the bundled install script will grow the filesystem to the size of the physical partition. i didn't look but i am willing to bet that, for all of these devices, the WITH_GMS setting does nothing at all to their OS partition sizes. this is why if you search for WITH_GMS in the lineage repos you'll find just a handful of matches and not hundreds, as you would expect if there were a match for each supported device. so older devices won't have the no-space-in-system issue, but as time goes by newer devices will. this is why we can't just sit around and ignore this. it also doesn't surprise me that the older devices on which you tested didn't display the issue. but moving on... next came android 10 and with it the (optional) dynamic partitioning scheme. there's a lot of variation in implementation here: one can choose any partition accessed after the kernel and its ramdisk loads to be 'dynamic', but typically system (or system_a/b), vendor (or vendor_a/b), etc are chosen to be dynamic (and userdata is not). (btw, dynamic partitioning is orthogonal to seamless a/b upgrading, so you can chose either, or both, or none. but there's yet another scheme since android 11: an optional 'virtual a/b' scheme that cleverly merges these features.) with dynamic partitioning there is a big physical super partition which acts as a container, and dynamic partitions are created within it via dm-linear. the objective is not having to reserve large spaces for system, system_ext, vendor, odm, product, etc... or their a/b variants, and create those partitions within super during each OTA to fit their exact image sizes. now you see the problem: the aforementioned grow-after-install step became a no-operation: for devices that have dynamic system partitions (probably all devices that use dynamic partitions) their post install system partitions will have no free space at all! so in order to support installation of gapps as an addon on those devices, the lineage people had to implement a solution atop AOSP. for dyn-system devices they just added a bunch of extra margin space to the dynamic filesystem image creation steps, but only if gapps are not being included in the build, and they overloaded the WITH_GMS setting for this. this was a quick dirty hack, and since it's implemented for each device and not centralized, it was a very bad call. my guess: it was probably done by the first maintainer of a dyn-system device on their own device tree, then other maintainers just copied the stuff to their trees, and nobody provided a system-wide solution... and here we are. why is it a bad solution? if you build without gapps, which is what they do, it sort of works: each maintainer chooses a max gapps size (probably copying from other maintainers), and if gapps ever grows too large, well they are all screwed. but the issue is building WITH_GMS=true: this flag doesn't work as it did before! before, it just added gapps and the post-install step would enlarge the partition and you'd have space for further addons. now WITH_GMS=true produces tight fitting partitions that don't support addons (breaking an important implicit functionality of LOS) and if the lineage people ever use it they'll run into a wall... just as we did. but a good solution is not trivial. it might look like this:
with all that behind...what can we do? it's clear to me at least that we can't ignore this: microg users are modders, and we hate not being able to mod! for non-dyn-system devices (the majority) this is a non-issue: WITH_GMS should not be found in their device repos, given that no space hack is needed and their filesystems will be grown to max during install, and so we can choose whatever we want for that setting and it won't affect the build. and if it does affect it (i think in only 2 google devices this is used to include a widevine DRM blob), we certainly want if OFF. for dyn-system devices (the minority, but growing):
so my take on this is that building using WITH_GMS=false is the way to go. only users of dyn-system devices will see the benefit of the change, but as time goes by more and more devices will fall in this class... so we better fix it. |
i want to add that today i was in the process of learning docker and building lineage images with a modded dockerfile. but suddenly everything stopped working and i can't even do a clean lineage build from the clean docker-lineage-cicd anymore, i don't know why. but builds are so slow and it's taking me a lot of time to debug. but the message is i'm working on it, and will hopefully make some progress soon. i'm testing on instantnoodlep (oneplus 8 pro), in which system_a/b are dynamic partitions contained in super. flashing a standard los4mg image produces this:
there's not much you can do with 2.5 MB these days... (this device has 256 GB storage.) if you have a bunch of files and want to create a tight fitting filesystem for them, your job is not easy: filesystems are not designed to be used that way. android build possibly just adds up the sizes of files, adds a margin to that, and hopes it is good enough; which might work because the android OS does not include a lot of small files (it look like the margin is 2.5 MB). or it could round up each file size to block size (which?). or it could add a per-file space cost. or... who knows! |
my proposal to go forward...given that lineageos already adds space to devices in which system is a dynamic partition, my proposal to go forward is to try reusing that code:
i'll try to start with step 1. |
currently stuck here: #468 |
Here's my final opinion on this issue.
So, in my opinion, this issue is not highlighting any defect in our project, and there is therefore nothing that needs to be fixed. Much of the discussion of this issue concerns whether or not we should set `` WITH_GMS=true`. The fact is that we do set it, it works and, so far as I can tell it will continue to work . So I intend to do the following:
I am sure many of the contributors to this thread will disagree with my opinion, my arguments and my actions. That is unfortunate, but I am acting in what I believe to be in the best interests of the project. Thank you for all your contributions. |
From this one data point we can conclude by looking at the source code that any addon that needs to write extra stuff to the system partitions will break. Currently the only workaround is to convert the addon into a Magisk module instead, which does the same thing but dynamically at runtime. The amount of comments on this issue indicate it's not just "one addon", I'm not sure where you are getting this "one addon" from. Just because it's not some popular addon doesn't mean it's not a bug, it's very very very difficult to debug and wasted several days of my time, multiply that by the number of people complaining here and you have some idea of how time-wasting it is. It discourages people from doing their own hobbyist Android development. I'm not sure if it's obvious from the bits I quoted in the OP, but from the wider conversation with them (i.e. the actual LineageOS devs) they specifically told me you guys should not be setting If you don't believe me, go and talk to them on IRC, they are very reachable. (But don't say the word "microg" because they have/had a bot in there that auto-kicks anyone that says this, apparently they have a dim view of this project. Maybe try the -dev channel instead if you need to say the word.) I'm honestly surprised at the amount of mental gymnastics some open source maintainers go through to convince themselves stuff is "not a bug". Lots of users are complaining, it's a bug by definition. I also don't understand why there has been so much discussion here. It should be simple as setting a bunch of makefile flags to reserve some extra space, surely? |
I'm closing this one now, along with the PR #460 |
This is not true.
Setting
All say: "this is not my bug, report in another place, closing". |
As workaround/quick fix it is possible to patch on fly all |
OK now we have one more bug report than we had before. That's not enough for us to change the way we make our builds. My reasoning is that, in affected devices
You may disagree with that reasoning, but I don't see any prospect of it changing!
We build for ~230 devices, we are not going to patch all those A number of possible fixes have been suggested (but not implemented or tested) in the issues and PRs mentioned above:if you want to try them out and make your own build that is up to you, but the project will not be making any such changes. And I have |
I do not ask to change way how you make builds, Free space on system partition required to many things, not only GApps.
Better remove bug tracker at all: no bug tracker = no bugs :) |
@petefoth sorry but your answer is bordering on ridiculous at this point. i don't care about you ~230 devices. i pleaded that at least you allowed a hidden, unsupported, undocumented flag to allow self-builders to build images by themselves without unnecessarily forking the project (#493) and you still didn't allow it; even thought it wouldn't affect a single of your 230 devices; even though this bug affects ALL android 10+ devices that use dynamic partitions (yes, ANY current device), as explained in the linked PR. so my question at this point is: who besides you have commit rights? thank you for your help. |
All 'Members' of the The 'Owners' of the organisation (of which I am not one), have rights to add new people to the organisation. If you disagree with the project's decision on this issue and the related PRs, or with the way this issue and the associated PRs have been handled, then I guess you have the following options:
There is no new information in recent posts, only people - including me - restating their opinions: to me this counts as 'noise'. I have therefore locked this issue and #493. |
WITH_GMS = true
is an internal LineageOS build-time variable which means more than simply "Google services are part of this build". According to LineageOS devs in #lineageos-dev:Internally, LineageOS treats
WITH_GMS
as implying that the user will never install any further addons, and therefore will not reserve any extra space in the relevant system partitions when it is set.Because MicroG is built with it set, system partitions have no extra space in them, meaning that addons that need to modify /system fail to do so, resulting in bugs like topjohnwu/Magisk#3820 (comment) and arovlad/bromite-webview-overlay#5.
There is no benefit to setting
WITH_GMS
from MicroG's perspective; LineageOS official builds do not set it, and GApps addons such as MindTheGapps/OpenGApps do not "switch it on" after being installed, as it is a build-time variable only and is invisible at runtime.Therefore, please stop building your LineageOS+MicroG images with
WITH_GMS
, and stop recommending this practice in this repo's documentation, as it prevents addon installers from working with no actual benefit in return.The text was updated successfully, but these errors were encountered: