fedpkg

Clone

#429 EPEL Next support
Merged 3 years ago by onosek. Opened 3 years ago by carlwgeorge.
carlwgeorge/fedpkg epel-next  into  master

file modified
+7 
@@ -150,6 +150,13 @@ 
 

              self.mockconfig = 'epel-%s-%s' % (self._distval, self.localarch)
 

              self.override = 'epel%s-override' % self._distval
 

              self._distunset = 'fedora'
 

+         elif re.match(r'epel\d+-next$', self.branch_merge):
 

+             self._distval = re.search(r'\d+', self.branch_merge).group(0)
 

+             self._distvar = 'rhel'
 

+             self._disttag = 'el%s.next' % self._distval
 

+             self.mockconfig = 'epel-next-%s-%s' % (self._distval, self.localarch)
 

+             self.override = 'epel%s-next-override' % self._distval
 

+             self._distunset = 'fedora'
 

          elif re.match(r'olpc\d$', self.branch_merge):
 

              self._distval = self.branch_merge.split('olpc')[1]
 

              self._distvar = 'olpc'

file modified
+6 -1 
@@ -1044,8 +1044,12 @@ 
 

                  *list(get_release_branches(pdc_url).values())))
 

              # treat epel*-playground the same as epel* release branches
 

              playground_match = re.match(r'^(epel\d+)-playground$', branch)
 

+             # treat epel*-next the same as epel* release branches
 

+             next_match = re.match(r'^(epel\d+)-next$', branch)
 

              if playground_match:
 

                  _branch = playground_match.groups()[0]
 

+             elif next_match:
 

+                 _branch = next_match.groups()[0]
 

              else:
 

                  _branch = branch
 

              if _branch in release_branches:
 
@@ -1101,7 +1105,8 @@ 
 

              auto_module = (
 

                  ns == 'rpms'
 

                  and not re.match(RELEASE_BRANCH_REGEX, b)
 

-                 and not playground_match  # Dont run auto_module on epel requests
 

+                 and not playground_match  # Dont run auto_module on epel-playground requests
 

+                 and not next_match        # Dont run auto_module on epel-next requests
 

                  and not no_auto_module
 

              )
 

              if auto_module:

