validator / htmlparser

The Validator.nu HTML parser https://about.validator.nu/htmlparser/
Other
56 stars 26 forks source link

Fixes for Maven POM, and a modularization patch #23

Closed carlosame closed 3 years ago

carlosame commented 4 years ago

Fixes: 7b05376c

carlosame commented 4 years ago

My validator-nu branch contains several patches related to the POM file, and finally a patch for modularization. The first patch fixes a broken build and should be taken for 2.0 regardless of the other ones.

EDIT: the first patch fixes a broken build for Java 8. Given that the last patch requires JDK 12 or higher, it is not necessary (but does not hurt either).

carlosame commented 4 years ago

One point that I wanted to make: this set of patches includes configuration to build a source and javadoc artifacts, while the previous POM asked to specify that as a command line option (the POM was not set up for that). However, due to a bug in the javadoc of recent JDKs that command line would fail (unless one puts a load of configuration options in the command line), and this is the reason for the change.

The section for building the source artifact could be removed though.

anthonyvdotbe commented 4 years ago

There's several issues with the module descriptor:

carlosame commented 4 years ago
* it exports all packages

I did not like exporting a package called impl either, but from looking at it I cannot guarantee that nobody is using it. If @hsivonen believes that some package should not be exported, in August or September we should have plenty of time to discuss that.

* it forces every user of the module to provide all of `nu.xom`, `com.ibm.icu` and `jchardet` as well, even if it doesn't actually use any of them

Concerning ICU4J and jchardet, they could be declared static (if that is what you mean) but I do not know whether they could be instantiated as a result of a parsing decision (as opposed to a specific usage by the developer). XOM objects are part of the public API, but could be left static as well.

* `jchardet` shouldn't be required at all

It is required but could be changed to a static requirement.

carlosame commented 4 years ago

I changed nu.xom, com.ibm.icu and jchardet to static dependencies, we can always switch back.

sideshowbarker commented 4 years ago

I added 651e417 — because without it, I get a few dozen errors about javadoc problems:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.0:jar (attach-javadocs) on project htmlparser: MavenReportException: Error while generating Javadoc:
[ERROR] Exit code: 1 - /Users/mike/workspace/validator/htmlparser/target/src/nu/validator/htmlparser/common/ByteReadable.java:41: warning: no description for @throws
[ERROR]      * @throws IOException
[ERROR]        ^
[ERROR] /Users/mike/workspace/validator/htmlparser/target/src/nu/validator/htmlparser/common/TokenHandler.java:182: warning: no @throws for org.xml.sax.SAXException
[ERROR]     public void ensureBufferSpace(int inputLength) throws SAXException;
[ERROR]                 ^
…
sideshowbarker commented 4 years ago

When running mvn package in this branch, I’m getting this non-fatal error:

[INFO] --- maven-javadoc-plugin:3.1.0:jar (attach-javadocs) @ htmlparser ---
[ERROR] Error fetching link: /Users/mike/workspace/validator/htmlparser/target/javadoc-bundle-options. Ignored it.

Given that it’s non-fatal and only related to the javadoc build, I don’t think we need to care much about it — but it’d be nice to get rid of that error if we can.

Any clues about what might be causing it?

carlosame commented 4 years ago

Any clues about what might be causing it?

It is a known bug with a few variants being reported, like:

https://issues.apache.org/jira/browse/MJAVADOC-623

In recent years, the production of javadocs with Maven -especially with modular JDKs- is a mess of bugs, with newer versions of maven-javadoc-plugin fixing things but at the same time breaking other stuff (for example some fix breaks modular projects, and then the fixed version breaks usage with Java 8, etc.)

We can be happy that with plugin version 3.1.0 the jar is at least built (it fails with 3.2.0 and the same happens in other projects).