ppkarwasz commented on code in PR #2404:
URL: https://github.com/apache/logging-log4j2/pull/2404#discussion_r1537394035
##########
log4j-core-java9/pom.xml:
##########
@@ -30,6 +30,8 @@
<properties>
<maven.compiler.release>9</maven.compiler.release>
<maven.deploy.skip>true</maven.deploy.skip>
+ <maven.install.skip>true</maven.install.skip>
Review Comment:
The `log4j-core-java9` artifact was intentionally "installable", so that we
only need to run:
```sh
./mvnw install -pl log4j-core-java9
```
once per release number and we can successfully run:
```sh
./mvnw verify -pl log4j-core
```
after that.
That being said I wouldn't mind, if we reintegrate the classes from
`log4j-core-java9` back into `log4j-core/src/main/java9`.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/xml/XmlSchemaTest.java:
##########
@@ -1,103 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to you under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.logging.log4j.core.config.xml;
-
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
-
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.Paths;
-import java.util.Arrays;
-import java.util.List;
-import java.util.stream.Stream;
-import javax.xml.XMLConstants;
-import javax.xml.transform.sax.SAXSource;
-import javax.xml.transform.stream.StreamSource;
-import javax.xml.validation.Schema;
-import javax.xml.validation.SchemaFactory;
-import javax.xml.validation.Validator;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.MethodSource;
-import org.xml.sax.Attributes;
-import org.xml.sax.InputSource;
-import org.xml.sax.SAXException;
-import org.xml.sax.SAXParseException;
-import org.xml.sax.helpers.XMLFilterImpl;
-import org.xml.sax.helpers.XMLReaderFactory;
-
-public class XmlSchemaTest {
Review Comment:
Can you add a "feature request" issue to validate the configuration files in
`log4j-core-test` using the generated schema?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java:
##########
@@ -142,7 +141,7 @@ public XmlConfiguration(final LoggerContext loggerContext,
final ConfigurationSo
try (final InputStream is =
Loader.getResourceAsStream(schemaResource,
XmlConfiguration.class.getClassLoader())) {
if (is != null) {
- final javax.xml.transform.Source src = new
StreamSource(is, LOG4J_XSD);
+ final javax.xml.transform.Source src = new
StreamSource(is);
Review Comment:
```suggestion
final javax.xml.transform.Source src = new
StreamSource(is, schemaResource);
```
The `systemId` parameter is very useful for debugging (it is included in the
exception messages).
**Remark**: this code is only executed when using the [strict XML
configuration
mode](https://logging.apache.org/log4j/2.x/manual/configuration.html#strict-xml)
that you PR makes completely obsolete. The XSD that you generate allows users
to validate a "concise XML config" and does not have any support for the
"strict XML config".
##########
log4j-core/pom.xml:
##########
@@ -222,40 +233,61 @@
</executions>
</plugin>
+ <!-- We can't use `plugin-processing` Maven profile activated by
`.log4j-plugin-processing-activator` file, because...
+ `log4j-core` contains `PluginProcessor`, that is responsible for
generating `META-INF/org/apache/.../Log4j2Plugins.dat` from `@Plugin`-annotated
members.
+ `log4j-core` also contains `@Plugin`-annotated members too.
+ That is, we need `log4j-core` to build `log4j-core`.
+ To work around this chicken-and-egg problem, we build as follows:
+
+ 1. Compile sources using the `default-compile` default compilation
execution.
+ This will generate the `PluginProcessor` class.
+
+ 2. Make a second compilation pass using the generated
`PluginProcessor`. -->
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
- <configuration>
- <annotationProcessorPaths combine.self="override" />
- </configuration>
<executions>
+
+ <!-- Compile sources as usual -->
<execution>
- <!-- disable annotation processing for first pass -->
<id>default-compile</id>
<configuration>
- <annotationProcessorPaths>
+ <annotationProcessorPaths combine.children="append">
+ <!--
`org.apache.logging.log4j.docgen.processor.DescriptorGenerator` for generating
`log4j-plugins.xml`: -->
<path>
- <groupId>com.google.errorprone</groupId>
- <artifactId>error_prone_core</artifactId>
- <version>${error-prone.version}</version>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-docgen</artifactId>
+ <version>${log4j-docgen.version}</version>
</path>
</annotationProcessorPaths>
Review Comment:
It makes more sense to me to use the `process-plugins` execution for both
the `PluginProcessor` and `DescriptorGenerator` processors.
--
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: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
