ExitCodes refactoring so that their number fit in the range 0-255 #297

cmadjar · 2018-04-11T19:57:20Z

As @nicolasbrossard pointed out in PR #289, the exit codes are limited to the range 0-255. If a value outside that range is used as an exit code, it is truncated to 8 bits.

In order to refactor this, the follow google doc has been created to map the old and new exit codes:
https://docs.google.com/spreadsheets/d/1QsDQl9rXvut6Li62Rba7iUFRBT8mq6bDfX9JjYRsVQY/edit#gid=0

NOTE: since the exit codes were not yet part of any release, it is the perfect time to refactor them within the upcoming release of 19.1.

Redmine ticket: https://redmine.cbrain.mcgill.ca/issues/14247

…er fit between 0 and 255.

cmadjar · 2018-04-11T19:58:35Z

@MounaSafiHarab @nicolasbrossard I put you both to review this PR so it can make it to 19.1. Thank you!!

MounaSafiHarab · 2018-04-12T15:14:38Z

uploadNeuroDB/NeuroDB/ExitCodes.pm


-7. Exit codes from DTIPrep/DTIPrepRegister.pl (exit codes from 120 to 139)
+6. Common other generic failures (exit codes from 80 to 149)


Other common generic failures ;)

MounaSafiHarab · 2018-04-12T15:16:14Z

uploadNeuroDB/NeuroDB/ExitCodes.pm


-13. Exit codes from uploadNeuroDB/register_processed_data.pl (exit codes from
-240 to 259)
+11. Exit codes from uploadNeuroDB/minc_insertion.pl (exit codes from 180 to 189)


10 I think ; so 12 and 13 below are off by 1 as well

… order...

cmadjar · 2018-04-12T17:55:18Z

@MounaSafiHarab I resolved your comments. Ready for another go.

Once this is merged, I'll pull the changes to the other 19.1 PR so that the exit code exists with the proper number. Actually, I am going to add the need work before merging for the other one just to make sure.

nicolasbrossard · 2018-04-16T15:26:35Z

I checked if all the exit code constants that are used exist in ExitCodes.pm and they do. There are still uses of the exit function with hard-coded numeric values remaining though (DCMSUM.pm, Sawn.pm, etc..) Were they ignored on purpose?

cmadjar · 2018-04-16T16:16:34Z

I checked if all the exit code constants that are used exist in ExitCodes.pm and they do. There are still uses of the exit function with hard-coded numeric values remaining though (DCMSUM.pm, Sawn.pm, etc..) Were they ignored on purpose?

I just modified the exit codes for DCMSUM.pm, I somehow missed them... I also noticed I missed batch_upload_tarchive, which is fixed now. :)

Everything under the MNI folder have not been modified since they are libraries coming from the MINC tools I think. As for the other scripts in dicom-archive, they all are old tools that we should probably move to the tools directory under an archive folder as I highly doubt they are being used by anyone. Some more spring cleaning for later!

MounaSafiHarab · 2018-04-23T17:48:05Z

dicom-archive/DICOM/DCMSUM.pm

 	}
 	# do not allow to run diccomSummary with database option if the study has already been archived
 	elsif (!$Archivemd5 && $row[3] ne "") { 
 	    print "\n\n PROBLEM: This study has already been archived. You can only re-archive it using dicomTar!\n";
 	    print " This is the information retained from the first time the study has been archived:\n $row[1]\n\n";
-	    exit 33; }
+	    exit $NeuroDB::ExitCodes::FILE_NOT_UNIQUE;
+        ; }


extra set of ; here
also not sure why the error is File_Not_Unique; is that really what is happening here?

good catch! Correcting the duplicated ;

For the exit code, that is what I undestand. The study archive (or file) has already been inserted into the database so is not unique (like on the tarchiveloader side with MINC files). Does that make sense?

MounaSafiHarab · 2018-04-23T17:50:02Z

dicom-archive/DICOM/DCMSUM.pm

@@ -681,7 +684,7 @@ sub confirm_single_study {
 	foreach my $studyUID (keys(%hash)) {
 	    print "'$studyUID'\n";
 	}
-	exit 33; 
+	exit $NeuroDB::ExitCodes::FILE_TYPE_CHECK_FAILURE;


just nitpicking...
is this really a file_type failure? we happen to have files with 2 studyUIDs in the upload... so the file contents (headers) are wrong, but not the file type as it is DICOM...

I'll create a new exit code for NOT_A_SINGLE_STUDY.

MounaSafiHarab · 2018-04-23T17:52:46Z

uploadNeuroDB/NeuroDB/ExitCodes.pm

+our $MISSING_ARG          = 3; # if missing script's argument(s)
+our $DB_SETTINGS_FAILURE  = 4; # if DB settings in profile file are not set
+our $INVALID_PATH         = 5; # if path to file or folder does not exist
+our $INVALID_ARG          = 6; # if one of the program argument is invalid


arguments in plural

cmadjar

@MounaSafiHarab Thanks for the feedback! I think I resolved them. Let me know if you see anything else or if I missed something!

cmadjar · 2018-04-23T18:39:33Z

dicom-archive/DICOM/DCMSUM.pm

 	}
 	# do not allow to run diccomSummary with database option if the study has already been archived
 	elsif (!$Archivemd5 && $row[3] ne "") { 
 	    print "\n\n PROBLEM: This study has already been archived. You can only re-archive it using dicomTar!\n";
 	    print " This is the information retained from the first time the study has been archived:\n $row[1]\n\n";
-	    exit 33; }
+	    exit $NeuroDB::ExitCodes::FILE_NOT_UNIQUE;
+        ; }


