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

addon: handle mounting of {product,vendor} on A only devices #879

Merged
merged 1 commit into from Sep 5, 2020

Conversation

merothh
Copy link
Contributor

@merothh merothh commented Sep 4, 2020

  • On devices with a separate product partition, addon scripts were
    not able to remove AOSP counterpart apps among possibly other
    similar issues as well

  • So fix it by mounting product before we get started

  • While we're at it, mount vendor too since its mounted in the
    main installer script

  • Thanks to @nikhilmenghani for his input

Fixes #872 for A only devices.

Copy link
Contributor

@nezorflame nezorflame left a comment

Choose a reason for hiding this comment

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

LGTM

@nezorflame
Copy link
Contributor

@osm0sis @mfonville please take a look as well

@osm0sis
Copy link
Member

osm0sis commented Sep 4, 2020

Only note I have would be to make sure the variables used are local only, otherwise looks good!

@mfonville
Copy link
Member

@merothh thanks for your work!
Did you test this on devices? :-)

* On devices with a separate product partition, addon scripts were
  not able to remove AOSP counterpart apps among possibly other
  similar issues as well

* So fix it by mounting product before we get started

* While we're at it, mount vendor too since its mounted in the
  main installer script

* Thanks to @nikhilmenghani for his input
@merothh
Copy link
Contributor Author

merothh commented Sep 4, 2020

@merothh thanks for your work!
Did you test this on devices? :-)

Indeed i had tested the initial commit i PR-ed on A only (Both Dynamic / Non Dynamic). As noted in the last comment i made on the issue

While i was adding the changes osm0sis mentioned above, i realized how arrays weren't exactly POSIX, so i've avoided using arrays as i did initially for $partitions (See here)

The latest commit has been again tested on A only (Both Dynamic / Non Dynamic), and after you guys have one last look, it seems good to go.

@merothh
Copy link
Contributor Author

merothh commented Sep 4, 2020

Also here's a diff between initial and the latest commit

@mfonville
Copy link
Member

@merothh thanks for improving and making it POSIX-compliant :-)

@osm0sis could you do an extra check to think of possible caveats/regressions before we merge?

@osm0sis
Copy link
Member

osm0sis commented Sep 4, 2020

I think it looks like it'll work fine. My only thought is there's probably no reason to mount twice, just mount rw the first time for each.

Also, minor caveat, I seem to recall there being some devices that report they're slot _a even though that's the only slot on the device, but it's hard to deal with broken device setups like that. 🤷‍♂️

@mfonville
Copy link
Member

Thanks for your comments @osm0sis
I think first mounting read-only and then trying to remount with rw is the same (good) behavior was we have in the installer script, because of some edge-cases (like emulators).

I will now merge this in, thanks @merothh !

If we can think of a way to detect the broken slot_a setups that would be very welcome in a new PR :-)

@mfonville mfonville merged commit 6ed0ee6 into opengapps:master Sep 5, 2020
@osm0sis
Copy link
Member

osm0sis commented Sep 5, 2020

Ah yes, I always forget about the emulators 😛

merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
* As osm0sis suggested [here](opengapps#879 (comment))
  some A only devices, seem to report slot as _a.

* So check with ro.build.ab_update instead
merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
* As osm0sis suggested (opengapps#879 (comment))
  some weird A only devices, seem to report slot as _a.

* So check with ro.build.ab_update instead
merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
* As osm0sis suggested (opengapps#879 (comment))
  some weird A only devices, seem to report slot as _a.

* So check with ro.build.ab_update instead
merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
* As osm0sis suggested (opengapps#879 (comment))
  some weird A only devices, seem to report slot as _a.

* So check with ro.build.ab_update instead

* $system_as_root is unused elsewhere, so drop that as well

Reference:
 - https://android.googlesource.com/platform/build/+/refs/tags/android-7.1.1_r61/tools/buildinfo.sh#26
 - https://android.googlesource.com/platform/build/+/refs/tags/android-10.0.0_r41/tools/buildinfo.sh#28
merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
* As osm0sis suggested (opengapps#879 (comment))
  some weird A only devices, seem to report slot as _a.

* So check with ro.build.ab_update instead.

* $system_as_root is unused elsewhere, so drop that too.

Reference:
 - https://android.googlesource.com/platform/build/+/refs/tags/android-7.1.1_r61/tools/buildinfo.sh#26
 - https://android.googlesource.com/platform/build/+/refs/tags/android-10.0.0_r41/tools/buildinfo.sh#28
merothh added a commit to merothh/opengapps that referenced this pull request Sep 5, 2020
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.

Proper addon.d support for the /vendor and /product partitions for A/B devices
4 participants