toltec-dev / toltec

Community-maintained repository of free software for the reMarkable tablet.
https://toltec-dev.org
MIT License
722 stars 60 forks source link

Define a process for moving packages from testing to stable #20

Closed matteodelabre closed 3 years ago

matteodelabre commented 4 years ago
Eeems commented 4 years ago

So do we also want an unstable repo where devs can manage their packages for testing deployments?

matteodelabre commented 4 years ago

Can you clarify what would be the difference of aim between the testing branch and the unstable branch? To make things manageable, I’d say that it would be best to find a solution that minimizes the number of branches while still achieving the goal of actual stability for the stable branch.

Eeems commented 4 years ago

From the Debian wiki:

Packages from Debian Unstable enter the next-stable testing distribution automatically, when a list of requirements is fulfilled:

  • The package has been in "unstable" at least for 2-10 days (depending on the urgency of the upload).
  • The package has been built for all the architectures which the present version in testing was built for.
  • Installing the package into testing will not make the distribution more uninstallable.
  • The package does not introduce new release critical bugs.

So basically unstable is being actively worked on by developers/maintainers and contains bleeding edge. Testing contains packages at versions that are deemed "stable" enough to move forward. After that they move from testing to stable.

If we don't want the overhead we could be like ArchLinux and just have testing and stable though.

LinusCDE commented 3 years ago

I find this idea good, but I doubt, that we'll stay active all the time to maintain even more branches.

Although we also need to consider having some kind of separation between rM1 and rM2 software when they diverge. The simplest idea in my mind would be to have a check in a preinst file and abort the installation (if possible) when a user wants to install a software that makes not sense for his device (e.g. software that disables buttons on the rM2).

One check to find out whether it's a reMarkable 1 or 2 could be with:

if grep deviceid=RM100- /home/root/.config/remarkable/xochitl.conf >/dev/null; then
  echo "This is a first gen"
else
  echo "This is a second gen"
fi

My initial idea was to have a stable-rm1 and stable-rm2 branch, but it think the above idea is more maintainable.

What are your opinions on this?

Eeems commented 3 years ago

Until we get a toolchain for the rM2 and can actually start building applications for it, I think we should hold off on any decisions relating to it. I think we likely should setup some sort of automation around promoting things to stable to help with when we aren't as active.

matteodelabre commented 3 years ago

I agree with @Eeems about holding off decisions on how to split packages between rM1 and rM2 until when we have a toolchain available & enough testers owning rM2.

What kind of automated criteria would you suggest @Eeems? It seems to me that fully automating the move from testing to stable kind of defeats the purpose of having separate branches. Ideally, we’d have to find enough trustworthy maintainers so that at least one is available at all times.

I propose a two-branches system (stable and testing) which would work as follows:


  1. All pull requests regarding new packages or package updates are based on testing.
  2. A pull request can be merged into testing after review from a maintainer and if it builds successfully in the CI.
    • If the pull request is an update for an existing package, the preferred reviewer is the maintainer of that specific package.
    • If it adds a new package, the pull request’s reviewer becomes the maintainer of that package.
  3. After merging a package update or a new package into testing, it can be moved to stable if:
    • At least two full days have passed since the merge in testing.
    • At least one repository maintainer, different from the maintainer of that package, has successfully checked that the package works and does not break any other package.

Here are the rationales:

Under that system, the testing branch offers no stability guarantee and is only intended for testing by maintainers, and the stable branch is the only one that users need to use.

This is only a first draft, and any comment is welcome.

Eeems commented 3 years ago

So how do we want to manage moving things from testing to stable in terms of the actual git workflow? We can't merge testing itself into stable since it might have other package updates that aren't ready yet.

This would influence what I would even recommend for automated moving.

matteodelabre commented 3 years ago

I’m not very good at Git workflows. Would cherry-picking from testing to stable work? (Assuming that each commit only changes at most one package.)

Eeems commented 3 years ago

So it would be a manual process where someone would have to create a PR against stable with the cherry-picked commits?

matteodelabre commented 3 years ago

Package maintainers would have write access to the repo. So they could simply push to stable when the above conditions are met? Since moving a package from testing to stable does not require an external review, I’m not sure that PRs would be useful.

Eeems commented 3 years ago

PRs help avoid mistakes. I'd prefer not to be directly pushing to stable as an extra check to make sure I'm not doing something silly. That way the normal CI can alert you to errors before merging the branch so you can "stage" the push to stable as you sort through what you are adding in a "merge window" of sorts. You could have the migration to stable started at the start of the week and start adding things to it as you identify they are stable. And when you are ready to merge all the CI checking has already been done.

matteodelabre commented 3 years ago

These are good points. Having a merge window sure seems more efficient than moving packages one by one.

Eeems commented 3 years ago

And for the merge window we could automate its creation, and potentially sort out a way to automate the cherry-picking. One thing to note on cherry-picking is that you'll want to merge stable back into testing every so often so that it doesn't start thinking there are merge conflicts.

matteodelabre commented 3 years ago

Updated proposal:

  1. New packages or package updates are proposed through pull requests based on the testing branch.
    • A proposal can be merged into testing after review from a maintainer and if it builds successfully in the CI.
    • If it is a proposal for a new package, the maintainer who reviews the pull request becomes the maintainer for that package.
    • If it is a proposal for an update, the maintainer of that specific package should do the review.
  2. Package changes are moved to stable following this process:
    • Each Monday, a pull request is opened by a maintainer or a bot, which cherry-picks commits from testing to stable.
    • This pull request can only contain package changes dating from the previous Friday or older.
    • Each of these package changes must be tested by a maintainer different from the maintainer of the affected package.
  3. Things to check when testing a package:
    • The package should work.
    • It must not destroy user data from previous versions of the package.
    • It must not break other packages.
matteodelabre commented 3 years ago

One thing to note on cherry-picking is that you'll want to merge stable back into testing every so often so that it doesn't start thinking there are merge conflicts.

Why is that?

Eeems commented 3 years ago

Because cherry-picked commits are not the same commit. So technically you've changed the same file. Usually git is pretty good at realizing there is no conflict, but every so often it gets confused.

LinusCDE commented 3 years ago

Because cherry-picked commits are not the same commit. So technically you've changed the same file. Usually git is pretty good at realizing there is no conflict, but every so often it gets confused.

That's probably similar to how I had problems with adding multiple PRs. The rebase would find a lot of conflicts when I wanted to merge the upstream repo back into my fork. Where I had like 5 commits, the PR squashed it into one totally new one. My next PR would've then tried to also push my old commits that god squashed await in previous PRs. I usually then just created a new branch based on the current state of upstream/testing and did my changes there to avoid those problems.


@matteodelabre Your proposal looks really good. :+1:
Though we should account for people not wanting to maintain certain packages anymore. People would then still wait on those reviewers and delay the process possibly a long time. Maybe set a time limit when a PR is then free for other reviewers to do.

Eeems commented 3 years ago

Because cherry-picked commits are not the same commit. So technically you've changed the same file. Usually git is pretty good at realizing there is no conflict, but every so often it gets confused.

That's probably similar to how I had problems with adding multiple PRs. The rebase would find a lot of conflicts when I wanted to merge the upstream repo back into my fork. Where I had like 5 commits, the PR squashed it into one totally new one. My next PR would've then tried to also push my old commits that god squashed await in previous PRs. I usually then just created a new branch based on the current state of upstream/testing and did my changes there to avoid those problems.

One of the reasons I tend not to rebase :)

LinusCDE commented 3 years ago

Because cherry-picked commits are not the same commit. So technically you've changed the same file. Usually git is pretty good at realizing there is no conflict, but every so often it gets confused.

That's probably similar to how I had problems with adding multiple PRs. The rebase would find a lot of conflicts when I wanted to merge the upstream repo back into my fork. Where I had like 5 commits, the PR squashed it into one totally new one. My next PR would've then tried to also push my old commits that god squashed await in previous PRs. I usually then just created a new branch based on the current state of upstream/testing and did my changes there to avoid those problems.

One of the reasons I tend not to rebase :)

But how would you go about it then?

What would you choose as an alternative or have I overlooked any? I'm personally working with git for a long time now, but I'm still a student and this is one of my first times, I have major code merging going on that highlights this problems for me.

Eeems commented 3 years ago

But how would you go about it then?

* Create a new up-to-date branch / nuke a branch to the up-to-date state

* Just keep not updating until you get merge conflicts

What would you choose as an alternative or have I overlooked any? I'm personally working with git for a long time now, but I'm still a student and this is one of my first times, I have major code merging going on that highlights this problems for me.

I'd just merge the remote into my current branch and deal with the conflicts in the single commit. That way you don't have to merge every single commit.

The other more tedious option is to create a new branch and cherry-pick your commits over one at a time. That said, I'd only ever do that if a rebase showed a conflict on enough commits that it would take less time than just doing the rebase.

raisjn commented 3 years ago

But how would you go about it then?

it's unfortunate, but github doesn't support stacked diffs properly (which people use in industry). people have generated their own workflows with github that emulate stacked diff (albeit somewhat tedious but better than no stacked diffs).

from my perspective: i do not want to see people's merge commits, nor do i want to see the history of feature branches, so i always squash and merge. if you want to be in sync, you can squash and merge locally, doing force pushes to your PR branch (so the PR only ever has one commit in it). i haven't caught up on the thread, but i'd like to propose that PRs always get squash merged and people don't use git merge on non-feature branches (stable/testing) if it isn't already part of the process.

around the overall process: looking good, i still need to catch up and write comments though

raisjn commented 3 years ago

looking at this, i like everything i see!

re: cherry picks: i don't particularly understand the rationale for using cherry picks here (and i see there's discussion about the problems it would cause). if we could, i think it's fine to just copy the packages/ directory over from testing -> stable and make a pull request for each subdir that changed. there's still a question of the remaining files in the repo (the infra and tooling) - i think those might need to be cherry-picked (but its also fine to just blanket copy them). the reason i think its fine is because i don't see a reason we would want to merge from stable -> testing except for resolving the potential merge conflicts.

misc questions:

Eeems commented 3 years ago

re: cherry picks: i don't particularly understand the rationale for using cherry picks here (and i see there's discussion about the problems it would cause). if we could, i think it's fine to just copy the packages/ directory over from testing -> stable and make a pull request for each subdir that changed. there's still a question of the remaining files in the repo (the infra and tooling) - i think those might need to be cherry-picked (but its also fine to just blanket copy them). the reason i think its fine is because i don't see a reason we would want to merge from stable -> testing except for resolving the potential merge conflicts.

Cherry-picks or a single commit where you just grab the apps you care about would basically be the same end result and have the same pitfalls.

misc questions:

  • what if a developer of software opens a PR with a bugfix that they think is urgent, can they get that merged into stable more quickly?

I'd suggest we handle that on a case by case basis.

  • can a package be maintained by more than one person?

You'd still want one person named as the primary maintainer.

raisjn commented 3 years ago

i'm not suggesting picking commits, i'm suggesting just doing copy of files, then make a commit for each package dir. i don't think we will ever need to merge from stable -> testing, so this would be ok. but since we never need to merge stable -> testing, maybe cherry picks are fine too

Eeems commented 3 years ago

i'm not suggesting picking commits, i'm suggesting just doing copy of files, then make a commit for each package dir. i don't think we will ever need to merge from stable -> testing, so this would be ok. but since we never need to merge stable -> testing, maybe cherry picks are fine too

Right, which has the same end result as a cherry-pick, and the same pitfalls. That said, if you are saying we are branching off of stable and copying the files in, that would negate the concern with merge conflicts.

matteodelabre commented 3 years ago

Though we should account for people not wanting to maintain certain packages anymore. People would then still wait on those reviewers and delay the process possibly a long time. Maybe set a time limit when a PR is then free for other reviewers to do.

I think if a maintainer is unresponsive about a PR, they will probably be unresponsive for subsequent ones, so we might as well allow maintainers to orphan their packages, i.e. allow packages to have no maintainer. In this case, any maintainer can review the PR, and this person will become the new maintainer of the package.

matteodelabre commented 3 years ago

from my perspective: i do not want to see people's merge commits, nor do i want to see the history of feature branches, so i always squash and merge. if you want to be in sync, you can squash and merge locally, doing force pushes to your PR branch (so the PR only ever has one commit in it). i haven't caught up on the thread, but i'd like to propose that PRs always get squash merged and people don't use git merge on non-feature branches (stable/testing) if it isn't already part of the process.

Seems sensible enough that PRs must be squash merged. Unfortunately, there doesn’t seem to be a GitHub option to enforce that. I added the following branch protection rules to testing and stable, which are better than nothing:

Screenshot of added branch protection rules: Require 1 pull request review before merging, Dismiss stale pull requests approvals when new commits are pushed, Require status checks to pass before merging, Require branches to be up to date before merging, Require linear history, Include administrators

matteodelabre commented 3 years ago

So, what emerges from the above discussion seems to be that the testing -> stable PRs will contain one commit in which we copy the updated recipes that we want to move to stable, plus updated infra files if necessary. Does everyone agree on that?

Eeems commented 3 years ago

Seems sensible enough that PRs must be squash merged. Unfortunately, there doesn’t seem to be a GitHub option to enforce that. I added the following branch protection rules to testing and stable, which are better than nothing:

The only way to do that would be to enforce it repo wide. That said, it might not be a bad idea, and we just don't use PRs to do the testing -> stable move. image

Edit:

So, what emerges from the above discussion seems to be that the testing -> stable PRs will contain one commit in which we copy the updated recipes that we want to move to stable, plus updated infra files if necessary. Does everyone agree on that?

Actually I guess enforcing squash merge everywhere would work with that.

matteodelabre commented 3 years ago

Seems sensible enough that PRs must be squash merged. Unfortunately, there doesn’t seem to be a GitHub option to enforce that. I added the following branch protection rules to testing and stable, which are better than nothing:

The only way to do that would be to enforce it repo wide. That said, it might not be a bad idea, and we just don't use PRs to do the testing -> stable move. image

Didn’t know about this option, thanks! I just changed it.

raisjn commented 3 years ago

PRs will contain one commit in which we copy the updated recipes that we want to move to stable, plus updated infra files if necessary.

this sounds fine to me (and less work in total), but originally i imagined that we have one PR per package move (and the PR is just a blank copy of package/$PACKAGE/ from testing -> stable) and the owner can accept the PRs for their packages.

matteodelabre commented 3 years ago

This is related to the merge window concept proposed by @Eeems above. As you say, it will be less work than creating separate PRs, so I think we’ll go with that unless there are clear advantages for creating separate PRs.

raisjn commented 3 years ago

with separate pr, then each maintainer can choose what to migrate and when. with one pr it means coordinating across multiple people while they figure out if packages are safe to be migrated

i still think one is fine