search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Outdated dependency versions #752

Closed JanWesterkamp-iJUG closed 1 year ago

JanWesterkamp-iJUG commented 1 year ago

Hi, as mentioned in the last call I found some outdated dependency versions here:

@donbourne I hope this is helpful feedback. At the moment i.e. Jakarta dependencies do not fit to Jakarta EE 10.0.0 versions and the ones in the tck/README.adoc are very outdated. A PR for updating the MP Parent tck-bom versions is pending - when versions are derived from there, manual updates on different locations can be omitted in the future. The ones you would like to be added there should be added, at least when multiple component specs require them.

donbourne commented 1 year ago

@JanWesterkamp-iJUG that's really thorough and helpful.

The ones I'm most concerned about, for MP Metrics 5.0 release, would be the two that are not under the TCK directory. The Jakarta CDI dependency is currently listed as 4.0.0, so is only off by a fix release. I think the Jakarta REST property is only used on the TCK side. @Channyboy , can you confirm that Jakarta REST isn't used on API side? I don't see it in the API-side pom.xml and I can't imagine how it could be used by the API). Need to make sure we know if this has any material impact on the 5.0 release.

The TCK ones we should fix at the first opportunity, but given they are TCK-side I wouldn't think they'd be considered stop-ship for 5.0.

Channyboy commented 1 year ago

@donbourne Right, API doesn't need the Jakarta REST. Only the Optional TCK does.

donbourne commented 1 year ago

I think that implies that none of these dependency versions listed in this issue would affect apps based on MP Metrics 5.0 API, so I'd suggest we do a PR to address these and include it if we need to do another 5.0.0-RC, otherwise we'd include it in 5.0.1.

Does that sound ok?

JanWesterkamp-iJUG commented 1 year ago

@donbourne the CDI "Service Release" fixed a severe issue: 4.0.0 had a provided dependency to Jakarta Transaction(s), which in turn had a dependency to CDI 3.x - so causing a circular dependency between them and preventing the intended use of CDI Lite as base for the Jakarta Core Profile 10.0.0 - intended in turn to be used in MicroProfile 6.0.

So the reference in the MP Metrics API and TCK of that version might work fine in a Jakarta EE Full Profile environment or in an OSGi based design where cou can control the dependencies beside Maven, but in other MicroProfile environments there is a dependency to something, that is (and will not) be available at all... One way to fix this is to extent the exclude statements - but then you could change the version much easier. By the way, having deviations between component spec CI test runs and platform spec CI runs or deviations between API and TCK configuration (not the case here) are problematic from my point of view in gerneral.

donbourne commented 1 year ago

@JanWesterkamp-iJUG , could you please clarify what you're asking for on the last 2 bullets... tck/api/pom.xml TBD -> may be define version a level lower only one time tck/api/pom.xml TBD -> may be define version a level lower only one time

donbourne commented 1 year ago

@JanWesterkamp-iJUG , could you please have a look at #753 to see if that PR looks good to you?

JanWesterkamp-iJUG commented 1 year ago

@JanWesterkamp-iJUG , could you please clarify what you're asking for on the last 2 bullets... tck/api/pom.xml TBD -> may be define version a level lower only one time tck/api/pom.xml TBD -> may be define version a level lower only one time

@donbourne and @Channyboy I meant that these two configurations could be done in one single place one level towards the parent POM (tck/pom.xml instead tck/rest/pom.xml and tck/optional/pom.xml). When the properties are configured there, then the dependencies could be declared in the two Maven sub modules and still could be maintained via a single setting (in tck/pom.xml). A lower chance to forgett something in the future.

But originally I used the wrong link text for this two points - I updated them now for clarification in the initial list on the top of this issue.

JanWesterkamp-iJUG commented 1 year ago

@Channyboy why you closed the issue and tag it with 5.1 and 5.1 afterwards?

You well addressed most of the findings, but some parts are still open and I suggest we should address them, i.e. the TCK README.adoc (lower part) is not fixed and the dependencies to Jakarta Core Profile solve the version mismatch, but declares more dependencies than necessary (the compete profile instead of specific parts of it). Of couse, solving the last might require some discussions in the MP technical call(s) first... ;-)

So I would suggest to reopen it until all parts fixed completely.

donbourne commented 1 year ago

why you closed the issue and tag it with 5.1 and 5.1 afterwards?

I assume you mean "why you closed the issue and tag it with 5.1 and 5.0 afterwards?" -- originally we thought this was something that could wait to be addressed in 5.1, but your comment from Oct 17 made sense and we did what we thought was needed to fix this issue, then put out a new RC (which is why we retagged it with 5.0).

the TCK README.adoc (lower part) is not fixed

from ballot discussion you mentioned that the lower part of the TCK readme indicates that Java 8 be used for TCK -- and that you think it should be changed to Java 11 (since MP Metrics 5.0 requires Jakarta Core Profile which requires Java 11). Is that the issue you mean? I think we should fix it, for consistency, but MP Metrics doesn't require anything from Java 11 or from newer capabilities of Jakarta EE 10 Core Profile, so it seems fine to address that in 5.1 - would you agree?

dependencies to Jakarta Core Profile solve the version mismatch, but declares more dependencies than necessary (the compete profile instead of specific parts of it).

MP Metrics 5 doesn't need all of Jakarta Core Profile, so the dependency could be changed to refer to specific parts of it instead. The build-time dependency on Jakarta Core Profile doesn't break anyone though (even MP Metrics 5 impls that don't implement Jakarta Core Profile), so I think we could also address this in a follow on issue in 5.1 release - would you agree?

donbourne commented 1 year ago

Reopening this as there are still parts of it that aren't fully addressed

donbourne commented 1 year ago

we should modify MP Metrics pom.xml to depend on the specific specs, and let the microprofile-parent specify the versions. eg., from the MicroProfile Fault Tolerance api pom.xml:

        <dependency>
            <groupId>jakarta.enterprise</groupId>
            <artifactId>jakarta.enterprise.cdi-api</artifactId>
            <scope>provided</scope>
        </dependency>
Channyboy commented 1 year ago

@JanWesterkamp-iJUG closing the item now. If there are any issues not covered from https://github.com/eclipse/microprofile-metrics/pull/762 and https://github.com/eclipse/microprofile-metrics/pull/761 we can re-open the issue.