Guess strip for patch from patch itself

[teselkin]

Some RPM spec files use non-standart way of applying patches: they don't use %patch macro but call 'patch' directly from inlined script or use 'git apply'. In such cases gbp can't detect patch strip value and fails. For example, take a look at centos spec for libvirt [1].

This patch adds a bit of logic to guess patch strip value from patch itself if possible.

[1] https://git.centos.org/blob/rpms!!libvirt/d7192236d3c0510c5debbd75087d36d481a4ead1/SPECS!libvirt.spec

[agx]

Thanks, one issue though:

The patch object might not refer to an existing file (yet) so guessing might fail. I'd rather see the code to guess the strip level called separately for the RPM guessing case. Maybe @marquiz has another comment.

[agx]

...oh and we need a testcase that checks this one we know which way we want it.

[teselkin]

I'm trying to guess the strip level by analyzing paths to files being patched, but without checking if they exist because of the same reason you gave - file might not exist. +1 for tests, I'll add them.

[marquiz]

How does gbp fail in this case? Currently, gbp does not play out nicely with these kind of spec files as it basically ignores (or at least should ignore) the patches. Gbp should not actually fail because it doesn't recognize that the patches are applied and it should not import/export them at all.

Anyway, I'm ok with the patch – with the unit test added (plu see my code comment).

[teselkin]

It fail to import patches, in fact, it just ignore them. There is another patch (that looks like a hack) that help me to import those patches by force. I can send it too. I've updated the patch, should I squash the commits to one?

[agx]

Thanks for the update but please check how gbp-pq uses the patch series. It parses the series file without ever looking into any patch file on disk (since the strip level is in there as well).

I want to retain this behaviour for the gbp-pq case. It's perfectly fine for pq_rpm's safe_patches/patchseries to look at the patch files and determine the strip level but not by Path's making the default constructor require a file on disk. E.g. move the guessing code into an alternate "constructor" like class method:

Patch(object):
  ...

  @classmethod
  def from_file(cls, patch):
    strip = _guess_patch_level(path)
    return cls.__init__(path, strip, ...)

and use Patch.from_file to build the patch object where you need it. This will also make it obvious where we have the strip level guessed and where not.

Oh, and it would be awesome if we could move these things either onto the Debian bugtracker or gbp's mailing list. Githubs Web-UI is just not suited for any kind of code review.

[marquiz]

Do you have that other patch (that looks like a hack) somewhere? I think it would be trivial to add for "force importing" all patches (i.e. all Patch: tags).

The problem with that is that without manually modifying the spec file gbp will not be able to build/modify the it correctly: gbp-pq export will add %patch macros for each Patch: tag and you would end up with patches applied twice. It might be worthwhile to add such a "force" option to gbp-import-srpm, though. It should probably print a warning if patches without a corresponding %patch macro are found, so that the user would be instructed to modify the spec file by hand to make it gbp-compatible.

[teselkin]

@agx I'll try to add separate method as you've suggested, and will update the patch. Unfortunately, I'm not familiar with code-review via mailing list. I'll send any new patch to ML, but could we continue to discuss this patch here? And, would it be OK if I create a pull-request for new patch here and send it to mailing list, or it's necessary to send patches to ML?

@marquiz Those patches in my private branch, I'll update them a bit and will send a bit later. Yes, I have also a patch to not create %patch macros when exporting :) I'm going to combine them together then.

[agx]

On Fri, May 20, 2016 at 01:34:01AM -0700, Dmitry Teselkin wrote:

@agx I'll try to add separate method as you've suggested, and will update the patch. Unfortunately, I'm not familiar with code-review via mailing list. I'll send any new patch to ML, but could we continue to discuss this patch here? And, would it be OK if I create a pull-request for new patch here and send it to mailing list, or it's necessary to send patches to ML?

Just stick to the current workflow for this PR - i think I can manage. It's just in case there are new (and non trivial) ones we should check how to not having to review code via the UI.

[teselkin]

@agx I'm just curious, have you seen GerritHub? It's quite useful for code-review.