good catch! Correcting the duplicated ;

For the exit code, that is what I undestand. The study archive (or file) has already been inserted into the database so is not unique (like on the tarchiveloader side with MINC files). Does that make sense?

cmadjar · 2018-04-23T18:41:33Z

dicom-archive/DICOM/DCMSUM.pm

@@ -681,7 +684,7 @@ sub confirm_single_study {
 	foreach my $studyUID (keys(%hash)) {
 	    print "'$studyUID'\n";
 	}
-	exit 33; 
+	exit $NeuroDB::ExitCodes::FILE_TYPE_CHECK_FAILURE;


I'll create a new exit code for NOT_A_SINGLE_STUDY.

cmadjar · 2018-04-23T18:42:02Z

uploadNeuroDB/NeuroDB/ExitCodes.pm

+our $MISSING_ARG          = 3; # if missing script's argument(s)
+our $DB_SETTINGS_FAILURE  = 4; # if DB settings in profile file are not set
+our $INVALID_PATH         = 5; # if path to file or folder does not exist
+our $INVALID_ARG          = 6; # if one of the program argument is invalid


nicolasbrossard · 2018-04-23T18:45:03Z

uploadNeuroDB/NeuroDB/ExitCodes.pm

                                     # -clobber was not set
+our $UNKNOWN_PROTOCOL          = 84; # if could not find acquisition protocol
+                                     # protocol of the file to be inserted


too too many many protocol protocol.... :)

nicolasbrossard · 2018-04-23T19:01:17Z

I checked and there are no more uses of exit with a numeric literal. 👍

MounaSafiHarab · 2018-04-23T19:10:45Z

it is good by me, waiting on @nicolasbrossard feedback

nicolasbrossard · 2018-04-23T19:12:24Z

All my comments were addressed. Approving.

MounaSafiHarab · 2018-04-23T19:53:36Z

me too, so am merging.

Aggregated ExitCodes so that they are more generalized and their numb…

c0f3f2f

…er fit between 0 and 255.

cmadjar added Bug [branch] minor Refactoring labels Apr 11, 2018

cmadjar added this to the 19.1 milestone Apr 11, 2018

cmadjar requested review from nicolasbrossard and MounaSafiHarab April 11, 2018 19:57

MounaSafiHarab reviewed Apr 12, 2018

View reviewed changes

Because apparently it is very hard to count and put words in a proper…

27f7fb8

… order...

cmadjar mentioned this pull request Apr 12, 2018

Modified dcmdump command to grep the patient information from the Config module #291

Merged

Missing exit codes in batch_upload_tarchive and DCMSUM library

443cd13

MounaSafiHarab reviewed Apr 23, 2018

View reviewed changes

Mouna's feedback

bb1b6fb

cmadjar commented Apr 23, 2018

View reviewed changes

nicolasbrossard requested changes Apr 23, 2018

View reviewed changes

Stuttering not welcome.

94312c7

nicolasbrossard approved these changes Apr 23, 2018

View reviewed changes

MounaSafiHarab merged commit 9072bd6 into aces:minor Apr 23, 2018

cmadjar deleted the RefactoringExitCodes branch May 31, 2018 15:19

cmadjar added this to the 20.0 (previously called 19.2) milestone Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExitCodes refactoring so that their number fit in the range 0-255 #297

ExitCodes refactoring so that their number fit in the range 0-255 #297

cmadjar commented Apr 11, 2018

cmadjar commented Apr 11, 2018

MounaSafiHarab Apr 12, 2018

MounaSafiHarab Apr 12, 2018 •

edited

Loading

cmadjar commented Apr 12, 2018

nicolasbrossard commented Apr 16, 2018

cmadjar commented Apr 16, 2018

MounaSafiHarab Apr 23, 2018

cmadjar Apr 23, 2018

MounaSafiHarab Apr 23, 2018

cmadjar Apr 23, 2018

MounaSafiHarab Apr 23, 2018

cmadjar Apr 23, 2018

cmadjar left a comment

cmadjar Apr 23, 2018

cmadjar Apr 23, 2018

cmadjar Apr 23, 2018

nicolasbrossard Apr 23, 2018

nicolasbrossard commented Apr 23, 2018

MounaSafiHarab commented Apr 23, 2018

nicolasbrossard commented Apr 23, 2018

MounaSafiHarab commented Apr 23, 2018


		7. Exit codes from DTIPrep/DTIPrepRegister.pl (exit codes from 120 to 139)
		6. Common other generic failures (exit codes from 80 to 149)

ExitCodes refactoring so that their number fit in the range 0-255 #297

ExitCodes refactoring so that their number fit in the range 0-255 #297

Conversation

cmadjar commented Apr 11, 2018

cmadjar commented Apr 11, 2018

Choose a reason for hiding this comment

MounaSafiHarab Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

cmadjar commented Apr 12, 2018

nicolasbrossard commented Apr 16, 2018

cmadjar commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasbrossard commented Apr 23, 2018

MounaSafiHarab commented Apr 23, 2018

nicolasbrossard commented Apr 23, 2018

MounaSafiHarab commented Apr 23, 2018

MounaSafiHarab Apr 12, 2018 •

edited

Loading