charlesgregory commented 3 years ago

There are far too many changes in this pull request to review so I will summarize the changes and we can discuss if this fits what we want to do moving forward.

This pull request does the following:

Updates the dstep Makefile to:
- create converted headers with dstep
- create patch files that will patch the converted headers to the current bindings
- create patch files that are easier to view by removing whitespace and comment style changes
  - These are NOT used to patch the dstep generated D files
- helper rules for cleaning and applying patches (copying patched files) to the dhtslib source
Updated the README with instruction on how to go about updating the bindings with a new version of htslib
Renames source/htslib/include-1.10 to source/htslib/include
Updates files under renamed source/htslib/include with current 1.13 headers
Updates the dstep/patches, dstep/output, and source/htslib/ folders with new patch files and bindings
- This makes sure they are up to date and valid for future htslib upgrades (htslib-1.14)
Adds patch files in the dstep/changes folder that details the major code changes (ignoring comment style and whitespace changes) with the current bindings and the generated bindings from dstep

Other:

dstep/include is a symlink that now points to source/htslib/include

With these changes the process needed to update bindings will be:

cd htslib
git pull
git checkout <version>
cd ..
cp htslib/htslib/* source/htslib/include
cd dstep
make clean
make

After this you should manually inspect the major changes patch files to see if anything new should be brought from new bindings. Once the bindings under source/htslib have been changed to update with new functions/structs, once again:

make clean
make 
make apply

Let me know what you think.

codecov-commenter commented 3 years ago

Codecov Report

Merging #121 (a65cf1f) into develop (0f284df) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #121   +/-   ##
========================================
  Coverage    85.57%   85.57%           
========================================
  Files           43       43           
  Lines         3826     3826           
========================================
  Hits          3274     3274           
  Misses         552      552

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 0f284df...a65cf1f. Read the comment docs.

jblachly commented 3 years ago

This seems overly complex, but I am likely misun derstanding some key parts. We should probably review in person/by video. Here are a few preliminary comments.

  * create converted headers with `dstep`

OK

  * create patch files that will patch the converted headers to the current bindings

Not sure what this means

  * create patch files that are easier to view by removing whitespace and comment style changes

This adds unnecessary complexity but I'd be happy to view some in a sample workflow to see if the payoff is worth it

  * helper rules for cleaning and applying patches (copying patched files) to the dhtslib source

Not sure I understand this either

* Renames `source/htslib/include-1.10` to `source/htslib/include`

Better yet, why don't we just get rid of it?

* Updates files under renamed `source/htslib/include` with current 1.13 headers

See above

* Adds patch files in the `dstep/changes` folder that details the major code changes (ignoring comment style and whitespace changes) with the current bindings and the generated bindings from `dstep`

This was my original (and sole) vision for the patch files (incidentally can you check to see whether github treats them differently if you call them .patch versus .diff ?)

* `dstep/include` is a symlink that now points to `source/htslib/include`

Since we have an htslib submodule, why not do one better and point to htslib/htslib, which would also incdentivize us to keep that submodule UTD

With these changes the process needed to update bindings will be: ... After this you should manually inspect the major changes patch files to see if anything new should be brought from new bindings. Once the bindings under source/htslib have been changed to update with new functions/structs, once again:

I am willing to give this a chance but I think it would be best if you walked me thru the 1.13 to 1.14 update on a pair coding session

charlesgregory commented 3 years ago

Not sure what this means

This adds unnecessary complexity but I'd be happy to view some in a sample workflow to see if the payoff is worth it

I probably should have provided some visual of how I understand/wrote the workflow

htslib source header ( htslib/htslib/*.h)
  |
  |  dstep
  v   
 converted header ( /tmp/*.d)       +        current bindings (source/htslib/*.d) 
                |                   |
                |                   | diff that ignores whitespace and comment changes
                |                   v
                |             patch/diff file for viewing major code changes
                |                    |
                |                    v
                |                (optional) manually modify current bindings (source/htslib/*.d) 
                |                to include new function bindings but retain 
                |                inline functions and others 
                |                                   |
                v                                   v
              converted header ( /tmp/*.d)   +  modified bindings (source/htslib/*.d)
                                             |
                                             v  diff
                     patch/diff file to update converted header to modified bindings
                                          |
                                          v
                    patch converted header with patch file and copy to source/htslib

This was my original (and sole) vision for the patch files (incidentally can you check to see whether github treats them differently if you call them .patch versus .diff ?)

I would prefer to use the patch files that contain only the major changes as the only patch files, however they end up being incompatible with the original files they are generated from. I tried only to have patch throw errors about conflicts. Though I may just be inexperienced with generating these patches.

As for the other question, It seems github recognizes .diff files as they get special highlighting in this pull request, also VScode does as well.

Better yet, why don't we just get rid of it?

Since we have an htslib submodule, why not do one better and point to htslib/htslib

That is fine, I didn't know if it needed to be kept and had considered just making it a symlink to the sub repo anyway.

I am willing to give this a chance but I think it would be best if you walked me thru the 1.13 to 1.14 update on a pair coding session

Agreed, currently this pull request just updates the patch files to be current (htslib 1.13). In hindsight, I probably should have just posted the changes to Makefile first and then later made the patches/changes to push up.

Another question I would have is to the manual modification of either a patch file or to the bindings. Is there an easier way than just modifying the bindings and recreating the patch file (rinse and repeat until satisfied) ? I would prefer to just modify the patch files but my understanding is that it is tricky.

charlesgregory commented 2 years ago

Closing this pull request in favor #129

blachlylab / dhtslib

Updates to dstep bindings generation #121

Codecov Report