Add file type signatures to binary files by mr-c · Pull Request #1031 · dib-lab/khmer

mr-c · 2015-05-26T17:27:38Z

Closes #519

ctb · 2015-05-26T17:29:59Z

I'm OK with oxli and CountGraph being used internally, but it seems problematic to put them in diagnostic/error output, no?

Okay. We can change it back during the planned renames.

mr-c · 2015-05-26T17:37:12Z

Is it mergeable?
Did it pass the tests?
If it introduces new functionality in scripts/ is it tested?
Check for code coverage with make clean diff-cover
Is it well formatted? Look at make pep8, make diff_pylint_report,
make cppcheck, and make doc output. Use make format and manual
fixing as needed.
Did it change the command-line interface? Only additions are allowed
without a major version increment. Changing file formats also requires a
major version number increment.
Is it documented in the ChangeLog?
http://en.wikipedia.org/wiki/Changelog#Format
Was a spellchecker run on the source code and documentation after
changes were made?
Is the Copyright year up to date?

ctb · 2015-05-26T17:43:33Z

Could you add some discussion about implementation and implications moving forward? How does this break backwards compatibility, etc. etc.?

mr-c · 2015-05-26T17:48:22Z

Discussion in the docs or here?

Old counting table and presence tables will not be loadable with khmer 2.0. Likewise counting table and presence tables created with 2.0 will not load with khmer 1.4 and prior.

ctb · 2015-05-26T17:49:44Z

On Tue, May 26, 2015 at 10:48:22AM -0700, Michael R. Crusoe wrote:

Discussion in the docs or here?

Old counting table and presence tables will not be loadable with khmer 2.0. Likewise counting table and presence tables created with 2.0 will not load with khmer 1.4 and prior.

Here is fine. OK, anything else we should know? :)

mr-c · 2015-05-26T17:52:51Z

To add the header to an old file:

echo -n 'OXLICountGraph' > new_file.ct
cat old_counting_table.ct >> new_file.ct

# or 

echo -n 'OXLINodeGraph_' > new_file.pt
cat old_presence_table.ct >> new_file.pt

luizirber · 2015-05-26T18:06:53Z

Marginally related to this PR: What is the official procedure to add this signature to libmagic?

mr-c · 2015-05-26T18:19:20Z

@luizirber I just left a comment in the tracking issue #519 (comment)

mr-c · 2015-05-26T18:38:52Z

Hmm.. I just noticed that we do include the sub-file type as the second field. I'm going to switch to 'OXLI' as the signature. According to the file documentation we just need 32 unique bits, so that should suffice.

mr-c · 2015-05-26T19:44:02Z

tagset
stoptags
subset

mr-c · 2015-05-26T21:11:50Z

@ctb @luizirber ready for review & merge

kdm9 · 2015-05-27T00:22:07Z

Somewhere in the back of my mind I remember being told to include a non-printable character in the magic string for binary files, as it helps some things (like grep) detect a file is a binary one. I have no idea if it's relevant here, but I know the likes of BAM, gz etc do so. [citation needed]

ctb · 2015-05-27T10:35:25Z

On Tue, May 26, 2015 at 12:44:02PM -0700, Michael R. Crusoe wrote:

tagset

stoptags

subset

also, labels - see #1021.

mr-c · 2015-05-27T14:26:29Z

@kdmurray91 Good point, though our versioning & subtype bytes that follow the magic are not in the printable ASCII range so we are already fine on the point. For example, file already correctly identifies a counting table as being of type 'data' prior to the introduction of our rules.

mr-c · 2015-05-27T14:27:03Z

@ctb do you want me to add the signature to your PR or later to this PR for merging after #1021 ?

ctb · 2015-05-27T14:29:03Z

On Wed, May 27, 2015 at 07:27:04AM -0700, Michael R. Crusoe wrote:

@ctb do you want me to add the signature to your PR or later to this PR for merging after #1021 ?

later, I think.

mr-c · 2015-05-29T19:12:30Z

@ctb updated for the recently merged labels/tags format. Ready for review & merge

ctb · 2015-05-31T01:05:08Z

ctb · 2015-05-31T01:14:30Z

Other than the comments, looks good. Note that this PR passes the truncation tests I'm adding in #1048 too, so +1 that :)

mr-c · 2015-06-14T09:14:11Z

@ctb comments are addressed; ready for review & merge

ctb · 2015-06-14T12:30:01Z

Should this be merged? If so, please add to ChangeLog.

Thanks for catching that. Probably shouldn't be merged as not everyone will have git-merge-changelog installed. I've moved it to my personal site-wide configuration file.

On Sun, Jun 14, 2015 at 12:34:20PM -0700, Michael R. Crusoe wrote:

@@ -1 +1,2 @@
khmer/_version.py export-subst
+ChangeLog merge=merge-changelog

Thanks for catching that. Probably shouldn't be merged as not everyone will have git-merge-changelog installed. I've moved it to my personal site-wide configuration file.

(It doesn't seem to be a problem for merging, note; I ran a merge with it
set, and no problem.)

Good to know. Something to investigate for another day.

On Sun, Jun 14, 2015 at 12:36 PM C. Titus Brown notifications@github.com
wrote:

In .gitattributes
#1031 (comment):

@@ -1 +1,2 @@
khmer/_version.py export-subst
+ChangeLog merge=merge-changelog

On Sun, Jun 14, 2015 at 12:34:20PM -0700, Michael R. Crusoe wrote: > @@ -1
+1,2 @@ > khmer/_version.py export-subst > +ChangeLog merge=merge-changelog
Thanks for catching that. Probably shouldn't be merged as not everyone will
have git-merge-changelog installed. I've moved it to my personal site-wide
configuration file.
(It doesn't seem to be a problem for merging, note; I ran a merge with it
set, and no problem.)

—
Reply to this email directly or view it on GitHub
https://github.com/dib-lab/khmer/pull/1031/files#r32383396.

Michael R. Crusoe: Programmer & Bioinformatician crusoe@ucdavis.edu
mcrusoe@msu.edu
The lab for Data Intensive Biology; University of California, Davis
https://impactstory.org/MichaelRCrusoe http://twitter.com/biocrusoe

ctb · 2015-06-14T12:30:28Z

Other than the .gitattributes change, LGTM. Please go ahead and merge (after fixing .gitattributes thing one way or another)

with magic file

mr-c · 2015-06-14T20:09:36Z

Jenkins, test this please

Add file type signatures to binary files

mr-c · 2015-06-14T22:09:10Z

@ctb, thanks!

mr-c added this to the 2.0 milestone May 26, 2015

mr-c force-pushed the feature/magicnumbers branch from 9d34ece to b50d5bf Compare May 26, 2015 17:28

ctb reviewed May 26, 2015
View reviewed changes

mr-c force-pushed the feature/magicnumbers branch from 0290083 to b813793 Compare May 29, 2015 19:11

ctb reviewed May 31, 2015
View reviewed changes

Comment thread doc/whats-new-2.0.rst Outdated

ctb May 31, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have

ctb mentioned this pull request May 31, 2015

test various load functions #1048

Merged

mr-c mentioned this pull request Jun 9, 2015

Refactor normalize-by-median stage 1: broken_paired #1057

Merged

SensibleSalmon mentioned this pull request Jun 9, 2015

Update whatsnew for changes to normalize-by-median.py #1076

Closed

mr-c force-pushed the feature/magicnumbers branch from 20af055 to 04a33a4 Compare June 14, 2015 09:03

ctb reviewed Jun 14, 2015
View reviewed changes

add signature to beginning of binary file types

8718de6

with magic file

mr-c force-pushed the feature/magicnumbers branch from 2aa5787 to 8718de6 Compare June 14, 2015 19:33

mr-c added 2 commits June 14, 2015 12:35

remove redundant CL lines

21c7b20

bring CL up to date

8f6a358

mr-c added a commit that referenced this pull request Jun 14, 2015

Merge pull request #1031 from dib-lab/feature/magicnumbers

7f72b99

Add file type signatures to binary files

mr-c merged commit 7f72b99 into master Jun 14, 2015

mr-c deleted the feature/magicnumbers branch June 14, 2015 22:06

ctb mentioned this pull request Jul 19, 2015

create 'What's new in 2.0' document #1012

Closed

Conversation

mr-c commented May 26, 2015

Uh oh!

ctb May 26, 2015

Choose a reason for hiding this comment

Uh oh!

mr-c May 26, 2015

Choose a reason for hiding this comment

Uh oh!

mr-c commented May 26, 2015

Uh oh!

ctb commented May 26, 2015

Uh oh!

mr-c commented May 26, 2015

Uh oh!

ctb commented May 26, 2015

Uh oh!

mr-c commented May 26, 2015

Uh oh!

luizirber commented May 26, 2015

Uh oh!

mr-c commented May 26, 2015

Uh oh!

mr-c commented May 26, 2015

Uh oh!

mr-c commented May 26, 2015

Uh oh!

mr-c commented May 26, 2015

Uh oh!

kdm9 commented May 27, 2015

Uh oh!

ctb commented May 27, 2015

Uh oh!

mr-c commented May 27, 2015

Uh oh!

mr-c commented May 27, 2015

Uh oh!

ctb commented May 27, 2015

Uh oh!

mr-c commented May 29, 2015

Uh oh!

ctb May 31, 2015

Choose a reason for hiding this comment

Uh oh!

ctb commented May 31, 2015

Uh oh!

mr-c commented Jun 14, 2015

Uh oh!

ctb Jun 14, 2015

Choose a reason for hiding this comment

Uh oh!

mr-c Jun 14, 2015

Choose a reason for hiding this comment

Uh oh!

ctb Jun 14, 2015

Choose a reason for hiding this comment

Uh oh!

mr-c Jun 14, 2015

Choose a reason for hiding this comment

Uh oh!

ctb commented Jun 14, 2015

Uh oh!

mr-c commented Jun 14, 2015

Uh oh!

mr-c commented Jun 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants