-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initialize RelaxNG support. #841
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a closer look as soon as I find time. One thing that struck me right away: You used the same maven dependency as I did in my PR. However, since then I found a more recent one:
<dependency>
<groupId>org.relaxng</groupId>
<artifactId>jing</artifactId>
<version>20181222</version>
</dependency>
I don't think there has been really any development on Jing between 2009 and 2018, but it might stil be better to use the more current one.
b6dfb97
to
5701262
Compare
It would be an immense help to have this. Is there anything preventing it from being merged? |
@adunning thanks for your feedback. At first please wote at redhat-developer/vscode-xml#450 The main problem is that RelaxNG is not our priority and it requires a lot of work (to support validation with error range and completion,hover, definition based on relaxng). Our current goal is to support robust support for XML, XSD and DTD.But if we see that we have more and more people(liek you) who are interested we could work. This PR is based on Jing, but I know @svanteschubert wanted to provide a relaxng support based on MSV. See redhat-developer/vscode-xml#451 (comment) @svanteschubert what is the status of your work? |
cac6f36
to
6d009b1
Compare
org.eclipse.lemminx/pom.xml
Outdated
<groupId>org.relaxng</groupId> | ||
<artifactId>jing</artifactId> | ||
<version>20220510</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking to @datho7561 , I noticed that https://repo1.maven.org/maven2/org/relaxng/jing/20220510/jing-20220510.pom depends on :
xerces:xercesImpl:2.9.1
& xml-apis:xml-apis:1.0.b2
while lemminx itself depends on :
xerces:xercesImpl:2.12.2
& xml-apis:xml-apis:1.4.01
We can't have multiple versions in the uber jar so would be good to ensure the latest versions are also supported by jing, and maybe contribute back upstream.
...eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/xerces/RelaxNGErrorHandler.java
Outdated
Show resolved
Hide resolved
We will need to do a bit of work in order to get the binary to work |
For
So these libraries would need to be approved via. https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab . |
e689512
to
27b771c
Compare
6c81de3
to
9c5ba42
Compare
Here's what's needed to get the binary working: diff --git a/org.eclipse.lemminx/src/main/resources/META-INF/native-image/resource-config.json b/org.eclipse.lemminx/src/main/resources/META-INF/native-image/resource-config.json
index 12a4fcdb..18d3640d 100644
--- a/org.eclipse.lemminx/src/main/resources/META-INF/native-image/resource-config.json
+++ b/org.eclipse.lemminx/src/main/resources/META-INF/native-image/resource-config.json
@@ -50,6 +50,12 @@
{
"name": "org.apache.xerces.impl.msg.XMLMessages"
},
+ {
+ "name": "com.thaiopensource.datatype.xsd.resources.Messages"
+ },
+ {
+ "name": "com.thaiopensource.relaxng.pattern.resources.Messages"
+ },
{
"name": "org.eclipse.lemminx.extensions.xerces.xmlmodel.msg.XMLMessages"
}, |
Thanksso much for your feedback. I'm surprised that there is just this fix to do. I think completion doesn't work with binary because I'm using Java reflection (for the moment). |
a9c8e67
to
01d1ef5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got an NPE. Not sure what caused it, but I'll try to figure out steps to reproduce it:
java.lang.NullPointerException
at org.eclipse.lemminx.extensions.contentmodel.participants.diagnostics.LSPSAXParser.startDocument(LSPSAXParser.java:87)
at org.eclipse.lemminx.extensions.relaxng.xml.validator.ExternalRelaxNGValidator.startDocument(ExternalRelaxNGValidator.java:68)
at org.apache.xerces.impl.dtd.XMLDTDValidator.startDocument(Unknown Source)
at org.eclipse.lemminx.extensions.xerces.ExternalXMLDTDValidator.startDocument(ExternalXMLDTDValidator.java:73)
at org.eclipse.lemminx.extensions.xerces.xmlmodel.XMLModelHandler.startDocument(XMLModelHandler.java:170)
at org.apache.xerces.impl.dtd.XMLDTDValidator.startDocument(Unknown Source)
at org.apache.xerces.impl.XMLDocumentScannerImpl.startEntity(Unknown Source)
at org.apache.xerces.impl.XMLVersionDetector.startDocumentParsing(Unknown Source)
at org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
at org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
at org.apache.xerces.parsers.XMLParser.parse(Unknown Source)
at org.apache.xerces.parsers.AbstractSAXParser.parse(Unknown Source)
at org.eclipse.lemminx.extensions.contentmodel.participants.diagnostics.XMLValidator.parseXML(XMLValidator.java:318)
at org.eclipse.lemminx.extensions.contentmodel.participants.diagnostics.XMLValidator.doDiagnostics(XMLValidator.java:132)
at org.eclipse.lemminx.extensions.contentmodel.participants.diagnostics.ContentModelDiagnosticsParticipant.doDiagnostics(ContentModelDiagnosticsParticipant.java:58)
at org.eclipse.lemminx.services.XMLDiagnostics.doExtensionsDiagnostics(XMLDiagnostics.java:67)
at org.eclipse.lemminx.services.XMLDiagnostics.doDiagnostics(XMLDiagnostics.java:49)
at org.eclipse.lemminx.services.XMLLanguageService.doDiagnostics(XMLLanguageService.java:173)
at org.eclipse.lemminx.services.XMLLanguageService.publishDiagnostics(XMLLanguageService.java:187)
at org.eclipse.lemminx.XMLTextDocumentService.validate(XMLTextDocumentService.java:662)
at org.eclipse.lemminx.XMLTextDocumentService.lambda$validate$32(XMLTextDocumentService.java:637)
at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804)
at java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1796)
at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)
at com.oracle.svm.core.thread.PlatformThreads.threadStartRoutine(PlatformThreads.java:704)
at com.oracle.svm.core.posix.thread.PosixPlatformThreads.pthreadStartRoutine(PosixPlatformThreads.java:202)
org.eclipse.lemminx/src/main/resources/META-INF/native-image/resource-config.json
Outdated
Show resolved
Hide resolved
Given: <?xml version="1.0" encoding="UTF-8"?>
<?xml-model href="asdf.rnc"?>
<addressBook>
<card>
<name>David</name>
<email></email>
</card>
</addressBook> and an empty |
If you reference an empty |
It would be nice to associate all |
...eclipse/lemminx/extensions/contentmodel/participants/diagnostics/MultipleContentHandler.java
Show resolved
Hide resolved
...minx/src/main/java/org/eclipse/lemminx/extensions/relaxng/jing/toremove/MyCombineSchema.java
Outdated
Show resolved
Hide resolved
...minx/src/main/java/org/eclipse/lemminx/extensions/relaxng/jing/toremove/MyPatternSchema.java
Outdated
Show resolved
Hide resolved
...main/java/org/eclipse/lemminx/extensions/relaxng/xml/validator/ExternalRelaxNGValidator.java
Show resolved
Hide resolved
* @author Angelo ZERR | ||
* | ||
*/ | ||
public class RelaxNGErrorHandler implements XMLErrorHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the SAX interface not enough to get good diagnostics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not enough because we need the error code key.
|
||
static { | ||
MODEL_VALIDATOR_FACTORIES = new ArrayList<>(); | ||
Iterator<XMLModelValidatorFactory> factories = ServiceLoader.load(XMLModelValidatorFactory.class).iterator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some places, the different validators are loaded dynamically, but in others, like LSPErrorReporterForXML
, they are hard coded. I think we should eventually get a clean separation between the different validators. This would let us do things like add a new validator that uses the lemminx grammar cache using the extension point mechanism, or compile a version of LemMinX with only specific validators in order to reduce the size.
However, I think maybe it makes more sense to do this in a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think maybe it makes more sense to do this in a future PR?
I agree it is not perfect but XSD and DTD validator does that too. Please do that in an another PR.
It should be fixed now and you should see the error as referenced diagnostic because the error comes from the schema and not from the XML: |
cc4ee18
to
3ebe288
Compare
Don't say "This class is a copy paste of", say "This class is a copy of " |
fcc2f3c
to
a6ac347
Compare
fixed |
If you reference an empty |
* XML validation with RelaxNG * XML completion based on RelaxNG * Find Type definition from XML to RelaxNG * RelaxNG snippet support for XML and RelaxNG *.rng files Signed-off-by: azerr <azerr@redhat.com>
It should be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works very well. Thank you, Angelo!
Initialize RelaxNG support (XML and compact syntax) with: