Skip to content

DOC: Update the documentation concerning binary data.#111

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
hjmjohnson:UploadBinaryDataInstructions
Nov 6, 2018
Merged

DOC: Update the documentation concerning binary data.#111
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
hjmjohnson:UploadBinaryDataInstructions

Conversation

@hjmjohnson
Copy link
Member

No description provided.

Update the documentation concerning binary data used in ITK. Specifically:
- Add the `UploadTestingData.sh` shell script option to
  `UploadBinaryData.md`.
- Refer to `UploadBinaryData.md` in  `Data.md` for the content link
  file generation.
- Remove the `CMake Run` section from `Data.md`, since that side car is no
  longer necessary.

Change-Id: Iaaf77ae93552f6e92a0aad804b2853a61d6b3932
@hjmjohnson
Copy link
Member Author

http://review.source.kitware.com/#/c/23830/

Jon Hait... Gorroño
Uploaded patch set 1.Oct 25 10:44 PM

Jon Hait... Gorroño
Uploaded patch set 2.Oct 25 10:47 PM

Jon Haitz Legarreta Gorroño
Oct 25 11:01 PM

Patch Set 1:
(9 comments)
A few comments in-line.
Besides the questions about removing some parts, I feel that the documentation is still cumbersome at this point.
Feel free to suggest ways to improve it.
Thanks.
Documentation/Data.md
Line 11:
Not sure whether we want to add this; I do not recall having crossed such a file, or else, whether some classes would be better covered if data that can be fed through text files would be added.
Feel free to comment, and to indicate that I'd better remove it.
Line 64:
Although the CMake side car is not necessary, out of curiosity, does it still work?
Line 131:
Not sure whether this blind substitution holds.
Line 133:
Same comment here.
Line 170:
The Run CMake section is gone, so I guess this section does need to be refactored/removed. Let me know how I should do this.
Line 201:
Same comment as above.
Line 221:
Same comment as above.
Line 224:
Since we will soon abandon gerrit, this alias will not exist, and thus the section may be liable to be removed.
Line 248:
I guess this paragraphs will still holds at the kitware robots; may it go somewhere else if the gerrit-push section is removed?

Kitware Build Robot
Patch Set 1: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23830-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 25 11:02 PM

Jon Hait... Gorroño
Patch Set 2: Also, I feel that CMake's role is not clear throughout the files concerning the retrieval of the binary files from the content link files.Oct 25 11:28 PM

Kitware Build Robot
Patch Set 2: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23830-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 25 11:31 PM

Dzenan Zukic
Oct 26 9:45 AM

Patch Set 2: Code-Review+1
(5 comments)
Documentation/Data.md
Line 170:
While -> When
Line 187:
md5 reference still present
Line 190:
This is not a SHA512 hash. It will be confusing for people familiar with hashes. The example hashes should be updated.
Line 219:
md5
Documentation/UploadBinaryData.md
Line 98:
removed

Dzenan Zukic
Oct 26 9:47 AM

Patch Set 1:
(1 comment)
Documentation/Data.md
Line 224:
This will be renamed into review-push

Matt McCormick
Oct 26 4:15 PM

Patch Set 1:
(11 comments)
Awesome, Jon Haitz.
We a few edits, it should read well.
Documentation/Data.md
Line 11:
+1 for keeping. I really like it.
PS2, Line 10:
text -> text file
PS2, Line 68:
to -> with
PS2, Line 84:
lines 84 to 118 can now be removed.
PS2, Line 163:
Lines 164 to 250 can now be removed because we have removed the need for all that.
We could add a link to the following article, which provides additional details, e.g.
For more information, see CMake ExternalData: Using Large Files with Distributed Version Control.
Documentation/UploadBinaryData.md
PS2, Line 58:
UploadTestingData.sh -> UploadBinaryData.sh
PS2, Line 68:
hashum -> hashsum
PS2, Line 69:
it -> the file
PS2, Line 70:
public -> publically available
PS2, Line 91:
I added
5. Re-build ITK, and the testing data will be downloaded into the build tree.
to the script help in order to help folks understand the full workflow.
PS2, Line 95:
in -> from

Matt McCormick
Patch Set 1: Note -- I made my comments on patch set 1 (line number refer to that patch set). I think most of the comments in patch set 2 are addressed.Oct 26 4:15 PM

Jon Hait... Gorroño
Uploaded patch set 3.Oct 26 10:26 PM

Jon Haitz Legarreta Gorroño
Oct 26 10:27 PM

Patch Set 3:
(17 comments)
Thanks for the reviews folks.
Patch set 3 hopefully fixes these aspects.
Documentation/Data.md
PS1, Line 11:
OK, thanks Matt.
PS1, Line 224:
Done. I sought for it yesterday since I remembered Matt having done it for the ITKExamples, but was unable to find it. It was late, I was tired. So thanks Dzenan !
PS2, Line 10:
Done
PS2, Line 68:
Done
PS2, Line 84:
Done
PS2, Line 163:
Done
PS2, Line 170:
Thanks for the comment Dzenan; removing the paragraph to due to Matt's comment, though.
PS2, Line 187:
Same comment as above.
PS2, Line 190:
Same comment as above. I appreciate the review though !
PS2, Line 219:
Same comment as above.
Documentation/UploadBinaryData.md
PS2, Line 58:
Done
PS2, Line 68:
Done
PS2, Line 69:
Done
PS2, Line 70:
Done
PS2, Line 91:
Done
PS2, Line 95:
Done
PS2, Line 98:
Done

Kitware Build Robot
Patch Set 3: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23830-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 27 1:29 AM

Matt McCormick
Oct 27 7:01 AM

Patch Set 3:
(1 comment)
Awesome!
One minor update noted inline.
Documentation/UploadBinaryData.md
Line 100:
We can remove this sentence, since we remove the file in the script, now.

Dzenan Zukic
Patch Set 3: Code-Review+1Oct 27 9:30 AM

Jon Hait... Gorroño
Uploaded patch set 4.Oct 27 4:47 PM

Jon Haitz Legarreta Gorroño
Oct 27 4:48 PM

Patch Set 3:
(1 comment)
Documentation/UploadBinaryData.md
Line 100:
Removed it. Although from ITK's point of view it far better (I guess to avoid the risk of messing commits with the file etc.) to remove the binary file, it may be nice to warn the users about it, or let hem know how to get it back (which we removed from this file).

Kitware Build Robot
Patch Set 4: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23830-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Oct 27 5:07 PM

Dzenan Zukic
Patch Set 4: Code-Review+1Oct 29 8:07 AM

Matt McCormick
Patch Set 4: Code-Review+2

@hjmjohnson hjmjohnson merged commit 0882c6b into InsightSoftwareConsortium:master Nov 6, 2018
@thewtex
Copy link
Member

thewtex commented Nov 6, 2018

Please do not merge patches until after the mirroring has been switched.

@hjmjohnson
Copy link
Member Author

@thewtex ??? I don't know what that means, or how to know when the mirroring has been switched.

@thewtex
Copy link
Member

thewtex commented Nov 6, 2018

Do not merge or push or push to master on GitHub or via the stage / Gerrit.

There can only be one canonical Git repository, and other repositories mirror this repository. If merges occur across multiple repositories, it can result in an inconsistent Git history, inconsistent hashes, merge conflicts, ...

After the mirroring configuration has been changed, I will post in this thread:

https://discourse.itk.org/t/merging-temporarily-suspended/1396

@hjmjohnson hjmjohnson deleted the UploadBinaryDataInstructions branch November 6, 2018 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants