Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5184

Issue with patches section of third-party packages developer doc

    Details

    • 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.

        Attachments

          Issue Links

            Activity

            Hide
            ktl 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
            ktl 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
            Parejkoj 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
            Parejkoj 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
            tjenness 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
            tjenness 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
            ktl 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
            ktl 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
            swinbank 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
            swinbank 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
            swinbank John Swinbank added a comment - PR: https://github.com/lsst-dm/dm_dev_guide/pull/331
            Hide
            Parejkoj John Parejko added a comment -

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

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

            Thanks for the fast turnaround. Merged.

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

              People

              • Assignee:
                Unassigned
                Reporter:
                jsick Jonathan Sick
                Reviewers:
                John Parejko
                Watchers:
                John Parejko, John Swinbank, Jonathan Sick, Kian-Tat Lim, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel