RFR: 7903666: Jextract should report issues with missing dependencies

[Jorn Vernee]

On Fri, 16 Feb 2024 15:05:49 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> When custom include filters are specified on the command line, it is possible for jextract to generate non-sensical code (now even more so than in the past). Possible issues:
> 
> * a function descriptor refers to a non-existent struct/union
> * a struct/union typedef refers to a non-existent struct/union
> * a struct/union field refers to a non-existent struct/union
> * a global variable refers to a non-existent struct/union
> 
> In such cases, jextract should terminate (and report the problem to the user), rather than generating buggy code.
> 
> The fix is relatively simple, and amounts at checking whether a declaration depends on a skipped struct in `IncludeHelper`. That said, when we detect an issue, we should stop jextract. So we need some way to notify jextract that some error occurred, and stop.
> 
> I initially had a hacky approach with some mutable field in `IncludeFilter`, but I've decided to implement a minimal Logger class, and make _all_ error/warnings go through the Logger. This way we can easily check how many errors occurred, and stop jextract if needed. Of course, this led to several changes, as I realized that we were being inconsistent on our usage of the property file. I now made sure that all messages use some key in the property file (e.g. no lone strings). This might be important if jextract message will ever be translated to languages other than English.

Marked as reviewed by jvernee (Committer).

src/main/java/org/openjdk/jextract/JextractTool.java line 133:

> 131:         return logger.hasErrors() ?
> 132:                 List.of() :
> 133:                 List.of(OutputFactory.generateWrapped(transformedDecl, targetPkg, libs, useSystemLoadLibrary));

So, we now don't generate anything when there are errors.

I think we should also return a non-zero exit code in that case.

test/testng/org/openjdk/jextract/test/toolprovider/includeDeps/TestBadIncludes.java line 47:

> 45:                 "--include-function", "n",
> 46:                 outputH.toString());
> 47:     }

Maybe `result.checkFailure()` here too?

-------------

PR Review: https://git.openjdk.org/jextract/pull/214#pullrequestreview-1885456106
PR Review Comment: https://git.openjdk.org/jextract/pull/214#discussion_r1492661540
PR Review Comment: https://git.openjdk.org/jextract/pull/214#discussion_r1492676369