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/04/29 17:06:21 UTC
[GitHub] [daffodil] tuxji opened a new pull request #545: Fix some runtime2 bugs
tuxji opened a new pull request #545:
URL: https://github.com/apache/daffodil/pull/545
Remove dependency on argp and use getopt instead. Build with clang
instead of gcc. Lower minimum required Mini-XML version from 3.2 to
3.0. Add missing LICENSE and NOTICE files to daffodil-runtime2 jar.
main.yml: Build with clang and llvm-ar instead of cc and ar.
BUILD.md: Lower Mini-XML version to 3.0. Remove argp and gcc
instructions. Add clang instructions.
README.md: Lower Mini-XML version to 3.0.
LICENSE: Add to daffdil-runtime2 jar.
NOTICE: Add to daffdil-runtime2 jar.
Makefile: Improve comments explaining how to use targets.
daffodil_argp.[ch]: Rename to daffodil_getopt.[ch].
daffodil_getopt.c: Remove all argp structs and handlers. Simplify to
just a single function calling getopt and returning a pointer to Error
if any error happens. Note there is no portable way to parse
"daffodil [options] command [more options] arguments" with getopt. We
will code the simplest way (hope that getopt will move all non-option
arguments to the end) and users will have to put all options before
all arguments if their getopt doesn't support that behavior ("daffodil
[options] [more options] command arguments").
daffodil_getopt.h: Include "errors.h" and make parse_daffodil_cli
return a pointer to Error so we can use continue_or_exit(error) for
all errors/messages.
daffodil_main.c: Remove fflush_continue_or_exit (no really good reason
to flush a stream right before closing it). Call continue_or_exit
after parse_daffodil_cli to handle any CLi error. Simplify rest of
code in main function.
errors.c: Move eof_or_error, get_diagnostics, add_diagnostics up so
they come first in file. Add error_message switch cases to define all
CLI messages. Add print_maybe_stop switch cases to print all CLI
messages, making sure to call printf & exit instead of error for CLI
help & version messages.
errors.h: Add CLI messages to ErrorCode and "int c" member to
ErrorCode anonymous union. Rename ERR_INFOSET_READ/WRITE to
CLI_INVALID_INFOSET since it's a CLI message too. Declare
daffodil_program_version (we were using argp_program_version before
which is no longer available) and move eof_or_error up before
get_diagnostics.
NestedUnion.c, ex_nums.c: Regenerate with code generator.
CodeGenerator.scala: Remove "-largp" since we no longer need it.
CodeGeneratorState.scala: Replace argp_program_version with
daffodil_program_version since we no longer use "argp.h".
DAFFODIL-2500 DAFFODIL-2505 DAFFODIL-2508
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r623398233
##########
File path: BUILD.md
##########
@@ -31,26 +31,32 @@ the latest [SBT] version following their websites' instructions or
install them using your operating system's package manager.
Since Daffodil now has a C backend as well as a Scala backend, you
-will need a C compiler supporting the [C99] standard or later, the
-[Mini-XML] library, and possibly the GNU [argp] library if your
-system's C library doesn't include it already. You can install either
-[gcc] or [clang] using your operating system's package manager. If
-you can't install the [Mini-XML] library using your operating system's
-package manager, you'll have to build it from source. We'll tell you
-how to do that on Windows but the commands should work on other
-operating systems too.
-
-You can set your environment variables "CC" and "AR" to the correct
+will need a C compiler supporting the [C99] standard or later and the
+[Mini-XML] library. You can install either [gcc] or [clang] using
+your operating system's package manager. If you can't install the
+[Mini-XML] library using your operating system's package manager,
+you'll have to build it from source. We'll tell you how to do that on
+Windows but the commands should work on other operating systems too.
+
+You can set your environment variables `CC` and `AR` to the correct
commands (or set them to `true` to disable C compilation altogether)
if you don't want `sbt compile` to call your C compiler with `cc` and
`ar` as the default commands.
-## Fedora 33
+## Fedora 34
-You can use the `dnf` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `dnf` package manager to install most of the tools
+needed to build Daffodil:
- sudo dnf install gcc git java-11-openjdk-devel make mxml-devel pkgconf
+ sudo dnf install clang git java-11-openjdk-devel llvm make mxml-devel pkgconf
+
+If you want to use clang instead of gcc, you'll have to set your
Review comment:
You set it up so it is clang by default now? So is this instruction correct still, or does it have these settings now, and you have to flip them to use gcc now?
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/errors.c
##########
@@ -17,34 +17,109 @@
#include "errors.h"
#include <assert.h> // for assert
-#include <error.h> // for error
#include <inttypes.h> // for PRId64
#include <stdbool.h> // for bool, false, true
-#include <stdio.h> // for NULL, feof, ferror, FILE, size_t
-#include <stdlib.h> // for EXIT_FAILURE
+#include <stdio.h> // for fprintf, stderr, NULL, feof, ferror, FILE, size_t, stdout
+#include <stdlib.h> // for exit, EXIT_FAILURE
-// error_message - return an internationalized error message
+// eof_or_error - get pointer to error if stream has eof or error indicator set
+
+const Error *
+eof_or_error(FILE *stream)
+{
+ if (feof(stream))
+ {
+ static Error error = {ERR_STREAM_EOF, {NULL}};
+ return &error;
+ }
+ else if (ferror(stream))
+ {
+ static Error error = {ERR_STREAM_ERROR, {NULL}};
+ return &error;
+ }
+ else
+ {
+ return NULL;
+ }
+}
+
+// get_diagnostics - get pointer to validation diagnostics
+
+Diagnostics *
+get_diagnostics(void)
+{
+ static Diagnostics diagnostics;
+ return &diagnostics;
+}
+
+// add_diagnostic - add a new error to validation diagnostics
+
+bool
+add_diagnostic(Diagnostics *diagnostics, const Error *error)
+{
+ if (diagnostics && error)
+ {
+ if (diagnostics->length < LIMIT_DIAGNOSTICS)
+ {
+ Error *err = &diagnostics->array[diagnostics->length++];
+ err->code = error->code;
+ err->s = error->s;
+ return true;
+ }
+ }
+ return false;
+}
+
+// error_message - get an internationalized error message
static const char *
error_message(enum ErrorCode code)
Review comment:
Feels like this should be in a CLI-oriented file, since it's giving CLI-help.
##########
File path: BUILD.md
##########
@@ -82,10 +96,18 @@ Install [MSYS2] following its website's instructions and open a new
"MSYS2 MSYS" window. We'll need its collection of free programs and
libraries.
-Install [gcc] and [libargp][argp] using MSYS2's `pacman` package
-manager:
+You can use the `pacman` package manager to install most of the tools
+needed to build Daffodil:
+
+ pacman -S clang diffutils git make pkgconf
+
+If you want to use clang instead of gcc, you'll have to set your
+environment variables `CC` and `AR` to the clang binaries' names:
Review comment:
and here
##########
File path: BUILD.md
##########
@@ -61,10 +67,18 @@ commands you type will be able to call the C compiler.
## Ubuntu 20.04
-You can use the `apt` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `apt` package manager to install most of the tools
+needed to build Daffodil:
+
+ sudo apt install build-essential clang-10 clang-format-10 default-jdk git libmxml-dev
- sudo apt install build-essential default-jdk git libmxml-dev
+If you want to use clang instead of gcc, you'll have to set your
Review comment:
ditto here. Ithought this was the default now.
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/daffodil_main.c
##########
@@ -15,13 +15,13 @@
* limitations under the License.
*/
-#include <stdio.h> // for NULL, perror, FILE, fclose, fflush, fopen, stdin, stdout
-#include <string.h> // for strcmp
-#include "daffodil_argp.h" // for daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, daffodil_cli, parse_daffodil_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, Error, print_diagnostics, PState, UState, ERR_FILE_CLOSE, ERR_FILE_FLUSH, ERR_FILE_OPEN, ERR_INFOSET_READ, ERR_INFOSET_WRITE
-#include "infoset.h" // for walkInfoset, InfosetBase, rootElement, ERD, VisitEventHandler
-#include "xml_reader.h" // for xmlReaderMethods, XMLReader
-#include "xml_writer.h" // for xmlWriterMethods, XMLWriter
+#include <stdio.h> // for NULL, FILE, perror, fclose, fopen, stdin, stdout
+#include <string.h> // for strcmp
+#include "daffodil_getopt.h" // for daffodil_cli, parse_daffodil_cli, daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
Review comment:
Line too long
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji merged pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
tuxji merged pull request #545:
URL: https://github.com/apache/daffodil/pull/545
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r624161157
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/daffodil_main.c
##########
@@ -15,13 +15,13 @@
* limitations under the License.
*/
-#include <stdio.h> // for NULL, perror, FILE, fclose, fflush, fopen, stdin, stdout
-#include <string.h> // for strcmp
-#include "daffodil_argp.h" // for daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, daffodil_cli, parse_daffodil_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, Error, print_diagnostics, PState, UState, ERR_FILE_CLOSE, ERR_FILE_FLUSH, ERR_FILE_OPEN, ERR_INFOSET_READ, ERR_INFOSET_WRITE
-#include "infoset.h" // for walkInfoset, InfosetBase, rootElement, ERD, VisitEventHandler
-#include "xml_reader.h" // for xmlReaderMethods, XMLReader
-#include "xml_writer.h" // for xmlWriterMethods, XMLWriter
+#include <stdio.h> // for NULL, FILE, perror, fclose, fopen, stdin, stdout
+#include <string.h> // for strcmp
+#include "daffodil_getopt.h" // for daffodil_cli, parse_daffodil_cli, daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
Review comment:
I don't have any problem with these comments, that said,
> If a developer is curious to learn more about a symbol, he or she can find exactly where to look by searching the comments to find the right include header and then open that .h file (also the .c file if both are in the repository).
Most IDE's and even text editors used for coding have an easy way to jump directly to the definition of a symbol. ctags+vim is my go-to, but there's lots of options for a more efficient alternative than searching through #include comments.
Your workflow to update these comments also seems pretty laborious. If it's not automated, I would expect that once other developers get involved that aren't used to this workflow it's going to be hard to keep these comments updated and useful. The removal of superfluous #includes does seem like a useful feature of iwyu though (I wasn't aware of this tool). Seems like there might be some benefit it automating some of these capabilities.
FYI, I've spoken to some devs recently that sound interested in this runtime2 and have similar FPGA goals, hoping to get them involved soon.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #545:
URL: https://github.com/apache/daffodil/pull/545#issuecomment-829595343
The rationale for lowering the dependency to 3.0 for mini-XML is what?
So that people won't have to dig around for the newer one if they already have the old one on their system?
That's kind of at cross purposes to all the scala-steward philosphy we use elsewhere now of moving forward to newer library versions as soon as they're available and stable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on pull request #545:
URL: https://github.com/apache/daffodil/pull/545#issuecomment-830408929
I like splitting the errors up that way yes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r623515706
##########
File path: README.md
##########
@@ -43,7 +43,7 @@ For more information about Daffodil, see <https://daffodil.apache.org/>.
* JDK 8 or higher
* SBT 0.13.8 or higher
* C compiler C99 or higher
-* Mini-XML Version 3.2 or higher
+* Mini-XML Version 3.0 or higher
Review comment:
Yes, the rationale for lowering the Mini-XML dependency is so that people won't have to build Mini-XML 3.2 from source if they already have 3.1 on their system (Steve's Fedora 33 had 3.1 although Fedora 34 has 3.2 now). I looked at the changes between 3.0, 3.1, and 3.2 carefully and determined that there were only fairly minor changes, nothing that would make someone regret running 3.1 or 3.0 instead of 3.2. I do agree with scala-steward's philosophy of moving to newer library versions as soon as they're available and stable but the preferred definition of "available" for a C developer may be whatever is available in a distribution's repositories.
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/errors.c
##########
@@ -17,34 +17,109 @@
#include "errors.h"
#include <assert.h> // for assert
-#include <error.h> // for error
#include <inttypes.h> // for PRId64
#include <stdbool.h> // for bool, false, true
-#include <stdio.h> // for NULL, feof, ferror, FILE, size_t
-#include <stdlib.h> // for EXIT_FAILURE
+#include <stdio.h> // for fprintf, stderr, NULL, feof, ferror, FILE, size_t, stdout
+#include <stdlib.h> // for exit, EXIT_FAILURE
-// error_message - return an internationalized error message
+// eof_or_error - get pointer to error if stream has eof or error indicator set
+
+const Error *
+eof_or_error(FILE *stream)
+{
+ if (feof(stream))
+ {
+ static Error error = {ERR_STREAM_EOF, {NULL}};
+ return &error;
+ }
+ else if (ferror(stream))
+ {
+ static Error error = {ERR_STREAM_ERROR, {NULL}};
+ return &error;
+ }
+ else
+ {
+ return NULL;
+ }
+}
+
+// get_diagnostics - get pointer to validation diagnostics
+
+Diagnostics *
+get_diagnostics(void)
+{
+ static Diagnostics diagnostics;
+ return &diagnostics;
+}
+
+// add_diagnostic - add a new error to validation diagnostics
+
+bool
+add_diagnostic(Diagnostics *diagnostics, const Error *error)
+{
+ if (diagnostics && error)
+ {
+ if (diagnostics->length < LIMIT_DIAGNOSTICS)
+ {
+ Error *err = &diagnostics->array[diagnostics->length++];
+ err->code = error->code;
+ err->s = error->s;
+ return true;
+ }
+ }
+ return false;
+}
+
+// error_message - get an internationalized error message
static const char *
error_message(enum ErrorCode code)
Review comment:
Yeah, I originally had these CLI messages in daffodil_getopt.c and thought upon it for a while before deciding it was easier to bundle all the messages in one place for easier internationalization and flow of control (daffodil_parse_cli needs only return statements, not printf and exit calls too). However, if we don't care about hardcoding 'daffodil' in the help, daffodil_parse_cli could pass the help message in err->s and both CLI_HELP_USAGE and CLI_PROGRAM_VERSION then would use the same format, "%s\n". Would you prefer that?
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/daffodil_main.c
##########
@@ -15,13 +15,13 @@
* limitations under the License.
*/
-#include <stdio.h> // for NULL, perror, FILE, fclose, fflush, fopen, stdin, stdout
-#include <string.h> // for strcmp
-#include "daffodil_argp.h" // for daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, daffodil_cli, parse_daffodil_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, Error, print_diagnostics, PState, UState, ERR_FILE_CLOSE, ERR_FILE_FLUSH, ERR_FILE_OPEN, ERR_INFOSET_READ, ERR_INFOSET_WRITE
-#include "infoset.h" // for walkInfoset, InfosetBase, rootElement, ERD, VisitEventHandler
-#include "xml_reader.h" // for xmlReaderMethods, XMLReader
-#include "xml_writer.h" // for xmlWriterMethods, XMLWriter
+#include <stdio.h> // for NULL, FILE, perror, fclose, fopen, stdin, stdout
+#include <string.h> // for strcmp
+#include "daffodil_getopt.h" // for daffodil_cli, parse_daffodil_cli, daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
Review comment:
Which would you prefer?
1. Truncate to 100 columns (losing information)
2. Wrap to 100 columns
3. Keep as is (remember these comments are machine generated)
##########
File path: BUILD.md
##########
@@ -31,26 +31,32 @@ the latest [SBT] version following their websites' instructions or
install them using your operating system's package manager.
Since Daffodil now has a C backend as well as a Scala backend, you
-will need a C compiler supporting the [C99] standard or later, the
-[Mini-XML] library, and possibly the GNU [argp] library if your
-system's C library doesn't include it already. You can install either
-[gcc] or [clang] using your operating system's package manager. If
-you can't install the [Mini-XML] library using your operating system's
-package manager, you'll have to build it from source. We'll tell you
-how to do that on Windows but the commands should work on other
-operating systems too.
-
-You can set your environment variables "CC" and "AR" to the correct
+will need a C compiler supporting the [C99] standard or later and the
+[Mini-XML] library. You can install either [gcc] or [clang] using
+your operating system's package manager. If you can't install the
+[Mini-XML] library using your operating system's package manager,
+you'll have to build it from source. We'll tell you how to do that on
+Windows but the commands should work on other operating systems too.
+
+You can set your environment variables `CC` and `AR` to the correct
commands (or set them to `true` to disable C compilation altogether)
if you don't want `sbt compile` to call your C compiler with `cc` and
`ar` as the default commands.
-## Fedora 33
+## Fedora 34
-You can use the `dnf` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `dnf` package manager to install most of the tools
+needed to build Daffodil:
- sudo dnf install gcc git java-11-openjdk-devel make mxml-devel pkgconf
+ sudo dnf install clang git java-11-openjdk-devel llvm make mxml-devel pkgconf
+
+If you want to use clang instead of gcc, you'll have to set your
Review comment:
The default compiler for sbt and make is cc, which is an alias for gcc. All I'm doing is showing how to install clang (which also installs gcc, FYI) and define the environment variables CC and AR to use clang instead of gcc. I have set up the CI workflow in main.yml to use clang by default, but I can't set up your own environment variables for you. You'll still have to set up CC and AR in your development environment if you want to use clang instead of gcc. Do you want me to make these instructions clearer?
##########
File path: BUILD.md
##########
@@ -61,10 +67,18 @@ commands you type will be able to call the C compiler.
## Ubuntu 20.04
-You can use the `apt` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `apt` package manager to install most of the tools
+needed to build Daffodil:
+
+ sudo apt install build-essential clang-10 clang-format-10 default-jdk git libmxml-dev
- sudo apt install build-essential default-jdk git libmxml-dev
+If you want to use clang instead of gcc, you'll have to set your
Review comment:
FYI, both the apt install here and the pacman -S below actually install gcc too even though it's no longer specified on the command line. But the default system compiler is still gcc since it's hard to change that without running some extra commands as root or defining your CC environment variable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a change in pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r624139525
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/daffodil_main.c
##########
@@ -15,13 +15,13 @@
* limitations under the License.
*/
-#include <stdio.h> // for NULL, perror, FILE, fclose, fflush, fopen, stdin, stdout
-#include <string.h> // for strcmp
-#include "daffodil_argp.h" // for daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, daffodil_cli, parse_daffodil_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, Error, print_diagnostics, PState, UState, ERR_FILE_CLOSE, ERR_FILE_FLUSH, ERR_FILE_OPEN, ERR_INFOSET_READ, ERR_INFOSET_WRITE
-#include "infoset.h" // for walkInfoset, InfosetBase, rootElement, ERD, VisitEventHandler
-#include "xml_reader.h" // for xmlReaderMethods, XMLReader
-#include "xml_writer.h" // for xmlWriterMethods, XMLWriter
+#include <stdio.h> // for NULL, FILE, perror, fclose, fopen, stdin, stdout
+#include <string.h> // for strcmp
+#include "daffodil_getopt.h" // for daffodil_cli, parse_daffodil_cli, daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
Review comment:
No, I don't want to remove the comments. The whole point of the comments is for me to show a developer looking at the code exactly what is being used from each include header. If a developer is curious to learn more about a symbol, he or she can find exactly where to look by searching the comments to find the right include header and then open that .h file (also the .c file if both are in the repository).
The includes are not autogenerated. The file has to compile successfully before iwyu can process it and iwyu doesn't edit the file. It only prints some replacement includes / comments on standard output along with some other output and I have to manually edit the file to replace the includes / comments. In fact, iwyu won't even print the replacement includes / comments if the only changes would be to the comments (it has no way of knowing what the original comments were), so I have to manually add an extra include in order for iwyu to show me the updated comments. I wouldn't bother using iwyu unless it was to put these comments there as well as get rid of unneeded includes.
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/errors.c
##########
@@ -17,34 +17,109 @@
#include "errors.h"
#include <assert.h> // for assert
-#include <error.h> // for error
#include <inttypes.h> // for PRId64
#include <stdbool.h> // for bool, false, true
-#include <stdio.h> // for NULL, feof, ferror, FILE, size_t
-#include <stdlib.h> // for EXIT_FAILURE
+#include <stdio.h> // for fprintf, stderr, NULL, feof, ferror, FILE, size_t, stdout
+#include <stdlib.h> // for exit, EXIT_FAILURE
-// error_message - return an internationalized error message
+// eof_or_error - get pointer to error if stream has eof or error indicator set
+
+const Error *
+eof_or_error(FILE *stream)
+{
+ if (feof(stream))
+ {
+ static Error error = {ERR_STREAM_EOF, {NULL}};
+ return &error;
+ }
+ else if (ferror(stream))
+ {
+ static Error error = {ERR_STREAM_ERROR, {NULL}};
+ return &error;
+ }
+ else
+ {
+ return NULL;
+ }
+}
+
+// get_diagnostics - get pointer to validation diagnostics
+
+Diagnostics *
+get_diagnostics(void)
+{
+ static Diagnostics diagnostics;
+ return &diagnostics;
+}
+
+// add_diagnostic - add a new error to validation diagnostics
+
+bool
+add_diagnostic(Diagnostics *diagnostics, const Error *error)
+{
+ if (diagnostics && error)
+ {
+ if (diagnostics->length < LIMIT_DIAGNOSTICS)
+ {
+ Error *err = &diagnostics->array[diagnostics->length++];
+ err->code = error->code;
+ err->s = error->s;
+ return true;
+ }
+ }
+ return false;
+}
+
+// error_message - get an internationalized error message
static const char *
error_message(enum ErrorCode code)
Review comment:
Sounds like you would prefer that libcli have its own cli_errors.[ch] with similar enum, struct, and functions just to keep all the CLI_* codes and messages outside of libruntime. Mike, do you want that too?
##########
File path: .github/workflows/main.yml
##########
@@ -53,7 +59,7 @@ jobs:
if: runner.os == 'Windows'
uses: msys2/setup-msys2@v2
with:
- install: gcc libargp-devel make pkgconf
+ install: clang diffutils make pkgconf
Review comment:
We don't need diffutils to build daffodil in the CI workflow. I was including it only for consistency with BUILD.md where I wanted to show developers how to install nearly everything they might need. I'll remove diffutils to keep the MSYS2 install as small as possible.
##########
File path: BUILD.md
##########
@@ -31,26 +31,32 @@ the latest [SBT] version following their websites' instructions or
install them using your operating system's package manager.
Since Daffodil now has a C backend as well as a Scala backend, you
-will need a C compiler supporting the [C99] standard or later, the
-[Mini-XML] library, and possibly the GNU [argp] library if your
-system's C library doesn't include it already. You can install either
-[gcc] or [clang] using your operating system's package manager. If
-you can't install the [Mini-XML] library using your operating system's
-package manager, you'll have to build it from source. We'll tell you
-how to do that on Windows but the commands should work on other
-operating systems too.
-
-You can set your environment variables "CC" and "AR" to the correct
+will need a C compiler supporting the [C99] standard or later and the
+[Mini-XML] library. You can install either [gcc] or [clang] using
+your operating system's package manager. If you can't install the
+[Mini-XML] library using your operating system's package manager,
+you'll have to build it from source. We'll tell you how to do that on
+Windows but the commands should work on other operating systems too.
+
+You can set your environment variables `CC` and `AR` to the correct
commands (or set them to `true` to disable C compilation altogether)
if you don't want `sbt compile` to call your C compiler with `cc` and
`ar` as the default commands.
-## Fedora 33
+## Fedora 34
-You can use the `dnf` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `dnf` package manager to install most of the tools
+needed to build Daffodil:
- sudo dnf install gcc git java-11-openjdk-devel make mxml-devel pkgconf
+ sudo dnf install clang git java-11-openjdk-devel llvm make mxml-devel pkgconf
+
+If you want to use clang instead of gcc, you'll have to set your
+environment variables `CC` and `AR` to the clang binaries' names:
+
+ CC=clang
+ AR=llvm-ar
+ export CC
+ export AR
Review comment:
I know there are some differences and incompatibilities between Unix shells, but I agree it'd be cleaner to do the export in one line (that's what I've been typing anyway).
##########
File path: BUILD.md
##########
@@ -82,10 +96,18 @@ Install [MSYS2] following its website's instructions and open a new
"MSYS2 MSYS" window. We'll need its collection of free programs and
libraries.
-Install [gcc] and [libargp][argp] using MSYS2's `pacman` package
-manager:
+You can use the `pacman` package manager to install most of the tools
+needed to build Daffodil:
+
+ pacman -S clang diffutils git make pkgconf
Review comment:
I wanted to show developers how to install nearly everything they might need for runtime2 development on each platform. The only time you need diffutils is to run the c/Makefile's "make test" rule, which you'd do only by hand. Actually you need [xmldiff](https://xmldiff.readthedocs.io/en/stable/installation.html) for that Makefile rule too but I decided not to mention xmldiff in BUILD.md because there's no MSYS2 package and the procedure's too complicated for most developers (install Python3 and pip, use pip to install xmldiff). I also thought it would be too complicated to show developers how to build and run [include-what-you-use](https://github.com/include-what-you-use/include-what-you-use/blob/master/README.md), another tool that I use for updating the runtime2 C files' includes and comments showing what is used from each include. Using iwyu requires some manual editing, so it's not easy to automate in CI either.
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/examples/NestedUnion.c
##########
@@ -25,7 +25,7 @@
// Initialize our program's name and version
-const char *argp_program_version = "daffodil-runtime2 3.1.0-SNAPSHOT";
+const char *daffodil_program_version = "daffodil-runtime2 3.1.0-SNAPSHOT";
Review comment:
No, this whole file is just a very lightly edited copy of an autogenerated file to be shown as an example of what generated code looks like. We've had some discussion about failing the CI workflow if there's a difference (which would be a hassle) or auto-updating these example files in a sbt build (which would be better). If we wanted to auto-update these files, we would have to delete the copyrights and change the filenames (unless you decide it's fine for the code generator to include the Apache copyright in its generated_code.[ch] files).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a change in pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r624218996
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/errors.c
##########
@@ -17,34 +17,109 @@
#include "errors.h"
#include <assert.h> // for assert
-#include <error.h> // for error
#include <inttypes.h> // for PRId64
#include <stdbool.h> // for bool, false, true
-#include <stdio.h> // for NULL, feof, ferror, FILE, size_t
-#include <stdlib.h> // for EXIT_FAILURE
+#include <stdio.h> // for fprintf, stderr, NULL, feof, ferror, FILE, size_t, stdout
+#include <stdlib.h> // for exit, EXIT_FAILURE
-// error_message - return an internationalized error message
+// eof_or_error - get pointer to error if stream has eof or error indicator set
+
+const Error *
+eof_or_error(FILE *stream)
+{
+ if (feof(stream))
+ {
+ static Error error = {ERR_STREAM_EOF, {NULL}};
+ return &error;
+ }
+ else if (ferror(stream))
+ {
+ static Error error = {ERR_STREAM_ERROR, {NULL}};
+ return &error;
+ }
+ else
+ {
+ return NULL;
+ }
+}
+
+// get_diagnostics - get pointer to validation diagnostics
+
+Diagnostics *
+get_diagnostics(void)
+{
+ static Diagnostics diagnostics;
+ return &diagnostics;
+}
+
+// add_diagnostic - add a new error to validation diagnostics
+
+bool
+add_diagnostic(Diagnostics *diagnostics, const Error *error)
+{
+ if (diagnostics && error)
+ {
+ if (diagnostics->length < LIMIT_DIAGNOSTICS)
+ {
+ Error *err = &diagnostics->array[diagnostics->length++];
+ err->code = error->code;
+ err->s = error->s;
+ return true;
+ }
+ }
+ return false;
+}
+
+// error_message - get an internationalized error message
static const char *
error_message(enum ErrorCode code)
Review comment:
That makes sense to me yes. We don't have to do this for this change set. We can do it subsequently if you prefer. But we definitely want libruntime to be usable from an application which has nothing whatsoever to do with CLIs nor XML, and has its own error architecture used to create error messages from the error-codes coming back from the libruntime.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #545:
URL: https://github.com/apache/daffodil/pull/545#issuecomment-830826020
OK, I've moved everything CLI-related in libruntime out to libcli now. I made the error lookup mechanism pluggable (libcli has a hook to insert its own error lookup routine). I also made changes to help automate clang-format, include-what-you-use, and generated_code.[ch] updates later.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r623987273
##########
File path: .github/workflows/main.yml
##########
@@ -53,7 +59,7 @@ jobs:
if: runner.os == 'Windows'
uses: msys2/setup-msys2@v2
with:
- install: gcc libargp-devel make pkgconf
+ install: clang diffutils make pkgconf
Review comment:
Is diffutils needed?
##########
File path: BUILD.md
##########
@@ -31,26 +31,32 @@ the latest [SBT] version following their websites' instructions or
install them using your operating system's package manager.
Since Daffodil now has a C backend as well as a Scala backend, you
-will need a C compiler supporting the [C99] standard or later, the
-[Mini-XML] library, and possibly the GNU [argp] library if your
-system's C library doesn't include it already. You can install either
-[gcc] or [clang] using your operating system's package manager. If
-you can't install the [Mini-XML] library using your operating system's
-package manager, you'll have to build it from source. We'll tell you
-how to do that on Windows but the commands should work on other
-operating systems too.
-
-You can set your environment variables "CC" and "AR" to the correct
+will need a C compiler supporting the [C99] standard or later and the
+[Mini-XML] library. You can install either [gcc] or [clang] using
+your operating system's package manager. If you can't install the
+[Mini-XML] library using your operating system's package manager,
+you'll have to build it from source. We'll tell you how to do that on
+Windows but the commands should work on other operating systems too.
+
+You can set your environment variables `CC` and `AR` to the correct
commands (or set them to `true` to disable C compilation altogether)
if you don't want `sbt compile` to call your C compiler with `cc` and
`ar` as the default commands.
-## Fedora 33
+## Fedora 34
-You can use the `dnf` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `dnf` package manager to install most of the tools
+needed to build Daffodil:
- sudo dnf install gcc git java-11-openjdk-devel make mxml-devel pkgconf
+ sudo dnf install clang git java-11-openjdk-devel llvm make mxml-devel pkgconf
+
+If you want to use clang instead of gcc, you'll have to set your
+environment variables `CC` and `AR` to the clang binaries' names:
+
+ CC=clang
+ AR=llvm-ar
+ export CC
+ export AR
Review comment:
Should we suggest doing these on one line, e.g. ``export CC=clang`` to make things cleaner. Or are there cases where that doesn't work?
##########
File path: BUILD.md
##########
@@ -82,10 +96,18 @@ Install [MSYS2] following its website's instructions and open a new
"MSYS2 MSYS" window. We'll need its collection of free programs and
libraries.
-Install [gcc] and [libargp][argp] using MSYS2's `pacman` package
-manager:
+You can use the `pacman` package manager to install most of the tools
+needed to build Daffodil:
+
+ pacman -S clang diffutils git make pkgconf
Review comment:
Same question about diffutils here?
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/daffodil_main.c
##########
@@ -15,13 +15,13 @@
* limitations under the License.
*/
-#include <stdio.h> // for NULL, perror, FILE, fclose, fflush, fopen, stdin, stdout
-#include <string.h> // for strcmp
-#include "daffodil_argp.h" // for daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, daffodil_cli, parse_daffodil_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, Error, print_diagnostics, PState, UState, ERR_FILE_CLOSE, ERR_FILE_FLUSH, ERR_FILE_OPEN, ERR_INFOSET_READ, ERR_INFOSET_WRITE
-#include "infoset.h" // for walkInfoset, InfosetBase, rootElement, ERD, VisitEventHandler
-#include "xml_reader.h" // for xmlReaderMethods, XMLReader
-#include "xml_writer.h" // for xmlWriterMethods, XMLWriter
+#include <stdio.h> // for NULL, FILE, perror, fclose, fopen, stdin, stdout
+#include <string.h> // for strcmp
+#include "daffodil_getopt.h" // for daffodil_cli, parse_daffodil_cli, daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli, DAFFODIL_PARSE, DAFFODIL_UNPARSE
Review comment:
What about just removing those comments? If the includes are autogenerated, is the information about why they are added useful? Seems it's only useful if you don't trust the autogenerated includes and want to verify why it added lines, which doesn't seem that necessary.
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/examples/NestedUnion.c
##########
@@ -25,7 +25,7 @@
// Initialize our program's name and version
-const char *argp_program_version = "daffodil-runtime2 3.1.0-SNAPSHOT";
+const char *daffodil_program_version = "daffodil-runtime2 3.1.0-SNAPSHOT";
Review comment:
Is this hardcoded version going to cause tests to fail when we update the version?
##########
File path: daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/errors.c
##########
@@ -17,34 +17,109 @@
#include "errors.h"
#include <assert.h> // for assert
-#include <error.h> // for error
#include <inttypes.h> // for PRId64
#include <stdbool.h> // for bool, false, true
-#include <stdio.h> // for NULL, feof, ferror, FILE, size_t
-#include <stdlib.h> // for EXIT_FAILURE
+#include <stdio.h> // for fprintf, stderr, NULL, feof, ferror, FILE, size_t, stdout
+#include <stdlib.h> // for exit, EXIT_FAILURE
-// error_message - return an internationalized error message
+// eof_or_error - get pointer to error if stream has eof or error indicator set
+
+const Error *
+eof_or_error(FILE *stream)
+{
+ if (feof(stream))
+ {
+ static Error error = {ERR_STREAM_EOF, {NULL}};
+ return &error;
+ }
+ else if (ferror(stream))
+ {
+ static Error error = {ERR_STREAM_ERROR, {NULL}};
+ return &error;
+ }
+ else
+ {
+ return NULL;
+ }
+}
+
+// get_diagnostics - get pointer to validation diagnostics
+
+Diagnostics *
+get_diagnostics(void)
+{
+ static Diagnostics diagnostics;
+ return &diagnostics;
+}
+
+// add_diagnostic - add a new error to validation diagnostics
+
+bool
+add_diagnostic(Diagnostics *diagnostics, const Error *error)
+{
+ if (diagnostics && error)
+ {
+ if (diagnostics->length < LIMIT_DIAGNOSTICS)
+ {
+ Error *err = &diagnostics->array[diagnostics->length++];
+ err->code = error->code;
+ err->s = error->s;
+ return true;
+ }
+ }
+ return false;
+}
+
+// error_message - get an internationalized error message
static const char *
error_message(enum ErrorCode code)
Review comment:
To me, feels like anything CLI related should not be in libruntime. The CLI is just a user of libruntime, and just like any other runtime user, they shouldn't need to update the runtime to change user specific error messages. Almost feels like the CLI needs a completely separate error path, or libruntime needs someway for users to inject addition error capabilities.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on pull request #545: Fix some runtime2 bugs
Posted by GitBox <gi...@apache.org>.
tuxji commented on pull request #545:
URL: https://github.com/apache/daffodil/pull/545#issuecomment-830373479
I've looked at what would happen if we split error codes between libruntime and libcli. It turns out that most of the existing error codes had been used only in libcli too. We would end up with this split between libcli and libruntime:
```c
// cli_errors.h - libcli
enum CliCode
{
CLI_FILE_CLOSE,
CLI_FILE_OPEN,
CLI_HELP_USAGE,
CLI_INVALID_COMMAND,
CLI_INVALID_INFOSET,
CLI_INVALID_OPTION,
CLI_MISSING_COMMAND,
CLI_MISSING_VALUE,
CLI_PROGRAM_ERROR,
CLI_PROGRAM_VERSION,
CLI_STACK_EMPTY,
CLI_STACK_OVERFLOW,
CLI_STACK_UNDERFLOW,
CLI_STRTOBOOL,
CLI_STRTOD_ERRNO,
CLI_STRTOI_ERRNO,
CLI_STRTONUM_EMPTY,
CLI_STRTONUM_NOT,
CLI_STRTONUM_RANGE,
CLI_UNEXPECTED_ARGUMENT,
CLI_XML_DECL,
CLI_XML_ELEMENT,
CLI_XML_ERD,
CLI_XML_GONE,
CLI_XML_INPUT,
CLI_XML_LEFT,
CLI_XML_MISMATCH,
CLI_XML_WRITE,
};
// errors.h - libruntime
enum ErrorCode
{
ERR_CHOICE_KEY,
ERR_FIXED_VALUE,
ERR_PARSE_BOOL,
ERR_STREAM_EOF,
ERR_STREAM_ERROR,
};
```
@mbeckerle and @stevedlawrence , do you still want the codes and messages to be split up? I would end up duplicating enum CliCode and enum Error, struct CliError and struct Error, and function cli_continue_or_exit and continue_or_exit along with a couple of static functions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org