Re: SLF4J instead of Log4J at the API level? [EXTERNAL]
Hi Richard,
Many, many thanks for doing this. I gave it a pass and don't see anything that
looks like it might cause a problem. I will run some tests. My comments on
your comments:
- The lifecycle plugin in the root POM uses "version" instead of "versionRange"
in the main branch - the PR fixes that
-- That sounds good to me.
- The main branch still uses ContextSingletonBeanFactoryLocator which is not
part of Spring anymore and thus the Eclipse has compiler errors
-- I can look for an appropriate replacement.
- Many calls to the logging use String.format or similar means to construct the
logged string. This can be done better using the SLF4J API,
e.g. `debug("{} is less than {}", x, y)` or `error("This is an error, ex)`. I
did not upgrade all these calls.
-- Though I like to be a boy scout (and I noticed your if/then braces!), I
don't expect anybody to make this code minty-fresh. It is nice that slf4j has
this capability.
- I commented out some calls to directly configure the logging system - instead
there should be `log4j2-test.xml` files or similar configuration files for
slf4j-simple used for testing.
-- I couldn't find these in the modified files. Do you recall where they were
- or for that matter, why they were?
- Calls to SLF4J in UIMA components should be replaced by `getLogger().` -
I did this in some cases
-- Here I am ignorant. What is the advantage, and do you think that it is
important enough to warrant a global find/replace? If so then I can give it a
shot.
- It would IMHO be a good idea to introduce the maven-dependency-plugin to
ensure that all packages used have direct non-transitive Maven dependencies and
there are no unused dependencies
-- Are you talking about the analyze goal? I suppose that we could add it to
a phase for automatic execution.
Cheers,
Sean
From: Richard Eckart de Castilho
Sent: Friday, July 26, 2024 8:36 PM
To: dev@ctakes.apache.org
Subject: Re: SLF4J instead of Log4J at the API level? [EXTERNAL]
* External Email - Caution *
Hi Sean,
i have set up an initial PR here:
https://urldefense.com/v3/__https://github.com/apache/ctakes/pull/21__;!!NZvER7FxgEiBAiR_!rOgnJ7H15Yz2Yhom0R9PmwVVqB8yO-_DzBI5-d-yDg0laTqSxcT1vW91vTLO1RYQk42IoHdn3V3MbcEQHKLmJw$
However, there are a few issues:
- The lifecycle plugin in the root POM uses "version" instead of "versionRange"
in the main branch - the PR fixes that
- The main branch still uses ContextSingletonBeanFactoryLocator which is not
part of Spring anymore and thus the Eclipse has compiler errors
- Many calls to the logging use String.format or similar means to construct the
logged string. This can be done better using the SLF4J API,
e.g. `debug("{} is less than {}", x, y)` or `error("This is an error, ex)`. I
did not upgrade all these calls.
- I commented out some calls to directly configure the logging system - instead
there should be `log4j2-test.xml` files or similar
configuration files for slf4j-simple used for testing.
- Calls to SLF4J in UIMA components should be replaced by `getLogger().` -
I did this in some cases
- It would IMHO be a good idea to introduce the maven-dependency-plugin to
ensure that all packages used have direct non-transitive
Maven dependencies and there are no unused dependencies
Cheers,
-- Richard
Re: Build cTAKES using GitHub action? [EXTERNAL]
> Btw. how about setting up a GitHub action for building cTAKES? I thought about that when we switched over from svn. I didn't do it myself because I am happy with Jenkins. It also seems like quite the beast to romp around in a GH action. I suppose that we could have build profiles that only compile the basic modules that are in the default clinical pipeline and things like that. Even without that, I don't think that we'd have problems with the infra rules https://infra.apache.org/github-actions-policy.html I don't place that as a high priority, but if anybody else does then I say "go for it". https://infra.apache.org/github-actions-secrets.html Sean From: Richard Eckart de Castilho Sent: Friday, July 26, 2024 8:45 PM To: dev@ctakes.apache.org Subject: Build cTAKES using GitHub action? [EXTERNAL] * External Email - Caution * Btw. how about setting up a GitHub action for building cTAKES? -- Richard
Re: Build cTAKES using GitHub action?
> On 29. Jul 2024, at 12:14, Finan, Sean > wrote: > >> Btw. how about setting up a GitHub action for building cTAKES? > > I thought about that when we switched over from svn. I didn't do it myself > because I am happy with Jenkins. Right, I honestly forgot that there's Jenkins. I guess that's because I didn't see Jenkins badges on the PRs: * https://github.com/apache/ctakes/pull/21 Cf. * https://github.com/apache/uima-uimaj/pull/370 I can see you're still using "traditional" UI-configured Jenkins jobs instead of Jenkins pipelines that are managed in the version control. Would you mind if I set up a pipeline build that can handle PRs for cTAKES by copying the approach I have taken in UIMA? See also: * https://ci-builds.apache.org/job/UIMA/job/uima-uimaj/ * https://ci-builds.apache.org/job/UIMA/configure * https://ci-builds.apache.org/job/UIMA/job/uima-uimaj/configure That approach is using a shared repo that contains the actual pipeline configuration which is used by all UIMA sub-projects. I'd probably temporarily use that for cTAKES as well but it should eventually be cloned to a separate repo for cTAKES. * https://github.com/apache/uima-build-jenkins-shared-library/tree/main Cheers, -- Richard
Re: SLF4J instead of Log4J at the API level? [EXTERNAL]
Hi Gandh, > On 27. Jul 2024, at 14:06, Gandhirajan N wrote: > > However, the versionRange in root POM could > introduce unpredictability in builds if newer versions introduce breaking > changes If that were a version range in a dependency: yes -- but this is a different case. However, this particular "version" tag was in the configuration of the lifecycle plugin that tells Eclipse if and when to execute certain goals. And in fact, "version" is not a valid parameter there while "versionRange" is. It tells Eclipse for which versions of a particular plugin the lifecycle configuration applies. And typically you want to have a generously broad version range there so you don't have to update the rules every time you update a Maven plugin. Cheers, -- Richard
Re: SLF4J instead of Log4J at the API level? [EXTERNAL]
Hi,
> On 29. Jul 2024, at 12:02, Finan, Sean
> wrote:
>
> - The main branch still uses ContextSingletonBeanFactoryLocator which is not
> part of Spring anymore and thus the Eclipse has compiler errors
> -- I can look for an appropriate replacement.
I briefly googled and I'm note sure if there is a proper direct replacement for
this. It looks like you're still using XML-based Spring configuration there.
Maybe using Spring Java-Config would be more sensible nowadays.
> - Many calls to the logging use String.format or similar means to construct
> the logged string. This can be done better using the SLF4J API,
> e.g. `debug("{} is less than {}", x, y)` or `error("This is an error, ex)`.
> I did not upgrade all these calls.
> -- Though I like to be a boy scout (and I noticed your if/then braces!), I
> don't expect anybody to make this code minty-fresh. It is nice that slf4j
> has this capability.
Maybe if I'm bored ;) Or maybe somebody else feels like it.
> - I commented out some calls to directly configure the logging system -
> instead there should be `log4j2-test.xml` files or similar configuration
> files for slf4j-simple used for testing.
> -- I couldn't find these in the modified files. Do you recall where they
> were - or for that matter, why they were?
For example in AssertionAnalysisEngine:
https://github.com/apache/ctakes/blob/837666cc2f8d25520255886e9525feeeb9510fbd/ctakes-assertion/src/main/java/org/apache/ctakes/assertion/medfacts/AssertionAnalysisEngine.java#L287
> - Calls to SLF4J in UIMA components should be replaced by `getLogger().`
> - I did this in some cases
> -- Here I am ignorant. What is the advantage, and do you think that it is
> important enough to warrant a global find/replace? If so then I can give it
> a shot.
Since UIMA already supplies a logging mechanism for the components, I would
tend to using it. Internally, that is by default also routed through SLF4J.
Theoretically, a user of the components might re-configure UIMA to route the
logging differently for their particular environment. You could also argue
that you prefer using the same logging API throughout the entire code. So while
I say "should", it is arguable and you might come to a different conclusion
than I do.
> - It would IMHO be a good idea to introduce the maven-dependency-plugin to
> ensure that all packages used have direct non-transitive Maven dependencies
> and there are no unused dependencies
> -- Are you talking about the analyze goal? I suppose that we could add it
> to a phase for automatic execution.
Yep, that one exactly.
-- Richard
Re: SLF4J instead of Log4J at the API level? [EXTERNAL]
> The main branch still uses ContextSingletonBeanFactoryLocator
...
It looks like I had made a replacement a while ago for other instances of this
problem, but had somehow missed those two tests. Consider it done.
> For example in AssertionAnalysisEngine:
Oh, those. From the days of yor. Garbage imo. I get rid of them whenever I
find them.
>should be replaced by `getLogger().`
Right now I want to get vulnerabilities remediated and then get a release out
with that focus. After that, some "play nice, refactor" release involving
things like using uima api getLogger() as well as code consolidation, generics
(how archaic are we?), modern collections and streams ...
Cheers,
Sean
From: Richard Eckart de Castilho
Sent: Monday, July 29, 2024 4:09 PM
To: dev@ctakes.apache.org
Subject: Re: SLF4J instead of Log4J at the API level? [EXTERNAL]
* External Email - Caution *
Hi,
> On 29. Jul 2024, at 12:02, Finan, Sean
> wrote:
>
> - The main branch still uses ContextSingletonBeanFactoryLocator which is not
> part of Spring anymore and thus the Eclipse has compiler errors
> -- I can look for an appropriate replacement.
I briefly googled and I'm note sure if there is a proper direct replacement for
this. It looks like you're still using XML-based Spring configuration there.
Maybe using Spring Java-Config would be more sensible nowadays.
> - Many calls to the logging use String.format or similar means to construct
> the logged string. This can be done better using the SLF4J API,
> e.g. `debug("{} is less than {}", x, y)` or `error("This is an error, ex)`.
> I did not upgrade all these calls.
> -- Though I like to be a boy scout (and I noticed your if/then braces!), I
> don't expect anybody to make this code minty-fresh. It is nice that slf4j
> has this capability.
Maybe if I'm bored ;) Or maybe somebody else feels like it.
> - I commented out some calls to directly configure the logging system -
> instead there should be `log4j2-test.xml` files or similar configuration
> files for slf4j-simple used for testing.
> -- I couldn't find these in the modified files. Do you recall where they
> were - or for that matter, why they were?
For example in AssertionAnalysisEngine:
https://urldefense.com/v3/__https://github.com/apache/ctakes/blob/837666cc2f8d25520255886e9525feeeb9510fbd/ctakes-assertion/src/main/java/org/apache/ctakes/assertion/medfacts/AssertionAnalysisEngine.java*L287__;Iw!!NZvER7FxgEiBAiR_!qx0-fe8dC60hk-itqClF2maIsPjLQh0tOznHEcnBrbH3lwJf_fWe9Q14Y5vuUljMFP4eAk2YyBH6oTe3Cw2edQ$
> - Calls to SLF4J in UIMA components should be replaced by `getLogger().`
> - I did this in some cases
> -- Here I am ignorant. What is the advantage, and do you think that it is
> important enough to warrant a global find/replace? If so then I can give it
> a shot.
Since UIMA already supplies a logging mechanism for the components, I would
tend to using it. Internally, that is by default also routed through SLF4J.
Theoretically, a user of the components might re-configure UIMA to route the
logging differently for their particular environment. You could also argue
that you prefer using the same logging API throughout the entire code. So while
I say "should", it is arguable and you might come to a different conclusion
than I do.
> - It would IMHO be a good idea to introduce the maven-dependency-plugin to
> ensure that all packages used have direct non-transitive Maven dependencies
> and there are no unused dependencies
> -- Are you talking about the analyze goal? I suppose that we could add it
> to a phase for automatic execution.
Yep, that one exactly.
-- Richard
Re: Build cTAKES using GitHub action? [EXTERNAL]
I am pretty sure that nobody would mind ... The great thing about version control is that what has been done can always be undone. From: Richard Eckart de Castilho Sent: Monday, July 29, 2024 3:38 PM To: dev@ctakes.apache.org Subject: Re: Build cTAKES using GitHub action? [EXTERNAL] * External Email - Caution * > On 29. Jul 2024, at 12:14, Finan, Sean > wrote: > >> Btw. how about setting up a GitHub action for building cTAKES? > > I thought about that when we switched over from svn. I didn't do it myself > because I am happy with Jenkins. Right, I honestly forgot that there's Jenkins. I guess that's because I didn't see Jenkins badges on the PRs: * https://urldefense.com/v3/__https://github.com/apache/ctakes/pull/21__;!!NZvER7FxgEiBAiR_!qsgb8wTFlVoBS9VU86JKrNyrbAQfAQd4MZIEkmpDyK3h10xw0SEz1iNheVt5Z6JaJB4XtWER6vxs-6jITDLAuw$ Cf. * https://urldefense.com/v3/__https://github.com/apache/uima-uimaj/pull/370__;!!NZvER7FxgEiBAiR_!qsgb8wTFlVoBS9VU86JKrNyrbAQfAQd4MZIEkmpDyK3h10xw0SEz1iNheVt5Z6JaJB4XtWER6vxs-6id7fIEbg$ I can see you're still using "traditional" UI-configured Jenkins jobs instead of Jenkins pipelines that are managed in the version control. Would you mind if I set up a pipeline build that can handle PRs for cTAKES by copying the approach I have taken in UIMA? See also: * https://urldefense.com/v3/__https://ci-builds.apache.org/job/UIMA/job/uima-uimaj/__;!!NZvER7FxgEiBAiR_!qsgb8wTFlVoBS9VU86JKrNyrbAQfAQd4MZIEkmpDyK3h10xw0SEz1iNheVt5Z6JaJB4XtWER6vxs-6i67BviWg$ * https://urldefense.com/v3/__https://ci-builds.apache.org/job/UIMA/configure__;!!NZvER7FxgEiBAiR_!qsgb8wTFlVoBS9VU86JKrNyrbAQfAQd4MZIEkmpDyK3h10xw0SEz1iNheVt5Z6JaJB4XtWER6vxs-6hzipOuGA$ * https://urldefense.com/v3/__https://ci-builds.apache.org/job/UIMA/job/uima-uimaj/configure__;!!NZvER7FxgEiBAiR_!qsgb8wTFlVoBS9VU86JKrNyrbAQfAQd4MZIEkmpDyK3h10xw0SEz1iNheVt5Z6JaJB4XtWER6vxs-6j88KBB1w$ That approach is using a shared repo that contains the actual pipeline configuration which is used by all UIMA sub-projects. I'd probably temporarily use that for cTAKES as well but it should eventually be cloned to a separate repo for cTAKES. * https://urldefense.com/v3/__https://github.com/apache/uima-build-jenkins-shared-library/tree/main__;!!NZvER7FxgEiBAiR_!qsgb8wTFlVoBS9VU86JKrNyrbAQfAQd4MZIEkmpDyK3h10xw0SEz1iNheVt5Z6JaJB4XtWER6vxs-6iYqO2wXQ$ Cheers, -- Richard
