kriegaex commented on code in PR #129:
URL: https://github.com/apache/xalan-java/pull/129#discussion_r1405285898
##########
serializer/src/main/java/org/apache/xml/serializer/Version.java:
##########
@@ -55,7 +55,16 @@ public final class Version
private static void readProperties() {
Properties pomProperties = new Properties();
- try (InputStream fromResource =
Version.class.getClassLoader().getResourceAsStream(POM_PROPERTIES_PATH)) {
+ ClassLoader classLoader = Version.class.getClassLoader();
+ if (classLoader == null) {
+ // Oops! Someone put Xalan is on the bootstrap class loader (BCL) -> fall
+ // back to the system class loader, because there is no Classloader
+ // instance for the BCL (native code). Due to class loader hierarchy,
+ // however, the resource will also be found when asking for it from a
+ // level below the BCL.
+ classLoader = ClassLoader.getSystemClassLoader();
+ }
+ try (InputStream fromResource =
classLoader.getResourceAsStream(POM_PROPERTIES_PATH)) {
Review Comment:
> This is all wrong
Another judgemental and and utterly unhelpful comment. Snap out of it,
please, dear @vlsi. If something is wrong in your opinion, please explain why
and show how it is done right.
> Have you considered `Version.class.getResourceAsStream(...)`?
Not just considered, but also tried before I even switched to the
`getClassLoader()` version. It failed, just like Joe said. Anyway, at least one
thing you achieved with your unhelpful comment: I got curious why it had not
worked, why the heck both `Class` and `Classloader` have the same method and
what the difference between them is. Javadoc is pretty much useless, not
explaining the difference in any meaningful way. But
[Baeldung](https://www.baeldung.com/java-class-vs-classloader-getresource)
does: `Class.getResourceAsStream` uses a *relative* path by default, in
contrast to `Classloader.getResourceAsStream`, which uses an *absolute* one.
This information enabled me to follow your "advice" and use
`Version.class.getResourceAsStream("version.properties")`. It does exactly the
same as my original solution, but does not require a null check and second try
and even enabled me to inline the file name. As a bonus, it is also still
relocation-friendly.
@jkesselm, I tested the new commit with normal and boot classpath and also
with Maven Shade relocation. LGTM.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org
For additional commands, e-mail: dev-h...@xalan.apache.org
