You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2021/07/01 13:12:26 UTC
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r662214144
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -880,7 +880,7 @@ object Main extends Logging {
}
}
- def run(arguments: Array[String]): Int = {
+ def run(arguments: Array[String]): ExitCode.Value = {
Review comment:
Around line 300, there is a `sys.exit(1)` when the arguments to the CLI are bad. This should be changed to not return a 1. There aren't that many standard exit codes but `/usr/include/sysexits.h` defines EX_USAGE (e.g. incorrect command arguments) to be 64, so we should probably use that for this error.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,26 @@ object Util {
}
inputFile
}
+
+
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode: Int = expectedExitCode.id
+ //
Review comment:
I don't think you should need the `:Int` here. Also remove the empty comment.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
Review comment:
Mentioned above, but NotEnoughData should be removed. It's not an actual error.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1295,14 +1297,13 @@ object Main extends Logging {
if (unparseResult.isError) {
keepUnparsing = false
- error = true
+ exitCode = ExitCode.UnparseError
} else {
keepUnparsing = maybeScanner.isDefined && maybeScanner.get.hasNext
- error = false
+ exitCode = ExitCode.Normal
Review comment:
Remove this exit code, it's set above to Normal and only changed on error.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
+
+ val SchemaDefinitionError = Value(40)
+ val UnableToCreateProcessor = Value(41)
+ val UnableToGenerateCode = Value(42)
+
+ val PerformanceTestFailed = Value(50)
+ }
+
def main(arguments: Array[String]): Unit = {
+
val ret = try {
run(arguments)
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.FileNotFound
}
case e: InvalidParserException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.InvalidParserException
}
case e: ExternalVariableException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.ExternalVariableException
}
case e: BindingException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BindingException
}
case e: NotYetImplementedException => {
nyiFound(e)
+ ExitCode.NotYetImplemented
}
case e: TDMLException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.TDMLException
Review comment:
I suggest remove this error code we have this just exit with a new TestError mentioned in another comment.
And TestError should probably be grouped with Parser/Unparse/etc. errors.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
Review comment:
A 0 exit code is usually called "Success"
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
Review comment:
I would say ExternalVariableException and BindingException can be combined into a generic "BadExternalVariable" or something. Both of those exceptions occur when an error occurs when providing external variables.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1084,7 +1085,9 @@ object Main extends Logging {
}.map{ _.withExternalVariables(retrieveExternalVariables(performanceOpts.vars, cfgFileNode)) }
.map{ _.withValidationMode(validate) }
- val rc = processor match {
+ val rc: ExitCode.Value = processor match {
+ case Some(proc) if (proc.isError) => ExitCode.SchemaDefinitionError
+ case None => ExitCode.UnableToCreateProcessor
case Some(processor: DataProcessor) if (!processor.isError) => {
Review comment:
You can remove the `if (!processor.isError)` condition on this case. Since you've moved the the `proc.isError` case above.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
Review comment:
1 should be reservered for a generic "Failure" exit code. I'm not sure if we want to use it, since in most cases we probably do want a unique error code. Though, maybe we should only use unique error codes on things a user might want to check against, and everything else can be a generic "Failure".
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1243,8 +1245,8 @@ object Main extends Logging {
case Some(fileName) => new FileInputStream(fileName)
}
- val rc = processor match {
- case None => 1
+ val rc: ExitCode.Value = processor match {
+ case None => ExitCode.UnableToCreateProcessor
case Some(processor) => {
Review comment:
I think this is missing the `case Some(processor) if (processor.isError)` that the other commands have. But this makes me think we should change `createProcessorFromParser/Schema` to just always return `None` if there is an error, regardless where the error is.
If createProcessorFromParser returns `None`, it means we couldn't reload a a processor. I think it's impossible for that function to return Some(proc) where proc.isError == true.
If createProcessorFromSchema returns `None`, it means we got a ProcessorFactory with isError == true.
If createProcessorFromSchema returns `Some(proc)` and if proc.isError == true, then it means we got a DataProcessor with isError == true.
In all the above cases, I think we just want to say UnableToCreateProcessor. If a user needs specifics as to why, then can read the error messages.
So if we make the suggested change and createProcessorFrom* only return a Some if there is no error, then these become something like this:
```scala
case None => ExitCode.UnableToCreateProcessor
case Some(processor) =>
Assert.invariant(!processor.isError)
...
```
Which I think is a bit less confusing.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1051,15 +1054,13 @@ object Main extends Logging {
val leftOverDataWarning = s"Left over data. Consumed ${loc.bitPos0b} bit(s) with ${remainingBits} bit(s) remaining." + firstByteString + dataHex + dataText
log(LogLevel.Warning, leftOverDataWarning)
keepParsing = false
- error = true
+ exitCode = ExitCode.LeftOverData
Review comment:
I wonder if we should change LogLevel.Warning here to LogLevel.Error? If we are gonig to exit with a non-zero exit code, this related message should be an error.
This is a bit related to [DAFFODIL-2483](https://issues.apache.org/jira/browse/DAFFODIL-2483). This doesn't resolve that issue, which is about making this behavior configurable, but it does I think make this all more in line with what people expect from the CLI.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
+
+ val SchemaDefinitionError = Value(40)
+ val UnableToCreateProcessor = Value(41)
+ val UnableToGenerateCode = Value(42)
Review comment:
I'd call this `GenerateCodeError`. And group this with ParserError/UnparseError.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1007,13 +1010,13 @@ object Main extends Logging {
}
log(LogLevel.Warning, "Left over data after consuming 0 bits while streaming. Stopped after consuming %s bit(s) with %s bit(s) remaining.", loc.bitPos0b, remainingBits)
keepParsing = false
- error = true
+ exitCode = ExitCode.NoForwardProgress
} else {
// last parse did consume data, and we know there is more
// data to come, so try to parse again.
lastParseBitPosition = loc.bitPos0b
keepParsing = true
- error = false
+ exitCode = ExitCode.Normal
Review comment:
Same as the above comment here. Setting exitCode here isn't needed since we set it above to Normal and only change it in the case of an error. This line can be removed.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
+
+ val SchemaDefinitionError = Value(40)
+ val UnableToCreateProcessor = Value(41)
+ val UnableToGenerateCode = Value(42)
+
+ val PerformanceTestFailed = Value(50)
+ }
+
def main(arguments: Array[String]): Unit = {
+
val ret = try {
run(arguments)
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.FileNotFound
}
case e: InvalidParserException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.InvalidParserException
Review comment:
Instead of catching this here, we should probably be catching this exception in createProcessorFromParser, output the exception message, and just return None. Then this InvalidParserException exit code goes away and we'll just get the CreateProcessorError exit code.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1451,7 +1452,7 @@ object Main extends Logging {
println("")
println("Total: %d, Pass: %d, Fail: %d, Not Found: %s".format(pass + fail + notfound, pass, fail, notfound))
}
- 0
+ ExitCode.Normal
Review comment:
I'm wonering if we should return an error here if there are any failures? Maybe something like
```scala
if (fail == 0) ExitCode.Normal else ExitCode.TestError
```
That way people can determine if any tests failed by looking at the error code.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
+
+ val SchemaDefinitionError = Value(40)
+ val UnableToCreateProcessor = Value(41)
Review comment:
From a comment above, I suggest we drop SchemaDefinitionError, and any error while trying to create a data processor just results in a UnableToCreateProcessor error. This might mean failed to reload a processor, it might mean there was a SDE when compiling a processor. Based on the command the user ran, they can infer the problem.
From a naming perspective, I think CreateProcessorError probably better follows the standard.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
+
+ val SchemaDefinitionError = Value(40)
+ val UnableToCreateProcessor = Value(41)
+ val UnableToGenerateCode = Value(42)
+
+ val PerformanceTestFailed = Value(50)
Review comment:
Similarly, I'd call this PerformanceTestError and group with Parse/Unparse/GenerateCodeError
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -990,15 +993,15 @@ object Main extends Logging {
// not even 1 more bit is available.
// do not try to keep parsing, nothing left to parse
keepParsing = false
- error = false
+ exitCode = ExitCode.NotEnoughData
Review comment:
This block of code is hit when we've streamed up until the end of the data without any errors. Which means this is a successful parse. This should just be `exitCode = ExitCode.Normal/not`. In fact, technically we don't eve need to set exitCode here. We set it at the beginning of this loop to ExitCode.Normal and only change it if there's an error. Since this isn't an error, I suggest we just remove this line.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+
+
+
+ val Normal = Value(0)
+ val FileNotFound = Value(1)
+ val InvalidParserException = Value(2)
+ val ExternalVariableException = Value(3)
+ val BindingException = Value(4)
+ val NotYetImplemented = Value(5)
+ val TDMLException = Value(6)
+ val OutOfMemory = Value(7)
+ val UserDefinedFunctionFatalError = Value(8)
+ val BugFound = Value(9)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(30)
+ val NoForwardProgress = Value(21)
+ val LeftOverData = Value(22)
+ val NotEnoughData= Value(23)
+
+ val SchemaDefinitionError = Value(40)
+ val UnableToCreateProcessor = Value(41)
+ val UnableToGenerateCode = Value(42)
+
+ val PerformanceTestFailed = Value(50)
+ }
+
def main(arguments: Array[String]): Unit = {
+
val ret = try {
run(arguments)
} catch {
case s: scala.util.control.ControlThrowable => throw s
case e: java.io.FileNotFoundException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.FileNotFound
}
case e: InvalidParserException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.InvalidParserException
}
case e: ExternalVariableException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.ExternalVariableException
}
case e: BindingException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BindingException
}
case e: NotYetImplementedException => {
nyiFound(e)
+ ExitCode.NotYetImplemented
}
case e: TDMLException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.TDMLException
}
case e: OutOfMemoryError => {
oomError(e)
+ ExitCode.OutOfMemory
}
case e: UserDefinedFunctionFatalErrorException => {
log(LogLevel.Error, "%s", e.getMessage())
e.printStackTrace()
- 1
+ ExitCode.UserDefinedFunctionFatalError
Review comment:
Suggest renaming this to UserDefinedFunctionError. The "Fatal" part is a way for Daffodil to differentiate between different kinds of UDF errors. But by the time we get to the CLI, the only UDF error that a CLI user could ever see is the fatal error. So all UDF errors are implied to be fatal from a CLI perspective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org