file modified
+5 -1 
@@ -326,7 +326,7 @@ 
 

      :param branch: a string of the branch name
 

      :return: a boolean
 

      """
 

-     return bool(re.match(r'^(?:el|epel)\d+$', branch))
 

+     return bool(re.match(r'^(?:el|epel)\d+(?:-next)?$', branch))
 

  

  

  def assert_valid_epel_package(name, branch):
 
@@ -450,6 +450,10 @@ 
 

          # target to build.
 

          elif re.match(r'^epel\d+-playground$', item['name']):
 

              continue
 

+         # epel8-next and above branches should be considered as release branches
 

+         # so that it will use epelX-next-candidate target to build.
 

+         elif re.match(r'^epel\d+-next$', item['name']):
 

+             continue
 

          else:
 

              stream_branches.append(item)
 

      return stream_branches

file modified
+15 
@@ -262,6 +262,21 @@ 
 

          self.assert_rpmdefines()
 

  

      @patch('pyrpkg.Commands.branch_merge', new_callable=PropertyMock)
 

+     def test_load_epel8_next_dist_tag(self, branch_merge):
 

+         branch_merge.return_value = 'epel8-next'
 

+ 

+         self.cmd.load_rpmdefines()
 

+ 

+         self.assertEqual('8', self.cmd._distval)
 

+         self.assertEqual('rhel', self.cmd._distvar)
 

+         self.assertEqual('el8.next', self.cmd._disttag)
 

+         self.assertEqual('epel-next-8-i686', self.cmd.mockconfig)
 

+         self.assertEqual('epel8-next-override', self.cmd.override)
 

+         self.assertTrue(hasattr(self.cmd, '_distunset'))
 

+ 

+         self.assert_rpmdefines()
 

+ 

+     @patch('pyrpkg.Commands.branch_merge', new_callable=PropertyMock)
 

      @patch('fedpkg.Commands._findrawhidebranch')
 

      def test_load_rawhide_dist_tag(self, _findrawhidebranch, branch_merge):
 

          _findrawhidebranch.return_value = '28'

Signed-off-by: Carl George carl@george.computer

rebased onto 8afa6c01d700127e0ba130a59adb70b9653da314
3 years ago

rebased onto c9f6caf7988fb29afc9bf3ee5d99300c916bc22d
3 years ago

rebased onto 242972452520b020da28fad19939820ba65944f1
3 years ago

3 new commits added

  • is_epel regex should also match playground/next branches
  • is_epel regex no longer needs to match el6
  • Add support for epel*-next branches
3 years ago

LGTM, please merge this.

LGTM also

Would be nice to get in soon. :)

"elif" belongs here to be combined with the previous (epelX-playground) match. Otherwise, you will get the wrong result for epel8-playground branch.

I will fix it and make a patch now. But I will consider only the first commit - epel-next. There are two other commits that need to be understood (by me).
As per your change, epel6 shouldn't be count as epel release. I know that RHEL6 support was ended in December. But there is still some extended support and I am not sure whether I can remove everything regarding this (there is more epel6 related functionality in the code). There is another source (fedpkg queries it):
https://pdc.fedoraproject.org/product-version/epel-6/
it shows epel6 is still active. And there is no mention about epel8-playground. So as per this source, I can not count epel8-playground as epel release. I need some clarification on it.
Edited 3 years ago by onosek

Updates for fedpkg-1.40-6 patch are waiting in Bodhi.

rebased onto 755b31c6f741824a6946b957c61f7c739b935bcc
3 years ago

Thanks for catching that problem with fedpkg/cli.py. I adjusted it as requested.

The is_epel function is only invoked when requesting a new branch and when a branch is being retired. Neither of these should ever happen again for an el6 branch. el6 branches are already marked as inactive in dist-git, there is no epel6 build target in koji, and EL-6 is marked as archived in bodhi. Regardless of rhel6 extended support, epel6 is retired. That said, it doesn't hurt anything to keep matching the old pattern, so I can remove that commit if you like.

The is_epel functions should return true for epel*-playground and epel*-next branches. When requesting a new branch, that check is used to verify that package is allowed in epel by checking against a json file of rhel packages. When retiring a branch, that check is used to always allow retiring an epel branch, because we don't require epel maintainers to maintain their packages for a full decade.

That's my justification for those two commits based on my understanding, if I'm missing anything please let me know.

Thanks for your opinion. I would like to lose el6 mentions where it is possible. I have already noticed one issue with retiring when I was analysing it: retiring. What is the correct behaviour when a developer tries to retire their own el6 branch? Allow or not? Maybe it is irrelevant, but when el6 is not counted as epel release, retiring won't be allowed by default. A state of the release is verified in Bodhi (as you mentioned - archived). It means fedpkg should not allow the "retire" operation. But there is a bug I found: fedpkg queries Bodhi with a wrong URL: https://bodhi.fedoraproject.org/releases/el6
As result, fedpkg will allow "retire". So is it something I should repair (and have el6 mention here for another decade) or forget?

If code behaves as expected, only unit-tests should be repaired to pass:

diff --git a/test/test_cli.py b/test/test_cli.py
index 4327990..35c3bc3 100644
--- a/test/test_cli.py
+++ b/test/test_cli.py
@@ -1014,7 +1014,7 @@ class TestRequestBranch(CliTestCase):
     def test_request_epel_playground_branch_override(
         self, mock_grb, mock_request_post, mock_request_get
     ):

-        """Tests request-epel-branch-override"""
+        """Tests request-epel-playground-branch-override"""
         mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'],
                                  'epel': ['el6', 'epel7', 'epel8']}
         mock_rv = Mock()
@@ -2400,11 +2400,11 @@ class TestRetire(CliTestCase):
         self.do_not_retire_release("f30", "current")
         self.do_not_retire_release("f30", "archived")
         self.retire_release("unknown_fedora_release", None)
-        # epel release
-        self.retire_release("el6", "disabled")
+        # el6 is not epel release now
+        self.do_not_retire_release("el6", "archived")
         self.retire_release("el6", "pending")
-        self.retire_release("el6", "frozen")
         self.retire_release("el6", None)
+        # epel release
         self.retire_release("epel7", "current")
         self.retire_release("epel7", "archived")
         self.retire_release("epel7", "pending")

2 new commits added

  • is_epel regex should also match playground/next branches
  • Add support for epel*-next branches
3 years ago

I removed the commit as requested, so that is_epel continues to match el6.

All el6 branches should already be retired, but if for some reason there are still some out there then a maintainer should be allowed to retire it at any time.

Tweak this for el8~next and we're gravy. :wink:

Would be nice to get this out... the rest of epel-next is close to working/setup...

Tweak this for el8~next and we're gravy.

There are still build systems running on EL7 that lack support for ~ and ^ versions/releases. Rebuilding EPEL packages with those characters in the disttag would fail there. This was recently the case for the CentOS Community Build System.

While I like the idea of taking advantage of ~, it doesn't gain us anything here that a routine release bump doesn't. I'd prefer to stick with .el8.next for now.
Edited 3 years ago by carlwgeorge

Tweak this for el8~next and we're gravy.

There are still build systems running on EL7 that lack support for ~ and ^ versions/releases. Rebuilding EPEL packages with those characters in the disttag would fail there. This was recently the case for the CentOS Community Build System.

While I like the idea of taking advantage of ~, it doesn't gain us anything here that a routine release bump doesn't. I'd prefer to stick with .el8.next for now.

The ~ modifier has been supported from the beginning in EL7 (it was introduced in RPM 4.10), it was backported to EL6 as well. Only ^ is not supported in EL7.

@kevin functionality of the PR (except the minor change ae50ec8) is released as a patch since Apr-3 in fedpkg-1.40-6

The patch I am talking about doesn't contain the ~ or ^ functionality. Is this something that has to be released soon?

Nope. As long as fedpkg can handle next branches I think we are fine.

It's unclear to me what advantage ~ or ^ gets us.

From our discussion in the EPEL Steering Committee meeting, I know that Neal is thinking about the upgrade path.

The build tag, this PR, and the backport patch) were set up for a disttag of .el8.next. With that disttag, the upgrade path would look like this:

foo-1.0-1.el8 linked against rhel8's bar-2.0-1.el8
foo-1.0-2.el8.next linked against cs8's bar-3.0-1.el8
foo-1.0-3.el8 linked against rhel8's bar-3.0-1.el8

In other words, a release bump every time it's rebuilt (which is what I expect most maintainers will do out of habit). If we change the disttag to .el8~next, it would allow this upgrade path:

foo-1.0-1.el8 linked against rhel8's bar-2.0-1.el8
foo-1.0-2.el8~next linked against cs8's bar-3.0-1.el8
foo-1.0-2.el8 linked against rhel8's bar-3.0-1.el8

All this gains us is one less release bump. The commit from the epel8-next branch could be merged to the epel8 branch and built as is.
Edited 3 years ago by carlwgeorge

Ah, I see. ok then, I have no further objection to the ~

The downside to using ~ is that it would break the upgrade path if a maintainer builds the same commit for both epel8 and epel8-next while a library difference exists, which is something I expect to happen often with active maintainers.

foo-1.5-1.el8 linked against rhel8's bar-2.0-1.el8
foo-1.5-1.el8~next linked against cs8's bar-3.0-1.el8

The downside to using ~ is that it would break the upgrade path if a maintainer builds the same commit for both epel8 and epel8-next while a library difference exists, which is something I expect to happen often with active maintainers.

foo-1.5-1.el8 linked against rhel8's bar-2.0-1.el8
foo-1.5-1.el8~next linked against cs8's bar-3.0-1.el8

I think we should encourage people to do a bump initially when rebuilding for new dependencies, and then when the new dependencies arrive to RHEL, we can just rebuild from the same commit without a new changelog entry. In that scenario, using ~ will be fine.

That said, I can definitely see your point, though I'm not sure how frequently that would be happening, relative to the elX -> csX -> elX+1 transition that happens every six months. Do you think this would be a significant issue? This could also happen regardless of this change, and then I would wonder if we might want to make DNF consider epel-next repo higher than epel repo...

Encouraging people wouldn't be enough, we'd have to require it for the upgrade path to consistently work. With .el8.next it works either way. That matches the behavior in Fedora when updating a package across multiple Fedora branches. I can update a version or add a patch in a commit, then build that same commit across as many branches as I like. Always requiring a separate release bump for epel8-next builds would cause maintainers extra work when they need to update a package in both epel8 and epel8-next. That would discourage active maintenance.

Okay I suppose. This isn't a hill I'm going to die on. I'm just annoyed that I'll have to have two rebuild bumps every time for this situation.
Edited 3 years ago by ngompa

rebased onto 67d8342955fd43d20be741b234c762565af9741f
3 years ago

@onosek I removed el6 change from this PR. Are there any other changes you'd like to see to get this merged?

rebased onto aee3833
3 years ago

I also removed everything about playground to keep this simple, and added a test_load_epel8_next_dist_tag test for completeness.

Thanks, @carlwgeorge, I am merging it.

Pull-Request has been merged by onosek
3 years ago
Metadata
None
No Tags
Changes Summary 4
+7 -0
file changed
fedpkg/__init__.py
+6 -1
file changed
fedpkg/cli.py
+5 -1
file changed
fedpkg/utils.py
+15 -0
file changed
test/test_commands.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.