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

LineagesOS updater #90

Merged
merged 11 commits into from Oct 7, 2021
Merged

Conversation

ajs124
Copy link
Contributor

@ajs124 ajs124 commented Feb 22, 2021

What I have done so far:

  • built and flashed a ROM with this change to my payton/Moto X4

  • verified that it boots and checks the specified URL for updates: host=rbtnx.ipv2.de:443 remote_addr=[…] remote_user=- request="GET / HTTP/1.1" ssl_proto=TLSv1.3 ssl_cipher=TLS_AES_128_GCM_SHA256 status=200 body_bytes_sent=970 referer="-" user_agent="Dalvik/2.1.0 (Linux; U; Android 10; Moto X4 Build/QQ3A.200805.001)" x-forwarded-for="[…]" rt=0.000 ua="-" us="-" ut="-" ul="-" cs=-

  • implemented metadata generation

  • actually install an ota update using this

@ajs124
Copy link
Contributor Author

ajs124 commented Feb 22, 2021

I noticed an issue when trying to generate the metadata: the LineageOS release (e.g. 17.1) does not seem to be exposed through any option, but I think the metadata won't work if version is not set to the correct release.

@danielfullmer
Copy link
Collaborator

@ajs124 Hmm, perhaps we could add and set a lineageos.version option in the lineageos flavor. However, I'd worry about exposing that to the user because they might think they could set lineageos.version in their config instead of androidVersion. So perhaps internal=true;?

@ajs124
Copy link
Contributor Author

ajs124 commented Feb 22, 2021

Sounds reasonable. AFAICT there isn't any mapping between androidVersion and LineageOS releases so far.

So what does currently happen if the following two things are set in a configuration?

  flavor = "lineageos";
  androidVersion = 11;

@danielfullmer
Copy link
Collaborator

I think currently that configuration would use the LineageOS 17.1 sources in conjunction with the android 11 patches in robotnix, so it would probably break. :) When we add the new LineageOS version we can conditionally include the right sources based on which version was selected. (as we currently do in the vanilla flavor).

I take your point about the mapping between androidVersion and LineageOS potentially being not one-to-one. An alternative would be to have the user select lineageos.version and have us set the androidVersion?

@danielfullmer
Copy link
Collaborator

Another note as you look at generating metadata: otaMetadata is currently generated under modules/release.nix. Since it depends on which updater is being used, we could move it to modules/apps/updater.nix to keep things in one place.

@ajs124
Copy link
Contributor Author

ajs124 commented Feb 22, 2021

I think currently that configuration would use the LineageOS 17.1 sources in conjunction with the android 11 patches in robotnix, so it would probably break. :)

That does indeed seem to be what happens.

I take your point about the mapping between androidVersion and LineageOS potentially being not one-to-one. An alternative would be to have the user select lineageos.version and have us set the androidVersion?

I'm not sure what the correct/best thing to do here is, tbh. My point was just an observation/me wondering if I'm reading the source correctly.

@samueldr
Copy link
Contributor

