# Issue with patches section of third-party packages developer doc

XMLWordPrintable

## Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Team:
SQuaRE

## Description

John Parejko reports:

The "patches" section of the third party packages page is problematic:
http://developer.lsst.io/en/latest/build-ci/third_party.html#the-patches-directory
Since the files themselves aren't checked in to git, but rather the tarballs, the git command listed there won't work.

## Activity

Hide
Kian-Tat Lim added a comment -

The text given is correct if you're sitting in a clone of the upstream, which you usually are while preparing the third-party package, since you need to create the tarball. But a more explicit recipe could be given.

Show
Kian-Tat Lim added a comment - The text given is correct if you're sitting in a clone of the upstream, which you usually are while preparing the third-party package, since you need to create the tarball. But a more explicit recipe could be given.
Hide
John Parejko added a comment - - edited

Kian-Tat Lim I'm confused by your statement. Why would I be sitting on a clone of upstream, instead of just using the tarballs provided by upstream? That seems like the better choice to me.

Also, the instructions don't help if you want to take just one diff from something that has had a lot of recent work. The example that made me complain about this point is a good one: there's been a lot of work on the relevant files upstream, but we only want one change for the time being: https://bitbucket.org/eigen/eigen/commits/6996ab297dedb4684a708e23cee3002906a4ef1c/raw/

I think some more details about the patches directory would be handy. It was not clear to me whether a patch like the above should work or not.

Show
John Parejko added a comment - - edited Kian-Tat Lim I'm confused by your statement. Why would I be sitting on a clone of upstream, instead of just using the tarballs provided by upstream? That seems like the better choice to me. Also, the instructions don't help if you want to take just one diff from something that has had a lot of recent work. The example that made me complain about this point is a good one: there's been a lot of work on the relevant files upstream, but we only want one change for the time being: https://bitbucket.org/eigen/eigen/commits/6996ab297dedb4684a708e23cee3002906a4ef1c/raw/ I think some more details about the patches directory would be handy. It was not clear to me whether a patch like the above should work or not.
Hide
Tim Jenness added a comment -

I think we are over analyzing things here. See the actual code:

https://github.com/RobertLuptonTheGood/eups/blob/master/lib/eupspkg.sh#L712

So the patch files in the patches directory are applied using:

 patch -s -p1 

so that actually defines how it's going to work.

Show
Tim Jenness added a comment - I think we are over analyzing things here. See the actual code: https://github.com/RobertLuptonTheGood/eups/blob/master/lib/eupspkg.sh#L712 So the patch files in the patches directory are applied using: patch -s -p1 so that actually defines how it's going to work.
Hide
Kian-Tat Lim added a comment -

Yes, there's no question about how it works in practice.

My comment was meant to reflect an expected environment in which someone discovered a problem with a third-party package, cloned it to debug the problem, and came up with a modification that cured the problem. I conflated that (perhaps mistakenly) with generating the original tarball for the package to go in the upstream directory. In any case, it wasn't intended for the case of cherry-picking a commit from upstream, although that should also not be too hard; I would expect that the commit above can be applied as-is.

Show
Kian-Tat Lim added a comment - Yes, there's no question about how it works in practice. My comment was meant to reflect an expected environment in which someone discovered a problem with a third-party package, cloned it to debug the problem, and came up with a modification that cured the problem. I conflated that (perhaps mistakenly) with generating the original tarball for the package to go in the upstream directory. In any case, it wasn't intended for the case of cherry-picking a commit from upstream, although that should also not be too hard; I would expect that the commit above can be applied as-is.
Hide
John Swinbank added a comment -

It looks to me as if this ticket was (mostly) addressed in https://github.com/lsst-dm/dm_dev_guide/commit/d2b6bd3c77ea2ed39b30ae5a4d726cf2f602d51d (which wasn't itself ticketed, as far as I can see).

The one leftover anomaly seems to be the “note” that:

EUPS expects the patches to be formatted according to the output of git diff, not the output of diff.

This is particularly surprising, since the instructions are now to use diff, not git diff.

I propose to update that note to read:

EUPS expects the patches to be in unified format, as generated by the -u option to the diff command.

Show
John Swinbank added a comment - It looks to me as if this ticket was (mostly) addressed in https://github.com/lsst-dm/dm_dev_guide/commit/d2b6bd3c77ea2ed39b30ae5a4d726cf2f602d51d (which wasn't itself ticketed, as far as I can see). The one leftover anomaly seems to be the “note” that: EUPS expects the patches to be formatted according to the output of git diff, not the output of diff. This is particularly surprising, since the instructions are now to use diff , not git diff . I propose to update that note to read: EUPS expects the patches to be in unified format , as generated by the -u option to the diff command.
Hide
John Swinbank added a comment -
Show
John Swinbank added a comment - PR: https://github.com/lsst-dm/dm_dev_guide/pull/331
Hide
John Parejko added a comment -

Tim Jenness and I have the same comment, but otherwise thanks for cleaning this up.

Show
John Parejko added a comment - Tim Jenness and I have the same comment, but otherwise thanks for cleaning this up.
Hide
John Swinbank added a comment -

Thanks for the fast turnaround. Merged.

Show
John Swinbank added a comment - Thanks for the fast turnaround. Merged.

## People

• Assignee:
Unassigned
Reporter:
Jonathan Sick
Reviewers:
John Parejko
Watchers:
John Parejko, John Swinbank, Jonathan Sick, Kian-Tat Lim, Tim Jenness