releng/tickets

Fork 2

Add script to check if a branch can be deleted from dist-git #9454

Merged

jnsamyak merged 5 commits from distgit-commit-unused into master

2026-06-08 03:11:36 +00:00

zbyszek commented

2020-05-13 13:20:16 +00:00

Contributor

Co-authored-by: Igor Raits ignatenkobrain@fedoraproject.org

https://pagure.io/releng/issue/9068
https://pagure.io/fesco/issue/2387
https://pagure.io/fesco/issue/2340

Signed-off-by: Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl

Co-authored-by: Igor Raits <ignatenkobrain@fedoraproject.org> https://pagure.io/releng/issue/9068 https://pagure.io/fesco/issue/2387 https://pagure.io/fesco/issue/2340 Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>

zbyszek commented

2020-05-13 13:24:50 +00:00

Author

Contributor

rebased onto 4604a4232332e78ac8228c47080224ee2e250c7e

zbyszek commented

2020-05-13 13:25:09 +00:00

Author

Contributor

Examples:
$ scripts/distgit-commit-unused.py --repository ~/fedora/systemd foo2
Working on package systemd in /home/zbyszek/fedora/systemd
Branch has commits not found anywhere else. Looking for builds.
0: afe51a0 foo2
1: a32a98a Merge branch 'master' into f32
Commit a32a98a0fa2fdee376e10eb53f67987fe1f3cc10 referenced from refs/remotes/origin/f32. Stopping iteration.
No builds found, seems OK to delete.

$ scripts/distgit-commit-unused.py --repository ~/fedora/systemd f32
Working on package systemd in /home/zbyszek/fedora/systemd
Branch has commits not found anywhere else. Looking for builds.
0: e3554e7 Move Provides:u2f-hidraw-policy to -udev subpackage
1: 8ae0be2 gitignore: add emacs backup files
2: 1a7473c Add abignore file to make abigail happy
3: 27e3b91 Fix some rpmlint issues and add filter for others
Sorry, 27e3b915a47af78655dc5cffdf277f16c76e4525 built as systemd-245.4-1.fc32.
See https://koji.fedoraproject.org/koji/taskinfo?taskID=42954546.

Examples: $ scripts/distgit-commit-unused.py --repository ~/fedora/systemd foo2 Working on package systemd in /home/zbyszek/fedora/systemd Branch has commits not found anywhere else. Looking for builds. 0: afe51a0 foo2 1: a32a98a Merge branch 'master' into f32 Commit a32a98a0fa2fdee376e10eb53f67987fe1f3cc10 referenced from refs/remotes/origin/f32. Stopping iteration. No builds found, seems OK to delete. $ scripts/distgit-commit-unused.py --repository ~/fedora/systemd f32 Working on package systemd in /home/zbyszek/fedora/systemd Branch has commits not found anywhere else. Looking for builds. 0: e3554e7 Move Provides:u2f-hidraw-policy to -udev subpackage 1: 8ae0be2 gitignore: add emacs backup files 2: 1a7473c Add abignore file to make abigail happy 3: 27e3b91 Fix some rpmlint issues and add filter for others Sorry, 27e3b915a47af78655dc5cffdf277f16c76e4525 built as systemd-245.4-1.fc32. See https://koji.fedoraproject.org/koji/taskinfo?taskID=42954546.

ignatenkobrain commented

2020-05-13 14:08:00 +00:00

Contributor

I think you could add this and next arguments into the mutually exclusive group so that you won't specify both.

ignatenkobrain commented

2020-05-13 14:15:47 +00:00

Contributor

Great job, @zbyszek!

I think I have only one concern here. It can happen that:

Repo foo exists and is active
Repo foo2 (like a compat version of foo) exists and is active
People have Name: foo in foo2.spec and make a build
Koji will record this package as foo, even though it was built from foo2 repo

In that case, we will allow deletion of commits from foo2 repo even though they were used. I don't think this is happening often and most likely is some corner case, but I think it is important to be aware of this. It actually happened to me few times with Rust packages. Probably solution to this problem can be enforcement of same dist-git repo as package name in koji, but it is not in place.

Great job, @zbyszek! I think I have only one concern here. It can happen that: * Repo `foo` exists and is active * Repo `foo2` (like a compat version of foo) exists and is active * People have `Name: foo` in `foo2.spec` and make a build * Koji will record this package as `foo`, even though it was built from `foo2` repo In that case, we will allow deletion of commits from `foo2` repo even though they were used. I don't think this is happening often and most likely is some corner case, but I think it is important to be aware of this. It actually happened to me few times with Rust packages. Probably solution to this problem can be enforcement of same dist-git repo as package name in koji, but it is not in place.

zbyszek commented

2020-05-13 16:22:40 +00:00

Author

Contributor

rebased onto 90439edc106ff212b4a99ff82fe760ab0a361a4b

zbyszek commented

2020-05-13 16:27:29 +00:00

Author

Contributor

Reworked. Two changes:

@churchyard raised the point that the script would say that e.g. "f28" is contained by "origin/f28" when running in a clone. While technically true, this is not very useful. So now when checking a remote branch, only remote branches are taken into account, and when checking a local branch, only local branches are taken into account. (The first mode is for users, the second for releng.)
To answer @ignatenkobrain's case above, the spec file is extracted for each commit and Name is extracted. If it doesn't match the package name, the script refuses to continue.

(That second check might reject some cases that could possibly be supported, e.g. when a macro is used in the name. If somebody has a case like that and they care enough, they can fix the script to parse the file using rpm. Hopefully such cases are rare, so I didn't bother doing that.)

Reworked. Two changes: - @churchyard raised the point that the script would say that e.g. "f28" is contained by "origin/f28" when running in a clone. While technically true, this is not very useful. So now when checking a remote branch, only remote branches are taken into account, and when checking a local branch, only local branches are taken into account. (The first mode is for users, the second for releng.) - To answer @ignatenkobrain's case above, the spec file is extracted for each commit and Name is extracted. If it doesn't match the package name, the script refuses to continue. (That second check might reject some cases that could possibly be supported, e.g. when a macro is used in the name. If somebody has a case like that and they care enough, they can fix the script to parse the file using rpm. Hopefully such cases are rare, so I didn't bother doing that.)

zbyszek commented

2020-05-15 13:32:35 +00:00

Author

Contributor

rebased onto 09c32ed2dc41225e9aa3c3c0d5fd098cb2902535

zbyszek commented

2020-05-15 13:41:30 +00:00

Author

Contributor

5 new commits added

Add a work-around for very old builds not having a source
Do not allow removing release branches from which builds have been made
When checking builds, extract the spec file and verify Name: is set set correctly
Look at local and remote branches separately
Add script to check if a branch can be deleted from dist-git

**5 new commits added** * ``Add a work-around for very old builds not having a source`` * ``Do not allow removing release branches from which builds have been made`` * ``When checking builds, extract the spec file and verify Name: is set set correctly`` * ``Look at local and remote branches separately`` * ``Add script to check if a branch can be deleted from dist-git``

zbyszek commented

2020-05-15 13:42:44 +00:00

Author

Contributor

Updated to handle @churchyard's request in https://pagure.io/fesco/issue/2387#comment-652070: do not allow removal of branches which were used to build anything.

@ignatenkobrain I also found another case where koji doesn't know the source for a build, see last commit.

Updated to handle @churchyard's request in https://pagure.io/fesco/issue/2387#comment-652070: do not allow removal of branches which were used to build anything. @ignatenkobrain I also found another case where koji doesn't know the source for a build, see last commit.

zbyszek commented

2020-05-25 08:05:56 +00:00

Author

Contributor

/cc @kevin, @mohanboddu

mohanboddu commented

2020-06-02 15:42:40 +00:00

Contributor

@zbyszek Can you rebase the PR as it has some merge conflicts?

Otherwise, its looking good.

Thanks.

@zbyszek Can you rebase the PR as it has some merge conflicts? Otherwise, its looking good. Thanks.

kevin commented

2020-06-02 15:42:59 +00:00

Owner

Script seems fine to me, as long as it expressed the policy that fesco approves/wants.

zbyszek commented

2020-06-02 16:38:30 +00:00

Author

Contributor

rebased onto a55d413cdf

rebased onto a55d413cdfd2a896e536095bf5677e928a7acb95

zbyszek commented

2020-06-02 16:50:46 +00:00

Author

Contributor

Can you rebase the PR as it has some merge conflicts?

It can't have conflicts, because it just adds a new file. I rebased in case you want to have linear history.

Script seems fine to me, as long as it expressed the policy that fesco approves/wants.

I think it satisfies the spirit of the policy approved by FESCo in #2340. The script is more restrictive in some regards, because it rejects branches from which packages have been built, even all the commits in those branches are reachable from other branches. OTOH, it permits removal of branches which have commits which are not reachable from a different branch, as long as none of those commits were used to successfully build real packages in koji. Those additional details match what was requested in the ticket and discussed before approving the one-sentence policy, even if the one-sentence policy does not explicitly express this. So yeah, I think this is good to go.

> Can you rebase the PR as it has some merge conflicts? It can't have conflicts, because it just adds a new file. I rebased in case you want to have linear history. > Script seems fine to me, as long as it expressed the policy that fesco approves/wants. I think it satisfies the spirit of the policy approved by FESCo in #2340. The script is more restrictive in some regards, because it rejects branches from which packages have been built, even all the commits in those branches are reachable from other branches. OTOH, it permits removal of branches which have commits which are not reachable from a different branch, as long as none of those commits were used to successfully build real packages in koji. Those additional details match what was requested in the ticket and discussed before approving the one-sentence policy, even if the one-sentence policy does not explicitly express this. So yeah, I think this is good to go.

kevin commented

2020-06-04 04:09:05 +00:00

Owner

I think it satisfies the spirit of the policy approved by FESCo in #2340. The script is more restrictive in some regards, because it rejects branches from which packages have been built, even all the commits in those branches are reachable from other branches. OTOH, it permits removal of branches which have commits which are not reachable from a different branch, as long as none of those commits were used to successfully build real packages in koji. Those additional details match what was requested in the ticket and discussed before approving the one-sentence policy, even if the one-sentence policy does not explicitly express this. So yeah, I think this is good to go.

well, shouldn't fesco formally vote on it before we just decide it's ok and start using it? I know it may be a formality, but would be good for everything to be acked.

> I think it satisfies the spirit of the policy approved by FESCo in #2340. The script is more restrictive in some regards, because it rejects branches from which packages have been built, even all the commits in those branches are reachable from other branches. OTOH, it permits removal of branches which have commits which are not reachable from a different branch, as long as none of those commits were used to successfully build real packages in koji. Those additional details match what was requested in the ticket and discussed before approving the one-sentence policy, even if the one-sentence policy does not explicitly express this. So yeah, I think this is good to go. well, shouldn't fesco formally vote on it before we just decide it's ok and start using it? I know it may be a formality, but would be good for everything to be acked.