4.32-I-build warning logs not accessible anymore since I20240321-1050

[HannesWell]

Since I-build I20240321-1050 the details about the

• Plugins containing compile errors or warnings
• Plugins containing access errors or warnings

listed at the test-result overview page cannot be accessed anymore:
https://download.eclipse.org/eclipse/downloads/drops4/I20240327-0720/compilelogs/plugins/org.eclipse.ant.core_3.7.400.v20240321-1303/@dot.html#OTHER_WARNINGS

But looking at its parent directory, there is a @dot.xml:
https://download.eclipse.org/eclipse/downloads/drops4/I20240327-0720/compilelogs/plugins/org.eclipse.ant.core_3.7.400.v20240321-1303/

The last build for which this worked seems to be I20240318-1800, see for example:
https://download.eclipse.org/eclipse/downloads/drops4/I20240318-1800/compilelogs/plugins/org.eclipse.ant.core_3.7.300.v20231214-1526/@dot.html#OTHER_WARNINGS

The the html seems not to be not generated anymore.
@laeubi could this be connected to the latest changes about compiler-logs?

[laeubi]

I fixed a NPE here, maybe more things lurking around:

• Add check for null before dereference id node eclipse.platform.releng.buildtools#59

[HannesWell]

https://download.eclipse.org/eclipse/downloads/drops4/I20240327-0720/compilelogs/plugins/org.eclipse.ant.core_3.7.400.v20240321-1303/@dot.html#OTHER_WARNINGS

But looking at its parent directory, there is a @dot.xml: https://download.eclipse.org/eclipse/downloads/drops4/I20240327-0720/compilelogs/plugins/org.eclipse.ant.core_3.7.400.v20240321-1303/

To me it looks like the cause is more that the link expect a @dot.html but there is only a @dot.xml?

[akurtakov]

It's visible in https://download.eclipse.org/eclipse/downloads/drops4/I20240401-1800/buildlogs/mb300_gatherEclipseParts.sh.log :

[eclipse.convert] Element type "compiler" must be declared.
[eclipse.convert] org.xml.sax.SAXParseException; systemId: file:///home/jenkins/agent/workspace/Builds/I-build-4.32/eclipse.platform.releng.aggregator/eclipse.platform.releng.aggregator/cje-production/siteDir/eclipse/downloads/drops4/I20240401-1800/compilelogs/plugins/org.eclipse.ant.core_3.7.400.v20240321-1303/lib_antsupportlib.jar.xml; lineNumber: 4; columnNumber: 145; Element type "compiler" must be declared.
[eclipse.convert] Error in /home/jenkins/agent/workspace/Builds/I-build-4.32/eclipse.platform.releng.aggregator/eclipse.platform.releng.aggregator/cje-production/siteDir/eclipse/downloads/drops4/I20240401-1800/compilelogs/plugins/org.eclipse.ant.launching_1.4.400.v20240321-1303/@dot.xml at line 4 and column 145

and many similar.

[akurtakov]

It's been broken by eclipse-platform/eclipse.platform.releng.buildtools#45 but it hasn't been visible as the new version hasn't been published and used when this change went in.

[akurtakov]

@jukzi What is the plan to fix here?

[akurtakov]

Local test can be done using : org.eclipse.releng.build.tools.convert.ant.Converter -v -r -i /path/to/dir/with/dot/xml/ . In case of success dot.html class should appear in the dir, in case of failure problem will be printed on the console.

[akurtakov]

So commenting https://github.com/eclipse-platform/eclipse.platform.releng.buildtools/blob/bd8c70a3d4044a7bd1ff7d11ab826a28387d4c46/bundles/org.eclipse.releng.build.tools.convert/src/org/eclipse/releng/build/tools/convert/ant/Converter.java#L211 fixes the issue. As such an entityresolver is practically disables any resolving setValidation(true) is guaranteed to break as dtd is specified. So I guess this line has to be simply removed and let the JVM defaults do its work.

[HannesWell]

So I guess this line has to be simply removed and let the JVM defaults do its work.