AFAIK LineageOS (from CyanogenMod's history) always mapped 1:1 to letter-based versions of Android.

The major version of CyanogenMod and then LineageOS increased when the letter changed.

Am I missing something about the desired 1:1 relationship?

@ajs124
Copy link
Contributor Author

ajs124 commented Feb 22, 2021

🤔 I think, e.g. with android 10, they first released (or at least branched and worked on) 17.0 and then decided to go with 17.1 instead. Not sure why that happened and if and why anyone would want 17.0 instead of 17.1, but I seem to remember that happening.

@samueldr
Copy link
Contributor

samueldr commented Feb 22, 2021

Yes, I mean for major versions, you know that e.g. 17.* is used for 10/Q.

It seems that the minor versions will be increased whenever AOSP "feature drops" are breaking their workflow enough

AFAIUI starting with LineageOS 17 / Q / AOSP10, the minor version is a somewhat internal "we had to rebase everything" number.

I think it should be safe to always map AOSP release number/letters to the major LineageOS version.

  • 14 → N / 7
  • 15 → O / 8
  • 16 → P / 9
  • 17 → Q / 10
  • 18 → R / 11

It was more of a mess initially, where it was like:

  • CM9 → I / 4.0
  • CM10 → J / 4.[1..3]
  • CM11 → K / 4.4
  • CM12 → L / 5.0
  • CM12.1 → L / 5.1
  • CM13 → M / 6.0
  • CM14 → N / 7.0
  • CM14.1 → N / 7.1

But even as CyanogenMod, the rule of thumb of each letter, one major version increase was true.

@danielfullmer
Copy link
Collaborator

In that case, the selection would be better done by choosing androidVersion and setting lineageos.version (modulo naming) based on that. We should always just use the latest version of LineageOS corresponding to a particular androidVersion. No need to keep 17.0 in robotnix if 17.1 is out.

@samueldr
Copy link
Contributor

@danielfullmer yes, definitely, here 17.0 has been closed and abandoned for 17.1.

I also think that setting androidVersion over lineageos.version is appropriate. Where robotnix would have a map of android versions to branches.

@danielfullmer
Copy link
Collaborator

As an FYI, there's also the LineageOS updater backend available here: https://github.com/lineageos-infra/updater
Actually using it is probably overkill for our purposes if we just need to publish some static metadata on a webserver, but it could still be worthwhile to take a look at.

@ajs124
Copy link
Contributor Author

ajs124 commented Jun 9, 2021

Sorry I'm only getting back to this now, but I should have some time to finally get this into a useful state soon.

How would I best introduce an option/property for lineageos.version? Alternatively, what do you think about just having a flavorVersion or similar option? Custom ROMs using different versioning schemes seems to be something not only LineageOS, but e.g. also ResurrectionRemix does.

@danielfullmer
Copy link
Collaborator

So, the thing I don't particularly like is having two options (e.g. androidVersion and flavorVersion), and trying to keep them in sync. If the user manually sets androidVersion, should flavorVersion be set based on that? What if the user sets flavorVersion, should androidVersion be set based on that? What if the user sets both androidVersion and flavorVersion, should we throw an error if they conflict? There's a bunch of alternatives here and I don't have a good sense of what the best choice would be. In the future, if we add more flavors, I'd think we'll have a better sense of the tradeoffs.

Since we only have LineageOS right now, I don't have a problem with you just doing a lookup specifically for LineageOS in your module. We also now have these lines in the LineageOS flavor:
https://github.com/danielfullmer/robotnix/blob/master/flavors/lineageos/default.nix#L13-L17
If you'd like, you could extract that into its own .nix file, and import the functions into your updater module. I'd just like to have one place where we maintain this mapping.

@ajs124
Copy link
Contributor Author

ajs124 commented Jun 10, 2021

Hm. We could make the option readonly and/or internal?
But I understand why having two options might not be a good idea.

I've seen the code that does the mapping in flavors/lineageos/default.nix, but wasn't sure what do to with it. As in, if I should just move it to a file like you suggested or how else this would best be organized.

Since you said "extract that into its own .nix file, and import the functions into your updater module", do you mean we should have a separate updater module for lineage or are you fine with having only one module and it behaving conditionally, as it does right now in this PR?

@danielfullmer
Copy link
Collaborator

Hm. We could make the option readonly and/or internal?
But I understand why having two options might not be a good idea.

Making flavorVersion an internal option would alleviate a lot of my concerns. (We could refactor things later if desired)

I've seen the code that does the mapping in flavors/lineageos/default.nix, but wasn't sure what do to with it. As in, if I should just move it to a file like you suggested or how else this would best be organized.

You could use that in the lineageos flavor to set flavorVersion.

Since you said "extract that into its own .nix file, and import the functions into your updater module", do you mean we should have a separate updater module for lineage or are you fine with having only one module and it behaving conditionally, as it does right now in this PR?

Sorry, I spoke without re-reading the PR recently, having one updater module and having it behave conditionally is fine.

@ajs124 ajs124 force-pushed the los_updater branch 2 times, most recently from f28cee4 to 570ef44 Compare June 13, 2021 14:47
@ajs124 ajs124 marked this pull request as ready for review June 13, 2021 14:47
@ajs124
Copy link
Contributor Author

ajs124 commented Jun 13, 2021

Making flavorVersion an internal option would alleviate a lot of my concerns. (We could refactor things later if desired)

done

You could use that in the lineageos flavor to set flavorVersion.

done

Sorry, I spoke without re-reading the PR recently, having one updater module and having it behave conditionally is fine.

No problem, if I hadn't taken months to finish this, it probably would have been more on your mind.

The remaining things with this I'm not 100% happy about are:

  • The updater URL always ends in a /. This is my fault (41cb9f0). The LineageOS Updater app queries this URI and expects a JSON there. If it didn't end in a /, the file could potentially be served to more easily.
  • The size is calculated in the release script. Actually, metadata only gets generated in the release script.

@danielfullmer
Copy link
Collaborator

danielfullmer commented Jun 14, 2021

Thanks! I hope to test it on my own device soon, in the meantime:

  1. I don't like fixing the choice of which updater metadata to use based on just the flavor. If someone made a new flavor they would have to modify this module to support it. (I imagine this is likely--with all the lineageos variants). Perhaps we could provide another option, apps.updater.package with possible values grapheneos or lineageos, and have the flavors set that option by default for which updater package should be used with that flavor. (apps.updater.package isn't exactly a great name though--maybe apps.updater.flavor?)
  2. We should support both ways of creating releases: using the releaseScript, as well as directly building the otaDir output (if on a signed build, then also using a Nix sandbox exception for the keys). Currently, if we built otaDir, the ROM_SIZE wouldn't get replaced with the true size. If the user is building otaDir, we should still be able to get the filesize of the ota output inside a derivation creating otaMetadata. (Also, I was curious how exactly the LineageOS updater uses the size attribute internally. Does it need to be accurate?)
  3. I noticed that releaseScript puts the metadata under index.json, but the otaMetadata names the derivation ${config.device}.json. Does the LineageOS updater look for index.json? If the user has multiple devices, do they need to have a unique apps.updater.url for each one? (since otherwise the index.json files would conflict). We have some flexibility here about how exactly we set updater_server_url based on apps.updater.url.
  4. We'd also need to update the documentation under docs/src/modules/ota.md to ensure it works with this.

@ajs124
Copy link
Contributor Author

ajs124 commented Jun 22, 2021

  1. I introduced apps.updater.flavor and it's using that now.
  2. Agreed. The size is displayed in the UI, not sure what happens if it's off. Is there anything stopping us from not building otaDir as a linkFarm, but a regular derivation where we symlink the regular build outputs and do the same sed thing to the metadata that is done in the releaseScript?
  3. The LineageOS updater queries the URL that's in the property. The problem is that currently the updater module in Robotnix enforces that this URL ends in a /, because GrapheneOS fails to query it properly otherwise (AFAIR). This apply function could probably be made conditional to the updater flavor, but that seems kind of ugly. Then again, the apply is kind of ugly in the first place. Maybe it should have been, or should be, an assert. Hence index.json, because then one can configure ones webserver to serve it on a request to a folder. That can obviously also be done if it's not called index.json, but it seemed fitting.
    As for the need for multiple URLs, I think so, yes. The JSON doesn't contain the device name or anything like that, after all.
  4. Will do, once everything else is ready.

@ajs124 ajs124 force-pushed the los_updater branch 3 times, most recently from 8d4aaa1 to 6d06e59 Compare June 22, 2021 20:52
Copy link
Collaborator

@danielfullmer danielfullmer left a comment

Choose a reason for hiding this comment

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

  1. That makes sense to me.

  2. We don't have to set updater_server_url to be exactly the same as apps.updater.url. We can still have apps.updater.url end in a / by default, but then set updater_server_url based on apps.updater.url:
    I was thinking we could do something like
    resources."packages/apps/Updater".updater_server_url = "${cfg.url}${config.device}";
    or
    resources."packages/apps/Updater".updater_server_url = "${cfg.url}lineageos-${config.device}";

flavors/lineageos/default.nix Outdated Show resolved Hide resolved
modules/apps/updater.nix Outdated Show resolved Hide resolved
writeOtaMetadata is moved up, because it's used by releaseScript and
otaDir

This isn't a no-op for the other flavors, because it changes otaDir into
a runCommand and copies (or rather "cat"s) the metadata, whereas it was
a symlink until now
@danielfullmer
Copy link
Collaborator

danielfullmer commented Oct 5, 2021

Apologies for not following up with this earlier. I just fixed the merge conflicts and added a commit to have the updater_server_url point directly to the lineage-${config.device}.json file. I tried an update on sunfish LineageOS using the metadata dir generated via the otaDir output, and it worked for me!

Are there any additional changes you'd like to make before merging, or is this ready to go?

@ajs124
Copy link
Contributor Author

ajs124 commented Oct 5, 2021

No problem. I haven't been actively using this and have been meaning to do start doing that, but if you've tested it that's good enough for me.

Coincidentally, I also recently prepared a new branch that is mergeable into master, but it probably doesn't differ much from what you did here.

@danielfullmer danielfullmer merged commit 8b1195b into nix-community:master Oct 7, 2021
@ajs124 ajs124 deleted the los_updater branch October 7, 2021 20:21
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