charlesgregory commented 3 years ago

Also added documentation on how I found the differences between htslib 1.12 and htslib 1.13.

codecov-commenter commented 3 years ago

Codecov Report

Merging #90 (f84409f) into develop (3f34c1b) will decrease coverage by 0.05%. The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop      #90      +/-   ##
===========================================
- Coverage    80.53%   80.47%   -0.06%     
===========================================
  Files           40       40              
  Lines         2805     2807       +2     
===========================================
  Hits          2259     2259              
- Misses         546      548       +2

Impacted Files	Coverage Δ
source/htslib/hts.d	`0.00% <0.00%> (ø)`
source/htslib/sam.d	`50.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3f34c1b...f84409f. Read the comment docs.

jblachly commented 3 years ago

@charlesgregory LGTM. Did you pull updates to existing headers in an automated way (e.g. dstep workflow) or simply eyeball the header diff and copy-paste?

charlesgregory commented 3 years ago

Eyeball copy-paste from diff. Process is described in the markdown file I added.

On Jul 28, 2021, at 10:42 PM, James S Blachly, MD @.***> wrote:

@charlesgregoryhttps://github.com/charlesgregory LGTM. Did you pull updates to existing headers in an automated way (e.g. dstep workflow) or simply eyeball the header diff and copy-paste?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/blachlylab/dhtslib/pull/90#issuecomment-888755325, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACH5L7LVTZGJF6ZFLXWMJO3T2C52XANCNFSM5BFNU3FA.

jblachly commented 3 years ago

Will this break the patches in dstep/patches ?

charlesgregory commented 3 years ago

Probably, I wasn't sure how those worked so they are probably already broke from the htslib 1.12 compatibility changes. Honestly we need an automated system (probably what you were going for with the dstep folder) for this but the bindings always have to have some manual changes (inline functions for instance).

On Jul 28, 2021, at 11:04 PM, James S Blachly, MD @.***> wrote:

Will this break the patches in dstep/patches ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/blachlylab/dhtslib/pull/90#issuecomment-888763272, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACH5L7NVL5CAPLRRMWJZNYDT2DAKRANCNFSM5BFNU3FA.

charlesgregory commented 3 years ago

Are we good to merge this in? If I am correct dhtslib is still compatible with 1.113 without these changes (minus the new functions).

jblachly commented 3 years ago

We can merge it if you pinky-swear to come up with an automated solution/fix the patches in the dstep folder. This will save us major grief when it comes to updating in the future so we don't accidentally miss any hand-tunes/inlines/rewrites we have added on that could be copied over if there's a large change in the source.

jblachly commented 3 years ago

for this but the bindings always have to have some manual changes (inline functions for instance).

@charlesgregory the patches in dstep/ are meant precisely to capture the difference between the automated conversion and our manual changes, so that we don't lose the manual changes in the future, and could potentially be applied on top of future automated changes

blachlylab / dhtslib

htslib 1.13 compatibility #90

Codecov Report