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

installer: mount /product for non dynamic devices as well #868

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

merothh
Copy link
Contributor

@merothh merothh commented Aug 16, 2020

  • As we dont mount /product for devices that are not dynamic
    and have a separate partition, on such devices AOSP counterpart
    apps in /product end up not being removed, among possibly other
    similar issues as well.

  • So mount it.

    Thanks to @nikhilmenghani for the tip

* As we dont mount /product for devices that are not dynamic
  and have a separate partition, on such devices AOSP counterpart
  apps in /product end up not being removed, among possibly other
  similar issues as well.

* So mount it.

  Thanks to @nikhilmenghani for the tip
@merothh
Copy link
Contributor Author

merothh commented Aug 16, 2020

(Just so that we're clear)

Verified on:

  • Redmi Note 7 Pro (A only - Non Dynamic, but uses a separate product partition in custom ROMs, works fine with this patch)
  • Poco X2 (A only - Dynamic, uses a separate product partition, works fine as previously)

@merothh
Copy link
Contributor Author

merothh commented Aug 16, 2020

Hold, while I do a follow up change for addon.d

@nezorflame
Copy link
Contributor

Hi, thanks for your PR!
@osm0sis can you take a look as well?

@osm0sis
Copy link
Member

osm0sis commented Aug 16, 2020

Makes sense, I guess!

@mfonville
Copy link
Member

So I wait for a merge until addon.d is also updated?

@osm0sis
Copy link
Member

osm0sis commented Aug 16, 2020

addon.d will be tricky, I'm not even sure if/how we handle /vendor there currently

@merothh
Copy link
Contributor Author

merothh commented Aug 17, 2020

So I wait for a merge until addon.d is also updated?

Yes, that would be ideal.

addon.d will be tricky, I'm not even sure if/how we handle /vendor there currently

Indeed. I didnt see anything to handle /vendor.

merothh@afa50fa

This was what i came up with then, it was working on non dynamic. But the dynamic one was screwing up and aborting the install when i dirty flashed. Not sure why, right now.

If we do manage to get it working somehow, might as well extend to it mount vendor as well (if needed).

@osm0sis
Copy link
Member

osm0sis commented Aug 17, 2020

So probably better to just merge this and have addon.d on a to-do list for later.. I'm thinking if we can get things mounted right then removals should be able to happen via the normal addon.d functions since it's all symlinked into /system.

But on dynamic, most importantly on A/B devices with addon.d-v2, those extra mounts will need to be into /postinstall, and are not standard to do, so it'll be pretty tricky.

@merothh
Copy link
Contributor Author

merothh commented Aug 17, 2020

So probably better to just merge this and have addon.d on a to-do list for later.. I'm thinking if we can get things mounted right then removals should be able to happen via the normal addon.d functions since it's all symlinked into /system.

But on dynamic, most importantly on A/B devices with addon.d-v2, those extra mounts will need to be into /postinstall, and are not standard to do, so it'll be pretty tricky.

Alright then. Guess it makes sense to merge this.

@mfonville
Copy link
Member

Alright, thanks!

@nezorflame
Copy link
Contributor

Let's track the addon.d issue here

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.

None yet

4 participants