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/09 10:40:22 UTC
[GitHub] [daffodil] dasmallwood opened a new pull request #601: Distinct return codes for CLI
dasmallwood opened a new pull request #601:
URL: https://github.com/apache/daffodil/pull/601
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r671474045
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,21 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+ val exitCodeCmd = if (Util.isWindows) "echo %errorlevel%" else "echo $?"
+ shell.sendLine(exitCodeCmd)
+
+ val sExitCode = shell.expect(contains("\n")).getBefore()
+ val actualInt = Integer.parseInt(sExitCode.trim())
Review comment:
It looks like some tests are failing because sExitCode isn't an integer, it's like an error message or the empty string or something. My guess is this is because Daffodil just hasn't exited by the time we call this function, and so executing `echo ...` to display the error code won't run until after that finishes. But we're going to immediately start looking for a newline, so if Daffodil is still running and outputs a newline (which is very common), we'll think the exit code is just whatever Daffodil happened to output.
I wonder if maybe we need to do something like this:
```scala
val keyWord = "EXITCODE:"
val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
shell.sendLine(exitCodeCmd)
shell.expect(contains(keyWord))
val sExitCode = shell.expect("\n").getBefore().trim()
val actualInt = Integer.parseInt(sExitCode)
...
```
So when we send the command to echo the exit code, we prepend a unique string. And then we then wait until we actually see this string. That way if Daffodil hasn't finished yet then we we'll wait until it is finished and the echo is actually run. Only once we see that do we then look for the newline and parse the int. This isn't tested, but something along these lines feels like the right approach.
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #601:
URL: https://github.com/apache/daffodil/pull/601#issuecomment-895409454
@dasmallwood, please rebase these commits onto the latest master and fix the conflicts. It seems GitHub will not run the checks until the conflicts are resolved.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r661807644
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,26 @@ object Util {
}
inputFile
}
+
Review comment:
excess blank lines
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r685937925
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -934,6 +958,8 @@ class TestCLIdebugger {
shell.sendLine("continue")
shell.expect(contains("<f xmlns=\"\">2</f>"))
shell.sendLine("quit")
+
Review comment:
I think the quit here needs to be removed. I think it was accidentally added back when conflicts were fixed.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675582140
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1570,79 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+ val Success = Value(0)
+ val Failure = Value(1)
+
+ val FileNotFound = Value(2)
+ val OutOfMemory = Value(3)
+ val BugFound = Value(4)
+ val NotYetImplemented = Value(5)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(21)
+ val GenerateCodeError = Value(23)
+ val TestError = Value(24)
+ val PerformanceTestError = Value(25)
+
+
+ val LeftOverData = Value(31)
+ val InvalidParserException = Value(32)
+ val BadExternalVariable = Value(33)
+ val UserDefinedFunctionError = Value(34)
+ val UnableToCreateProcessor = Value(35)
+
+ val Usage = Value(64)
+
+ }
+
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
- }
- case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.FileNotFound
}
case e: ExternalVariableException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BadExternalVariable
}
case e: BindingException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BadExternalVariable
}
case e: NotYetImplementedException => {
nyiFound(e)
+ ExitCode.NotYetImplemented
}
case e: TDMLException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.TestError
}
case e: OutOfMemoryError => {
oomError(e)
+ ExitCode.OutOfMemory
}
case e: UserDefinedFunctionFatalErrorException => {
log(LogLevel.Error, "%s", e.getMessage())
e.printStackTrace()
- 1
+ ExitCode.UserDefinedFunctionError
+ }
+ case e: DebuggerExitException => {
+ log(LogLevel.Error, "%s", e.getMessage())
+ ExitCode.Failure
Review comment:
I could see an argument for it being both. It is definitely a normal exit, but it also means we exited without generating a complete infoset. I could imagine someone using a script that triggers the CLI debugger, but still wants the script to be able to detect if the infoset was complete or not once the debugger finishes.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r685938103
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -974,6 +1000,8 @@ class TestCLIdebugger {
shell.sendLine("continue")
shell.expect(contains("<f xmlns=\"\">3</f>"))
shell.sendLine("quit")
+
Review comment:
Same here, I think this quit needs to be removed.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r688064816
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/unparsing/TestCLIUnparsing.scala
##########
@@ -395,7 +395,7 @@ class TestCLIunparsing {
val shell = Util.start("")
try {
- val cmd = String.format("echo '<tns:e1>Hello</tns:e1>' | %s unparse -s %s -r root", Util.binPath, testSchemaFile)
+ val cmd = String.format(Util.echoN("\'<tns:e1>Hello</tns:e1>\'") + "| %s unparse -s %s -r root", Util.binPath, testSchemaFile)
Review comment:
The single quotes need to be changed to double quotes. Single quotes don't mean anything special in windows like they do in bash.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r666861053
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/parsing/TestCLIParsing.scala
##########
@@ -1196,4 +1198,23 @@ class TestCLIparsing {
}
}
+ @Test def test_CLI_Error_Return_Codes(): Unit = {
+
+ val shell = Util.startIncludeErrors("")
+
+ val schemaFile = Util.daffodilPath("daffodil-test/src/test/resources/org/apache/daffodil/section00/general/generalSchema.dfdl.xsd")
+ val (testSchemaFile) = if (Util.isWindows) (Util.cmdConvert(schemaFile)) else (schemaFile)
+
+ try {
+ val cmd = String.format("%s parse -I scala-xml -s %s -r e1", Util.binPath, "/this/does/not/exist")
+
+ shell.sendLine(cmd)
+ Util.expectExitCode(ExitCode.FileNotFound, shell)
+
+ } finally {
+ shell.close()
+ }
+
+ }
Review comment:
I wonder if we should add this `expectExitCode` to all CLI integration tests? I imagine most of the different error cases are already covered by tests (including this file not found that this tests is checking). That would also make it easy to determine which exit codes aren't testing for, and we add new tests for those?
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1049,17 +1042,15 @@ object Main extends Logging {
"at least " + (bytesAvailable * 8)
}
val leftOverDataWarning = s"Left over data. Consumed ${loc.bitPos0b} bit(s) with ${remainingBits} bit(s) remaining." + firstByteString + dataHex + dataText
Review comment:
This is no longer a warning, so this variable name isn't quite right. Maybe we should rename it `leftOverDataMessage`?
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1243,8 +1233,9 @@ object Main extends Logging {
case Some(fileName) => new FileInputStream(fileName)
}
- val rc = processor match {
- case None => 1
+ val rc: ExitCode.Value = processor match {
Review comment:
I don't think you need the ExitCode.Value type here
##########
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 don't think you addressed this comment.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1567,74 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+ val Success = Value(0)
+ val Failure = Value(1)
+
+ val FileNotFound = Value(2)
+ val OutOfMemory = Value(3)
+ val BugFound = Value(4)
+ val NotYetImplemented = Value(5)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(21)
+ val GenerateCodeError = Value(23)
+ val TestError = Value(24)
+ val PerformanceTestError = Value(25)
+
+
+ val NoForwardProgress = Value(30)
+ val LeftOverData = Value(31)
+ val InvalidParserException = Value(32)
Review comment:
This error code is no longer used, we should remove these, especially since there isn't a significant difference in mening from UnableToCreateProcessor.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -301,7 +285,7 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
}
log(LogLevel.Error, "%s", msg)
- sys.exit(1)
+ sys.exit(64)
Review comment:
Please add a enum for this value, something like `ExitCode.Usage` probably makes sense to match the name in `sysexits.h`
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -990,30 +985,28 @@ 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
} else {
// There is more data available.
if (parseOpts.stream.toOption.get) {
// Streaming mode
if (lastParseBitPosition == loc.bitPos0b) {
// this parse consumed no data, that means this would get
// stuck in an infinite loop if we kept trying to stream,
- // so we need quit
+ // so we need to quit
val remainingBits =
if (loc.bitLimit0b.isDefined) {
(loc.bitLimit0b.get - loc.bitPos0b).toString
} else {
"at least " + (inStream.inputSource.bytesAvailable * 8)
}
- 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)
+ log(LogLevel.Error, "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
Review comment:
I wonder if this should just be `ExitCode.LeftOverData` and we can remove `NoForwardProgress`.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1720,7 +1722,7 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
|Immediately abort all processing.""".stripMargin
override lazy val short = "q"
def act(args: Seq[String], state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- sys.exit(1)
+ throw new DebuggerExitException("")
Review comment:
If the string is always going to be the empty string, I suggest just removing the `str: String` parameter.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r661817703
##########
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)
Review comment:
They should be in numeric order, so move this one below.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r669748218
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -63,7 +64,8 @@ class TestCLIdebugger {
shell.expect(contains("error: expression evaluation failed: Schema Definition Error:"))
shell.expect(contains("(debug)"))
- shell.sendLine("quit")
+ Util.expectExitCode(ExitCode.Failure, shell)
+ shell.send("quit")
Review comment:
It looks like you've changed all instances of `shell.sendLine("quit")` to `shell.send("quit")`. The difference between send and sendLine is that send will not include a line separator at the end, which is needed to actually execute the quit command in the CLI debugger. It's like typing the word "quit" without pressing enter. So in this file, these need to be sendLines.
Also in the TestCLIDebugger file, the "quit" here is to quit the CLI interactive debugger, not the shell. So the exit code check needs to happen after the "quit" command so we check that the CLI debugger exits with the right exit code.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/generating/TestCLIGenerateC.scala
##########
@@ -78,6 +83,8 @@ class TestCLIGenerateC {
try {
shell.sendLine(generateCmd)
shell.expect(contains("Unknown option 's'"))
+
+ Util.expectExitCode(ExitCode.GenerateCodeError, shell)
Review comment:
I think this and many other tests in this file should be an ExitCode.Usage error.
And looking at the CI, many of these tests are failing because they are looking for the wrong exit code.
Make sure you are running the tests to make sure they work before pushing them for review. To run these CLi integration tests, you need to run them with
```
sbt IntegrationTest/test
```
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1233,10 +1236,11 @@ object Main extends Logging {
case Some(fileName) => new FileInputStream(fileName)
}
- val rc: ExitCode.Value = processor match {
+ val rc = processor match {
case Some(proc) if (proc.isError) => ExitCode.UnableToCreateProcessor
case None => ExitCode.UnableToCreateProcessor
case Some(processor) => {
+ Assert.invariant(processor.isError == false)
Review comment:
Make sure to remove the `case Some(proc) if (proc.isError)` line, that line shouldn't be possible if you've made the above change correctly. Also, note that a similar line occurs in multiple places in the file. Make sure to remove all places where we proc.isError is true. The only place that should be possible is in the createProcessorFrom* functions.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/executing/TestCLIexecuting.scala
##########
@@ -130,6 +139,8 @@ class TestCLIexecuting {
try {
shell.sendLine(String.format("%s test -l %s", Util.binPath, testTdmlFile))
shell.expect(contains(output16))
+
+ Util.expectExitCode(ExitCode.TestError, shell)
Review comment:
Some of these exit codes seem incorrect to me. For example, this test is just listing the tests in a tdml file. That should succeed, so I would expect this to have an exit code of ExitCode.Success. Feels like this test is checking for the wrong exit code and it's going to fail, or the CLI is returning a TestError when it should be returning a Success.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -695,10 +695,13 @@ object Main extends Logging {
// of compilation, where it asks for the parser)
//
val schemaSource = URISchemaSource(schema)
- val pf = Timer.getResult("compiling", {
+ val res = Timer.getResult("compiling", {
val processorFactory = compiler.compileSource(schemaSource)
if (!processorFactory.isError) {
val processor = processorFactory.onPath(path.getOrElse("/")).withValidationMode(mode)
+ if (processor.isError) {
+ None
+ }
Review comment:
This doesn't actually return None from the function. It just means the result of this if statement is None, but that isn't used and so is essentially a no-op. This instead wants to be something like this:
```scala
val processor = ...
displayDiagnostics(processor)
if (processor.isError) {
None
} else {
Some(processor)
}
```
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/executing/TestCLIexecuting.scala
##########
@@ -162,6 +175,8 @@ class TestCLIexecuting {
val cmd = String.format("%s test %s byte_entities_6_08", Util.binPath, testTdmlFile)
shell.sendLine(cmd)
shell.expect(contains("[Pass] byte_entities_6_08"))
+
+ Util.expectExitCode(ExitCode.TestError, shell)
Review comment:
Here's another example. We are running a TDML test, it's passing, yet we are checking for TestError. I would expect this to be a Success.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675706654
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,25 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+
+ val keyWord = "EXITCODE:"
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+ shell.sendLine(exitCodeCmd)
+ shell.expect(contains(keyWord))
Review comment:
Is there a race condition between what we're telling it to echo back, and stuff it was going to output anyway?
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r661807113
##########
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 {
+
Review comment:
close up some excess whitespace lines in this.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r688012700
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/unparsing/TestCLIUnparsing.scala
##########
@@ -259,13 +259,13 @@ class TestCLIunparsing {
try {
- shell.sendLine(String.format("""echo "<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>" | %s -v unparse -s %s --root e1""", Util.binPath, testSchemaFile))
+ shell.sendLine(String.format(Util.echoN("\"<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>\"") + "| %s -v unparse -s %s --root e1", Util.binPath, testSchemaFile))
shell.expectIn(1, contains("[info]"))
- shell.sendLine(String.format("""echo "<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>" | %s -vv unparse -s %s --root e1""", Util.binPath, testSchemaFile))
+ shell.sendLine(String.format(Util.echoN("\"<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>\"") + "| %s -vv unparse -s %s --root e1", Util.binPath, testSchemaFile))
shell.expectIn(1, contains("[compile]"))
- shell.sendLine(String.format("""echo "<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>" | %s -vvv unparse -s %s --root e1""", Util.binPath, testSchemaFile))
+ shell.sendLine(String.format(Util.echoN("\"<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>\"") + "| %s -vvv unparse -s %s --root e1", Util.binPath, testSchemaFile))
Review comment:
Looks like you've only updated this single test `test_3580_CLI_Unparsing_SimpleUnparse_verboseMode`. This test is passing in the github actions on windows, so it does seem like echoN is the right fix.
But there are other tests that are failing that need the same change:
* test_3528_CLI_Unparsing_SimpleUnparse_stdin2
* test_3529_CLI_Unparsing_SimpleUnparse_stdin3
* test_3578_CLI_Unparsing_SimpleUnparse_defaultRoot
* test_3583_CLI_Unparsing_SimpleUnparse_rootPath
Also, these two tests use echo but happen to work because they are negative tests that fail for reasons other than echo issues. But for correctness these should use echoN too:
* test_3662_CLI_Unparsing_badSchemaPath
* test_3579_CLI_Unparsing_negativeTest
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r676820742
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1570,79 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+ val Success = Value(0)
+ val Failure = Value(1)
+
+ val FileNotFound = Value(2)
+ val OutOfMemory = Value(3)
+ val BugFound = Value(4)
+ val NotYetImplemented = Value(5)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(21)
+ val GenerateCodeError = Value(23)
+ val TestError = Value(24)
+ val PerformanceTestError = Value(25)
+
+
+ val LeftOverData = Value(31)
+ val InvalidParserException = Value(32)
+ val BadExternalVariable = Value(33)
+ val UserDefinedFunctionError = Value(34)
+ val UnableToCreateProcessor = Value(35)
+
+ val Usage = Value(64)
+
+ }
+
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
- }
- case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.FileNotFound
}
case e: ExternalVariableException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BadExternalVariable
}
case e: BindingException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BadExternalVariable
}
case e: NotYetImplementedException => {
nyiFound(e)
+ ExitCode.NotYetImplemented
}
case e: TDMLException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.TestError
}
case e: OutOfMemoryError => {
oomError(e)
+ ExitCode.OutOfMemory
}
case e: UserDefinedFunctionFatalErrorException => {
log(LogLevel.Error, "%s", e.getMessage())
e.printStackTrace()
- 1
+ ExitCode.UserDefinedFunctionError
+ }
+ case e: DebuggerExitException => {
+ log(LogLevel.Error, "%s", e.getMessage())
+ ExitCode.Failure
Review comment:
Also, we should remove this `log()` call. Although this is an exception, it's an expected exception when the user quits the debugger. When they do this, they'll know that things are going to exit, so they don't need a message confirming things.
--
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
[GitHub] [daffodil] stevedlawrence merged pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #601:
URL: https://github.com/apache/daffodil/pull/601
--
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
[GitHub] [daffodil] dasmallwood commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
dasmallwood commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675697850
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,25 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+
+ val keyWord = "EXITCODE:"
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+ shell.sendLine(exitCodeCmd)
+ shell.expect(contains(keyWord))
Review comment:
from running the tests manually i've seen the Expect operation fails due to timeout
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r677838364
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -173,9 +173,13 @@ object Util {
val expectedCode = expectedExitCode.id
val keyWord = "EXITCODE:"
- val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+// val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "^|%errorlevel%" else "\\!$?")
Review comment:
Please remove the commented code. Also, I think it's worth adding a comment explaining why we have this `^|` and `\\!` stuff. The idea being that the echo command needs to look different from what the echo outputs or the expect command might match the actual command. By echo things that are escaped (an escaped pipe in windows and an escaped exclamation in linux), then the echo command and the echo output include different text, so expect can tell the difference.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r685951204
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/unparsing/TestCLIUnparsing.scala
##########
@@ -244,19 +255,20 @@ class TestCLIunparsing {
val schemaFile = Util.daffodilPath("daffodil-test/src/test/resources/org/apache/daffodil/section00/general/generalSchema.dfdl.xsd")
val (testSchemaFile) = if (Util.isWindows) (Util.cmdConvert(schemaFile)) else (schemaFile)
- val shell = Util.start("", true)
+ val shell = Util.start("")
try {
- shell.sendLine(String.format("""echo '<tns:e1 xmlns:tns="http://example.com">Hello</tns:e1>' | %s -v unparse -s %s --root e1""", Util.binPath, testSchemaFile))
- shell.expect(contains("[info]"))
+ shell.sendLine(String.format("""echo "<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>" | %s -v unparse -s %s --root e1""", Util.binPath, testSchemaFile))
Review comment:
Hmm, looks like using the double quotes still doesn't work. I guess echo on windows will still echo double quotes? I think maybe using our `Util.echoN` function will fix this, since that echos out data a little differently and I think will strip off the double quotes? So maybe something like this:
```scala
shell.sendLine(String.format(Util.echoN("\"<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>\"") + "| %s -v unparse ....", ...))
```
Note that we still need the double quote in whatever is echo'ed to escape the angle brackets, so we need to include an escaped quote on either end of the string passed into `Util.echoN`.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r663179755
##########
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:
I would not change PropGen. The debugger can have a defined ExitCode or use generic ExitCode.Failure
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r663064075
##########
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:
Yes, ExitCode.Success is more customary.
##########
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)
Review comment:
Steve suggested renaming this to UserDefinedFunctionError, agreeing.
##########
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)
Review comment:
Steve suggested renaming this to TestError and regrouping, agreeing.
##########
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:
Agreed, let's have a catch-all generic ExitCode.Failure (set to 1) in case we want to reduce the number of exit codes later. Since we're regrouping some errors together, I suggest we start each group of following exit codes from its own new base. That will give us room to add errors to each group in the future without renumbering following groups of errors. Feel free to adjust the base of each group to whatever makes sense (I like 8, 16, 32, 64, ... but 10, 20, 30, ... or other bases could work too).
##########
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 fine with returning an ExitCode.TestError if any tests failed to pass.
##########
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:
That reminded me to grep the entire codebase for exit calls. We should make sure this PR catches and changes all exit calls. I found some more calls here:
```rg
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
120: System.exit(1)
304: sys.exit(1)
1623: System.exit(ret)
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/PropertyGenerator.scala
703: System.exit(1);
daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
1723: sys.exit(1)
```
##########
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'm fine with changing the log level from warning to error.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r685936375
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -203,4 +171,30 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+
+ val keyWord = "EXITCODE:"
+
+ //Escaped characters ^| for windows and \\! for linux makes the echo outputs different text than the command,
+ //That way the expect function can tell the difference.
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "^|%errorlevel%" else "\\!$?")
+ val exitCodeExpectation = keyWord + (if (Util.isWindows) "|" else "!")
+
+ shell.sendLine(exitCodeCmd)
+ shell.expect(contains(exitCodeExpectation))
+
+ val sExitCode = shell.expect(contains("\n")).getBefore().trim()
+ val actualInt = Integer.parseInt(sExitCode)
+
+ if (actualInt != expectedCode) {
+ val expectedExitCodeName = expectedExitCode.toString
+ val actualExitCodeName = ExitCode(actualInt)
Review comment:
If the `actualInt` does not match the expected exit code, then that likely means there's some bug, and it's possible that `actualInt` doesn't even map to an ExitCode. This line would then will result in a NoSuchElement exception.
We could just not output an actual exit code name, but it probably is useful information to have when it's available. So instead we could do something like this to get an ExitCode name if it exists, otherwise just display "Unknown:
```scala
val actualExitCodeName = ExitCode.values.find { _.id == actualInt }.map { _.toString }.getOrElse("Unknown")
```
--
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
[GitHub] [daffodil] dasmallwood closed pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
dasmallwood closed pull request #601:
URL: https://github.com/apache/daffodil/pull/601
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #601:
URL: https://github.com/apache/daffodil/pull/601#issuecomment-898396650
+1
This is good to squash and be merged. Unfortunately, because there's a merge commit (which should be avoided in the future), you can't just use `rebase -i` that's described in the code contributor workflow to squash everything. Instead you need to do this:
```bash
# make sure you are on the branch to squash
git checkout daf-2235-exitcode
# create a patch that is the diff between your current branch and
# the most recent common ancestor with asf/master
git diff $(git merge-base HEAD asf/master) > patch.diff
# throw away all changes on your current branch by resetting to that
# common ancestor
git reset --hard $(git merge-base HEAD asf/master)
# apply the patch with changes staged
git apply --index patch.diff
# commit the already staged changes, making sure to follow the standards
# for a good commit message mentioned in the code contributor workflow
git commit
```
If things don't look right at any point above, or a command failed, you can always reset back to where we started by running running:
```bash
git reset --hard origin/daf-2235-exitcode
```
But otherwise, if everything looks good, you can force push the squashed commit to update this PR and delete the patch file:
```bash
# push your changes to update this PR
git push --force origin daf-2235-exitcode
# delete the patch
rm patch.diff
```
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #601:
URL: https://github.com/apache/daffodil/pull/601#issuecomment-887884786
Getting so close. Only a handful of windows tests are failing now. If we look at the output of a failing test, we see something like this:
```
echo '<tns:e1 xmlns:tns="http://example.com">Hello</tns:e1>' | .\daffodil-cli\target\universal\stage\bin\daffodil.bat unparse -s .\daffodil-test\src\test\resources\org\apache\daffodil\section00\general\generalSchema.dfdl.xsd --root e1 -
D:\a\daffodil\daffodil>echo '<tns:e1 xmlns:tns="http://example.com">Hello</tns:e1>' | .\daffodil-cli\target\universal\stage\bin\daffodil.bat unparse -s .\daffodil-test\src\test\resources\org\apache\daffodil\section00\general\generalSchema.dfdl.xsd --root e1 -
echo EXITCODE:^|%errorlevel%
The system cannot find the file specified.
Error: Unparse Error: Illegal content where start or end event expected: Error: Unexpected EOF in prolog
at [row,col {unknown-source}]: [1,0] on line 1
Schema context: element reference tns:e1 Location line 19 in file:/D:/a/daffodil/daffodil/./daffodil-test/src/test/resources/org/apache/daffodil/section00/general/generalSchema.dfdl.xsd
D:\a\daffodil\daffodil>echo EXITCODE:^|%errorlevel%
EXITCODE:|21
```
I *think* this is the output for test_3529_CLI_Unparsing_SimpleUnparse_stdin3. This test runs the unparse command and then expects the output "Hello". I think this test passed previously for a similar reason that we just had to fix the expect exit code stuff: We are running a command, windows is outputting that command, and then we are matching the output. So when we `expect` "Hello", it's actually finding the `Hello` in the command, and not in the unparsed data. So these tests are just plain broken and we didn't know it.
So there are two issues:
1. Why are these tests failing on windows
2. Is there a way to ensure we aren't matching the command, and are instead matching the actual unparsed data.
I'm guessing that the answer to 1 is because these tests are using single quotes to escape the XML infoset. But single quotes don't do anything on windows--only double quotes escape things. So since the infoset isn't being escaped properly, windows see's the angle brackets as file redirection (hence "The system cannot find the file specified." error). We use single quotes in these tests because the infoset already includes double quotes in the XML. But I think an easy solution here is to just swap single quotes and double quotes for these tests that fail, so the command would look like this:
```scala
val cmd = String.format("""echo "<tns:e1 xmlns:tns='http://example.com'>Hello</tns:e1>" | %s ... """)
```
So the double quotes escape things correctly, and we use single quotes inside the XML, which is still valid XML.
For issue 2, I think we probably need to make it so what we unparse is different from the content in the infoset. So if we try to unaprse an infoset containing the string "Hello" the result can't also be "Hello" or expect won't know the difference. One way to make sure there is a diffrence is to add an initiator to the schea element. So for example, if our schema looks like this:
```xml
<element name="e1" dfdl:lengthKind="explicit" dfdl:length="5" type="xs:string" />
```
We can change it to this instead:
```xml
<element name="e1" dfdl:initiator="init:" dfdl:lengthKind="explicit" dfdl:length="5" type="xs:string" />
```
This way if our infoset looks like this:
```xml
<tns:e1 xmlns:tns="http://example.com">Hello</tns:e1>
```
It will unparse to `init:Hello` rather than just "Hello". And then we can `expect` the full `init:Hello` and be confident that we're expecting the unparsed data rather than the infoset data. This does mean that test inputs and outputs need to change. Parsed data needs to look like "init:foo" and unparsed data needs to expect "init:foo"--the XML infoset stays the same though.
An alternative might just to find a way to prevent windows from printing the command to stdout, but I'm not familiar enough with windows to know if that's possible.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r671438139
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##########
@@ -64,8 +64,8 @@ class TestCLIdebugger {
shell.expect(contains("error: expression evaluation failed: Schema Definition Error:"))
shell.expect(contains("(debug)"))
- Util.expectExitCode(ExitCode.Failure, shell)
- shell.send("quit")
+ shell.sendLine("quit")
+ Util.expectExitCode(ExitCode.BugFound, shell)
Review comment:
No tests should ever return a BugFound exit. If that's the case, we have a bug that needs to be fixed.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/performance/TestCLIPerformance.scala
##########
@@ -341,7 +341,7 @@ class TestCLIPerformance {
if (exitCode == 0)
fail("Tests were successful when they were expected to fail. Exit code: " + exitCode)
- Util.expectExitCode(ExitCode.PerformanceTestError, shell)
+ Util.expectExitCode(ExitCode.Success, shell)
Review comment:
Same as above, looks like this tests should exit with a non-zero exit code.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/performance/TestCLIPerformance.scala
##########
@@ -180,7 +180,7 @@ class TestCLIPerformance {
if (exitCode == 0)
fail("Tests were successful when they were expected to fail. Exit code: " + exitCode)
- Util.expectExitCode(ExitCode.PerformanceTestError, shell)
+ Util.expectExitCode(ExitCode.Success, shell)
Review comment:
It looks like this tests is expecting things to fail, and so shouldn't be ExitCode.Success. If this test is passing and exiting with Zero I think there is a bug somewhere.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/performance/TestCLIPerformance.scala
##########
@@ -50,7 +50,7 @@ class TestCLIPerformance {
if (exitCode != 0)
fail("Tests failed. Exit code: " + exitCode)
- Util.expectExitCode(ExitCode.PerformanceTestError, shell)
+ Util.expectExitCode(ExitCode.Success, shell)
Review comment:
Looks like this test is already checking the exit code. I'd suggest you delete the previous stuff and only use the new expectExitCode stuff. Other tests do something similar-we should replace that with expectExitCode in all the integration tests that do that.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1334,9 +1326,9 @@ object Main extends Logging {
val rc = processor match {
case Some(processor) => {
Timer.getResult("saving", processor.save(output))
Review comment:
Should add an assert here to match the above cases.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -916,7 +912,10 @@ object Main extends Logging {
}.map{ _.withExternalVariables(retrieveExternalVariables(parseOpts.vars, cfgFileNode))}
val rc = processor match {
- case Some(proc) if (!proc.isError) => {
+ case Some(proc) if (proc.isError) => ExitCode.UnableToCreateProcessor
Review comment:
This case is impossible and can be removed. With the above change, we will never get a Some with isError == true.
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1084,8 +1078,10 @@ object Main extends Logging {
}.map{ _.withExternalVariables(retrieveExternalVariables(performanceOpts.vars, cfgFileNode)) }
.map{ _.withValidationMode(validate) }
- val rc = processor match {
- case Some(processor: DataProcessor) if (!processor.isError) => {
+ val rc: ExitCode.Value = processor match {
+ case Some(proc) if (proc.isError) => ExitCode.UnableToCreateProcessor
Review comment:
Same as above, this case can be removed. We should an the case Some below to match the above code.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/unparsing/TestCLIUnparsing.scala
##########
@@ -284,7 +284,7 @@ class TestCLIunparsing {
shell.sendLine(cmd)
shell.expect(contains("There should be exactly one of the following options: schema, parser"))
- Util.expectExitCode(ExitCode.UnparseError, shell)
+ Util.expectExitCode(ExitCode.Success, shell)
Review comment:
Seems like this should be a usage error.
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/unparsing/TestCLIUnparsing.scala
##########
@@ -400,7 +400,7 @@ class TestCLIunparsing {
shell.expect(contains("Bad arguments for option 'schema'"))
shell.expect(contains("Could not find file or resource"))
- Util.expectExitCode(ExitCode.UnparseError, shell)
+ Util.expectExitCode(ExitCode.Success, shell)
Review comment:
Should be a UsageError
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1720,7 +1722,7 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
|Immediately abort all processing.""".stripMargin
override lazy val short = "q"
def act(args: Seq[String], state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- sys.exit(1)
+ throw new DebuggerExitException()
Review comment:
Nothing catches this exception. This should be caught in Main.scala so that we can exit with an appropriate exit code. A generic `ExitCode.Failure` seems reasonable.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r663080713
##########
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:
PropertyGenerator probably isn't a big deal. That won't have access to the Main.scala exit code unless we want to move the exit codes to lib. But many of these codes are very specific to the CLI, so I'm not sure that makes sense. The property generator also isn't ever run by users, it's just generates code as part of the buil.
The interactive debugger is also an interesting case. Although the interactive debugger is trigger from the CLI, it doesn't necessarily have to be, so a sys.exit() might even be wrong here. I wonder if instead it should throw some kind of debugger or interrupt exception that the CLI can catch and exit with an appropriate error code. And no CLI uses could handle it differently?
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #601:
URL: https://github.com/apache/daffodil/pull/601#issuecomment-887477300
Interesting, it looks like things are still failing on windows. And the failures are all with the error:
```scala
java.lang.NumberFormatException: For input string: "%errorlevel%"
```
So it seems our new expectExitCode capture our echo line, and not the results. Looking at the output for the integration tests, we see things like this:
```
<?xml version="1.0" encoding="UTF-8"?>
<never-fails>2f6481e6-542c-11eb-ae93-0242ac130002</never-fails>
echo EXITCODE:%errorlevel%
D:\a\daffodil\daffodil>echo EXITCODE:%errorlevel%
EXITCODE:0
```
So we can see some output happened (the never-fails stuff), and before we got a prompt we call sendLine with `echo EXITCODE:%errorlevel%`, which we can see (that's output to the console). Then we will call `expect("EXITCODE")`. But even though echo command has already been output, windows will output it again, and we are going to match that. I guess Linux doesn't output that stuff twice.
So as to a solution? I wonder if rather than expecting `EXITCODE:`, we can instead look for `\nEXITCODE', with a prepended newline? That way we won't match the keyword in the echo because there's a space before that keyword? Which would mean the expect line becomes this:
```scala
shell.expect(contains("\n" + keyWord))
```
--
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
[GitHub] [daffodil] stevedlawrence commented on pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on pull request #601:
URL: https://github.com/apache/daffodil/pull/601#issuecomment-898501749
> hope we caught all the places where the tests exited the debugger already and didn't need to send a quit command before capturing the exit status.
I think we did. If we didn't, we would send the `quit` command to bash instead of the debugger. But `quit` isn't a valid command, so bash will set the exit code to 127 (command not found). And since we always check the exit code in these tests now, a test would fail if it got 127 instead of the expected exit code.
--
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
[GitHub] [daffodil] tuxji commented on pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #601:
URL: https://github.com/apache/daffodil/pull/601#issuecomment-887500297
> If we can get the CI to run, I am ok with the changes here.
>
> I don't know why it doesn't run any more on each push of a new commit.
GitHub changed its CI policy two or three months ago because miners were spamming repos with one-off pull requests. A new contributor now needs a committer to manually run CI with a button press each time. Once the new contributor's first PR is merged, GitHub will run CI on the contributor's next PR automatically. We could suggest that new contributors make a tiny PR as their first PR before making larger PRs.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r677411142
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/schematron/package.scala
##########
@@ -25,15 +25,16 @@ import net.sf.expectit.matcher.Matchers.regexp
import net.sf.expectit.matcher.Matchers.sequence
import net.sf.expectit.Result
import net.sf.expectit.matcher.Matcher
-import org.junit.Assert.fail
+import org.apache.daffodil.Main.ExitCode
+//import org.junit.Assert.fail
import java.io.File
import java.io.FileOutputStream
import java.nio.file.Path
import java.nio.file.Paths
-import scala.util.Failure
-import scala.util.Success
-import scala.util.Try
+//import scala.util.Failure
+//import scala.util.Success
+//import scala.util.Try
Review comment:
Remove commented code
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/schematron/package.scala
##########
@@ -74,12 +75,7 @@ package object schematron {
try {
shell.sendLine(cmd).expect(expectation)
- val actualEc = shell.sendLine(echoEC).expect(matchEC).getBefore.trim.split(eol).last
Review comment:
The echoEC and matchEC variables can be removed below, they are no longer used.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675637285
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,25 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+
+ val keyWord = "EXITCODE:"
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+ shell.sendLine(exitCodeCmd)
+ shell.expect(contains(keyWord))
Review comment:
It looks like a handful of tests are failing to find this exit code, any idea why that is?
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r666908085
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
##########
@@ -1720,7 +1722,7 @@ class InteractiveDebugger(runner: InteractiveDebuggerRunner, eCompilers: Express
|Immediately abort all processing.""".stripMargin
override lazy val short = "q"
def act(args: Seq[String], state: ParseOrUnparseState, processor: Processor): DebugState.Type = {
- sys.exit(1)
+ throw new DebuggerExitException("")
Review comment:
Also, this exception isn't caught in the CLI, so if this is thrown it's going to show up as a bug. The CLI should catch this and output an appropriate error code. Probably ExitCode.Success is reasonable. since the debugger exiting is probably not considered a failure.
--
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
[GitHub] [daffodil] dasmallwood commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
dasmallwood commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675697850
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,25 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+
+ val keyWord = "EXITCODE:"
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+ shell.sendLine(exitCodeCmd)
+ shell.expect(contains(keyWord))
Review comment:
from running some of the tests manually i've seen the expect exit code operation timeout looking for an exitcode
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675737178
##########
File path: daffodil-cli/src/it/scala/org/apache/daffodil/CLI/Util.scala
##########
@@ -200,4 +203,25 @@ object Util {
}
inputFile
}
+
+ def expectExitCode(expectedExitCode: ExitCode.Value, shell: Expect): Unit = {
+ val expectedCode = expectedExitCode.id
+
+ val keyWord = "EXITCODE:"
+ val exitCodeCmd = "echo " + keyWord + (if (Util.isWindows) "%errorlevel%" else "$?")
+ shell.sendLine(exitCodeCmd)
+ shell.expect(contains(keyWord))
Review comment:
Looking at a handful of failing tests, it looks like ones that have this issue are ones that start the shell with:
```scala
val shell = Util.start("", true)
```
The `true` here is a flag sets the `expectErr` parameter, and looking at the getShell function, it ends up doing this:
```scala
val inputStream = if (expectErr) {
process.getErrorStream()
} else {
process.getInputStream()
}
val shell = new ExpectBuilder()
.withInputs(inputStream)
...
```
I think what this is saying is that when `expectErr = true`, expect only captures the error stream (i.e. stderr) instead of the input stream (i.e. stdout). So when `Util.start("", true)`, expect will never see EXITCODE because that is always output to stdout, which isn't captured. So it's impossible to use `expectErr = true` and check the exitcode.
Since all integration tests now need access to stdout to check against the exit code, that suggests that we really can't use the `Util.start("", true)` function anymore to only capture stderr. We always need to capture stdout too.
So, I wonder if our integration tests should just always capture BOTH stdout and stderr, and when they want to check something in stderr, they need to be explicit about it?
Note that we already have a function that does this: `Util.sartIncludeErrors`. The difference here is if you want to expect something in stderr, you need to be explicit and do this:
```scala
shell.expectIn(1, ...)
```
The `1` here says to check stderr.
So I'm thinking maybe that suggests we should just get rid of `getShell` and rename `getShellWithErrors` to `getShell`. That way all tests will capture both stdout and stderr. And it means `expectErr` parameter just goes away, and all tests are started with `Util.start("")`. And then we need to update tests that are looking at stderr (i.e. the ones passing `true` to the `start()` method) to use `shell.expectIn(1, ...)` instead of `shell.expect(...)`. Looks like there's 36 of these tests, so a decent number, but not a ton, and the changes are fairly straightforward.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r675574593
##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1570,79 @@ object Main extends Logging {
1
}
+ object ExitCode extends Enumeration {
+
+ val Success = Value(0)
+ val Failure = Value(1)
+
+ val FileNotFound = Value(2)
+ val OutOfMemory = Value(3)
+ val BugFound = Value(4)
+ val NotYetImplemented = Value(5)
+
+
+ val ParseError = Value(20)
+ val UnparseError = Value(21)
+ val GenerateCodeError = Value(23)
+ val TestError = Value(24)
+ val PerformanceTestError = Value(25)
+
+
+ val LeftOverData = Value(31)
+ val InvalidParserException = Value(32)
+ val BadExternalVariable = Value(33)
+ val UserDefinedFunctionError = Value(34)
+ val UnableToCreateProcessor = Value(35)
+
+ val Usage = Value(64)
+
+ }
+
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
- }
- case e: InvalidParserException => {
- log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.FileNotFound
}
case e: ExternalVariableException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BadExternalVariable
}
case e: BindingException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.BadExternalVariable
}
case e: NotYetImplementedException => {
nyiFound(e)
+ ExitCode.NotYetImplemented
}
case e: TDMLException => {
log(LogLevel.Error, "%s", e.getMessage())
- 1
+ ExitCode.TestError
}
case e: OutOfMemoryError => {
oomError(e)
+ ExitCode.OutOfMemory
}
case e: UserDefinedFunctionFatalErrorException => {
log(LogLevel.Error, "%s", e.getMessage())
e.printStackTrace()
- 1
+ ExitCode.UserDefinedFunctionError
+ }
+ case e: DebuggerExitException => {
+ log(LogLevel.Error, "%s", e.getMessage())
+ ExitCode.Failure
Review comment:
Should be ExitCode.Success. A quit command is a normal exit.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #601: Distinct return codes for CLI
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r663107605
##########
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:
Also, I would suggest that if the debugger exits via the "quit" command, that probably shouldn't be an error since that's correct behavior. So whatever happens when the debugger runs "quit", for the CLI it should probably result in a Success exit code.
--
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