Enable debug/trace for parse, unparse and test only by olabusayoT · Pull Request #1315 · apache/daffodil

olabusayoT · 2024-09-24T15:17:26Z

make debug/trace non global and sub option for parse, unparse and test only
update CLI tests

Deprecation/Compatibility:
--trace and --debug are no longer global arguments for daffodil, and are instead arguments for parse, unparse and test only.

DAFFODIL-1141

mbeckerle

+1

stevedlawrence

Useful change, just some comments about update the usage strings.

stevedlawrence · 2024-09-25T13:43:52Z

    banner("""|Usage: daffodil parse (-s <schema> [-r <root>] | -P <parser>)
              |                      [-c <file>] [-D<variable>=<value>...] [-I <infoset_type>]
              |                      [-o <output>] [--stream] [-T<tunable>=<value>...] [-V <mode>]
+              |                      [--trace | --debug <file>]


<file> should be in brackets to imply it's optional, e.g. [--trace | --debug [<file>]]. Same for the unparse command.

stevedlawrence · 2024-09-25T14:10:33Z

  object test extends scallop.Subcommand("test") {
    banner(
-      """|Usage: daffodil test [-I <implementation>] [-l] [-r] [-i] <tdmlfile> [testnames...]
+      """|Usage: daffodil test [-I <implementation>] [-l] [-r] [-i] [-d <file> | -d -- | -t] <tdmlfile> [testnames...]


-d -- isn't really the right way to signify the ability to use --, since it sort of implies -- is an argument to the -d option. But that's not really case. The -- can go anywhere and just says that everything following it should not be treated as an option or an option argument, and should instead be treated as a trailing argument.

We happen to only really need it for the -d option since there's sometimes an ambiguity if the thing following it is the file with debug commands or a trailing argument. This ambiguity is whys why it's needed in some of our tests that do something like this:

daffodil test -d foo.tdml`

In this case you do need the -- to say that foo.tdml is not actually an argument to the -d option, e.g.

daffodil test -d -- foo.tdml

But note that -- isn't always necessary if there is no ambiguity. For example, if you do this

daffodil test -d -r foo.tdml 'test_foo_.*'

Then scallop knows that -r is an option so it can infer the -d option did not have an argument, and so the -- isn't needed at all. This is also why you don't need -d -- for the parse/unparse tests. In those tests there is no ambiguity because -d is immediately followed by another option.

But note that the same problem can still happen for the parse/unparse commands, it's not specific to the test command. For example, if you did this:

daffodil parse -s foo.dfdl.xsd -d input.bin

Scallop would think "input.bin" is the file of debug commands, there would be no trailing args, and so daffodil would parse stdin. Instead, you would need to do:

daffodil parse -s foo.dfdl.xsd -d -- input.bin

The -- is also useful in rare edge cases where the file you want to parse looks like an argument. For example, if you had a file that started with a hyphen you would also need to use --, e.g.

daffodil parse -s foo.dfdl.xsd -- --file_that_looks_like_an_argument.bin

(there's good reason we don't start files with hyphens).

So all this to say that -- isn't really specific to -d so shouldn't be added to the usage like that. If we really want to make it clear that it can be used, probably the best way to do it is to add [--] before the trailing arguments. That would imply that -- is optional, but if you want it to deal with ambiguity then put it before trailing arguments.

Git does something similar for commands like git add, and the man page even says:

-- This option can be used to separate command-line options from the list of files, (useful when filenames might be mistaken for command-line options).

I'm not sure if scallop would let us add that to the option description though.

I decided to add the following comment to all the banners

-- can be used to separate command-line options from trailing arguments

stevedlawrence · 2024-09-25T14:10:55Z

+      descr =
+        "Enable the interactive debugger. Optionally, read initial debugger commands from [file] if provided. " +
+          "To use debug without a file, one must supply '--' inplace of file " +
+          "i.e 'daffodil test -d -- path/to/tests.tdml'"


This description isn't quite right. See above comment.

stevedlawrence · 2024-09-25T14:11:23Z

+  def withDebugOrTrace(
+                        proc: DFDL.DataProcessor,
+                        traceArg: ScallopOption[Boolean],
+                        debugArg: ScallopOption[Option[String]]


Indention got weird here. scalafmt should fix it?

stevedlawrence · 2024-09-25T14:32:46Z

              |                        [-c <file>] [-D<variable>=<value>...] [-I <infoset_type>]
              |                        [-o <output>] [--stream] [-T<tunable>=<value>...] [-V <mode>]
+              |                        [--trace | --debug <file>]
              |                        [infile]


These usage strings are starting to get a bit verbose. I'm wondering they are getting too complicated to be useful and if we should simplify the usage string to just something like this:

Usage: daffodil unparse (-s <schema> | -P <parser>) [UNPARSE_OPTS] [--] [infile] Unparse Options: -- scallop generated stuff--

So it shows only the required and commonly used options (-s and -P) and trailing args, and anything else is described in more detail below that. We would probably want to also modify some descriptions to mention restrictions. For example, '--root' would need to say that it's only valid with the --schema option. But that and trace/debug mutual exclusion are the only restrictions I think.

It also makes the [--] stand out a bit more so might make it's availability more clear. And it means we don't have to update this usage and make it even more complicated every time we add a new option.

stevedlawrence

+1, just some suggestions about removing extra verbosity. Keeping --help output as minimal as possible it important to keep it from being overwhelming.

stevedlawrence · 2024-09-25T17:59:09Z

-        "Root element to use. Can be prefixed with {namespace}. Must be a top-level element. Defaults to first top-level element of DFDL schema."
+      descr = "Root element to use. Can be prefixed with {namespace}. " +
+        "Must be a top-level element. Defaults to first top-level element of DFDL schema. " +
+        "Only valid with the --schema option. Cannot be used with the --parser option."


Just to keep as succinct as possible to keep --help minimal, we could probably leave off "cannot be used with the --parser option". I think that's implied by saying it's only valid with the --schema option and the usage showing you can only have one one of -s or -P. Same below.

stevedlawrence · 2024-09-25T18:00:36Z

+      argName = "file",
+      descr =
+        "Enable the interactive debugger. Optionally, read initial debugger commands from [file] if provided. " +
+          "Cannot be used with --trace option. They are mutually exclusive options."


Remove "they are mutually exclusive options, think "Cannot be used with ---trace option" is sufficient.

stevedlawrence · 2024-09-25T18:03:17Z

              |
              |Parse a file, using either a DFDL schema or a saved parser
              |
+              |-- can be used to separate command-line options from trailing arguments


I'll open a ticket with scallop to see if they can add a way for "--" to show up in the --help output. It's usage is rare enough that it probably doesn't belong in the main description, but I agree that a description is needed. I think this is unfortunately the only place it can exist without a scallop change. This is fine for now, but hopefully scallop can support it.

- make debug/trace non global and sub option for parse, unparse and test only - update CLI tests - update daffodil-test-integration - update command line documentation Deprecation/Compatibility: --trace and --debug are no longer global arguments for daffodil, and are instead arguments for parse, unparse and test only. DAFFODIL-1141

mbeckerle · 2024-09-30T18:08:34Z

Does trace in fact work on daffodil test? I'd swear I tried this the other day and got no trace output. Do you perhaps need to do daffodil -t trace -i (both -t and -i in the older syntax) to see the trace output? I definitely wanted this to work and was surprised when it did not. I could not tell you which daffodil version I was using. 3.5.0 or 3.8.0, but I'm not sure which.

olabusayoT · 2024-09-30T18:38:52Z

Does trace in fact work on daffodil test? I'd swear I tried this the other day and got no trace output. Do you perhaps need to do daffodil -t trace -i (both -t and -i in the older syntax) to see the trace output? I definitely wanted this to work and was surprised when it did not. I could not tell you which daffodil version I was using. 3.5.0 or 3.8.0, but I'm not sure which.

It definitely works for the cli...for both 3.5.0 and 3.8.0 without needing to do anything other than:
daffodil --trace parse -s schema.xsd data.bin
or
daffodil -t parse -s schema.xsd data.bin

I also confirmed Runner.trace works on both 3.5.0 and 3.8.0

stevedlawrence · 2024-09-30T18:55:50Z

I confirmed it also works with the test subcommand with 3.9.0, e.g. daffodil --trace test foo.tdml.

olabusayoT · 2024-09-30T20:00:05Z

Oh I misunderstood your comment. Attempts to use "daffodil --trace test csv/test/csv.tdml" doesn't work @stevedlawrence , what tdml file did you use?

olabusayoT · 2024-09-30T20:11:18Z

Oh I misunderstood your comment. Attempts to use "daffodil --trace test csv/test/csv.tdml" doesn't work @stevedlawrence , what tdml file did you use?

Ah looks like that's because csv.tdml fails. It works when I try "daffodil --trace test daffodil-test/src/test/resources/org/apache/daffodil/section06/entities/Entities.tdml" for 3.8.0 but not for 3.5.0

stevedlawrence · 2024-09-30T20:28:48Z

For CSV I had to run it from the src/ directory so that Daffodil could find the non-embedded schema, e.g.

cd path/to/csv/src/
daffodil --trace test ../test/csv.tdml

mbeckerle · 2024-09-30T20:30:05Z

I learned --trace or -t with daffodil test DOES work with 3.8.0, does NOT work with 3.5.0.

mbeckerle@orca:~/dataiti/opensource/DFDLSchemas/faketdl$ daffodil --version
Apache Daffodil 3.5.0
mbeckerle@orca:~/dataiti/opensource/DFDLSchemas/faketdl$ daffodil -t test test/TestFakeTDL.tdml test_msg_01
[Pass] test_msg_01

Total: 1, Pass: 1, Fail: 0, Not Found: 0
mbeckerle@orca:~/dataiti/opensource/DFDLSchemas/faketdl$ daffodil -t test -i test/TestFakeTDL.tdml test_msg_01
[Pass] test_msg_01

Total: 1, Pass: 1, Fail: 0, Not Found: 0

olabusayoT requested review from mbeckerle and stevedlawrence September 24, 2024 15:17

olabusayoT force-pushed the daf-1141-makeTraceDebugNonGlobal branch from c3a4c7b to c1e3b0f Compare September 24, 2024 15:24

mbeckerle approved these changes Sep 24, 2024

View reviewed changes

stevedlawrence requested changes Sep 25, 2024

View reviewed changes

stevedlawrence approved these changes Sep 25, 2024

View reviewed changes

olabusayoT force-pushed the daf-1141-makeTraceDebugNonGlobal branch from 9b1042e to b36811b Compare September 30, 2024 17:34

olabusayoT merged commit aaf9f1c into apache:main Sep 30, 2024

olabusayoT deleted the daf-1141-makeTraceDebugNonGlobal branch September 30, 2024 21:04

Conversation

olabusayoT commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbeckerle left a comment

Choose a reason for hiding this comment

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbeckerle commented Sep 30, 2024

Uh oh!

olabusayoT commented Sep 30, 2024

Uh oh!

stevedlawrence commented Sep 30, 2024

Uh oh!

olabusayoT commented Sep 30, 2024

Uh oh!

olabusayoT commented Sep 30, 2024

Uh oh!

stevedlawrence commented Sep 30, 2024

Uh oh!

mbeckerle commented Sep 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olabusayoT commented Sep 24, 2024 •

edited

Loading