Thanks for finding these. Can you tell if this converted is used by anything else than to gather the test-results/warnings?
If not I think we can trust the files the I-builds produces enough to disable it.

[akurtakov]

So I guess this line has to be simply removed and let the JVM defaults do its work.

Thanks for finding these. Can you tell if this converted is used by anything else than to gather the test-results/warnings? If not I think we can trust the files the I-builds produces enough to disable it.

It's not used anywhere else AFAICT. In an ideal world this "converter" would become an xslt template and the need for this parsing and etc. would be gone.

[laeubi]

Just wondering why any DTDs are required anyways to parse the xml, maybe just disable the validation of the parser altogether (setValidation(false))?

[akurtakov]

I am not the one that created the app but I don't see a benefit from skipping validation for files that have dtd. If validation is to be disabled I would expect the code to be cleaned from all things related to https://github.com/eclipse-platform/eclipse.platform.releng.buildtools/blob/bd8c70a3d4044a7bd1ff7d11ab826a28387d4c46/bundles/org.eclipse.releng.build.tools.convert/src/org/eclipse/releng/build/tools/convert/ant/Converter.java#L147

[laeubi]

I am not the one that created the app but I don't see a benefit from skipping validation for files that have dtd

The benefit would be that we don't need the DTD (neither local nor remote) and I think for this usecase we can assume the xmls is valid (as one can see the validation error is also only discovered by accident ... so to save us some hassle I would just disable validation here and handle this like plain XML that has some known structure.

[akurtakov]

I would proceed with partial revert of the previous patch as dot.xml files are produced in eclipse.org infra and conversion runs there so there is no security concern from fetching the dtd. If someone managed to breach to that level we would have far bigger concerns than this one.

[akurtakov]

Built and deployed. I'm starting new I-build now to confirm the fix today.

[akurtakov]

https://download.eclipse.org/eclipse/downloads/drops4/I20240403-0940/compilelogs/plugins/org.eclipse.core.commands_3.12.100.v20240322-0723/@dot.html#OTHER_WARNINGS points to working html page now so it's confirmed fixed.

[HannesWell]

Great, thank you!

Now we can fix all those warnings😃

[jukzi]

The dot.xml contain
<!DOCTYPE compiler PUBLIC "-//Eclipse.org//DTD Eclipse JDT 3.2.006 Compiler//EN" "https://www.eclipse.org/jdt/core/compiler_32_006.dtd"> taken from org.eclipse.jdt.internal.compiler.batch.Main.Logger.XML_DTD_DECLARATION

I would proceed with partial revert of the previous patch as dot.xml files are produced in eclipse.org infra and conversion runs there so there is no security concern from fetching the dtd

Security holes may start with wrong assumptions. It would be better to at least check that the URL refers to the trusted "https://www.eclipse.org/" domain instead of arbitrary URLs. Even if we don't know how other URL could be foisted such a simple check on EntityResolver's systemId would not hurt.

[laeubi]

Can't we simply embeds the DTD somehow or read it from the bundle instead? Network I/O is not nice anyways...

[jukzi]

Effectively the URL leads to a pinned version of org.eclipse.jdt.core\schema\compiler.dtd however it it is not obvious to which revision, so copying it or including a slightly wrong JDT version could result in issues.

[laeubi]

Thats why I mentioned we maybe can disable validation altogether, as it theoretically can cause issues, practically this almost never changes, there was one change 8 years ago to support an info category:

https://github.com/eclipse-jdt/eclipse.jdt.core/commits/master/org.eclipse.jdt.core/schema/compiler.dtd

the change before that is 14 years ago... so this might never even practically impact the usecase here...

[akurtakov]

Guys, I'm all for better tool as long as there is someone to drive these changes. I've just stepped up to take the reports back with the very minimal time I can afford for that.

[jukzi]

I could sponsor a PR for a minimal URL check if we agree on that.

[merks]

Maybe a stupid question (because I didn't review all the context) but could one use a platform:/plugin/org.eclipse.jdt.core/schema/compiler.dtd to access the DTD directly from the bundle?