JavaMoney / jsr354-ri

JSR 354 - Moneta: Reference Implementation
Other
334 stars 100 forks source link

Would you mind wrapping up a 1.4.3 release? #383

Closed odrotbohm closed 7 months ago

odrotbohm commented 1 year ago

There hasn't been a release in 2,5 years and the branch for 1.4.3 contains quite a few much awaited, low-level fixes (see below), some of them almost 3 years old. Do you think it'd make sense to wrap up the release to ship these?

keilw commented 1 year ago

Unfortunately there was not enough interest to aim for a 2.x release anytime soon. Without #370 it does not make sense to release 1.4.3, most of the others in https://github.com/JavaMoney/jsr354-ri/milestones/1.4.3 could probably be postponed.

otaviojava commented 1 year ago

@keilw But why not release what we have and then release a version with the others fixes?

keilw commented 1 year ago

@otaviojava Because nothing works without that fixed. The conversion modules are not even built right now and still there are at least 4 test failures or more in the core module.: https://app.travis-ci.com/github/JavaMoney/jsr354-ri/jobs/587916157 The exchange rates and providers in convert rely on the config and as that's somehow broken they also do not get the correct settings.

There are multiple test failures like

javax.money.MonetaryException: Failed to load currency conversion data: null
    at org.javamoney.moneta.convert.ecb/org.javamoney.moneta.convert.ecb.ECBAbstractRateProvider.getExchangeRate(ECBAbstractRateProvider.java:130)
    at org.javamoney.moneta.convert.ecb/org.javamoney.moneta.convert.ecb.ECBCurrentRateProvider.getExchangeRate(ECBCurrentRateProvider.java:1)
    at org.javamoney.moneta/org.javamoney.moneta.spi.LazyBoundCurrencyConversion.getExchangeRate(LazyBoundCurrencyConversion.java:57)
    at org.javamoney.moneta/org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:105)
    at org.javamoney.moneta/org.javamoney.moneta.Money.with(Money.java:382)
    at org.javamoney.moneta/org.javamoney.moneta.Money.with(Money.java:1)
    at org.javamoney.moneta.convert.ecb/org.javamoney.moneta.convert.ecb.ProviderTest.testAccess_ECB(ProviderTest.java:24)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
    at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
    at org.testng.internal.Invoker.invokeMethod(Invoker.java:571)
    at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:707)
    at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:979)
    at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:125)
    at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:109)
    at org.testng.TestRunner.privateRun(TestRunner.java:648)
    at org.testng.TestRunner.run(TestRunner.java:505)
    at org.testng.SuiteRunner.runTest(SuiteRunner.java:455)
    at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:450)
    at org.testng.SuiteRunner.privateRun(SuiteRunner.java:415)
    at org.testng.SuiteRunner.run(SuiteRunner.java:364)
    at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
    at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:84)
    at org.testng.TestNG.runSuitesSequentially(TestNG.java:1187)
    at org.testng.TestNG.runSuitesLocally(TestNG.java:1116)
    at org.testng.TestNG.runSuites(TestNG.java:1028)
    at org.testng.TestNG.run(TestNG.java:996)
    at org.testng.remote.AbstractRemoteTestNG.run(AbstractRemoteTestNG.java:115)
    at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:251)
    at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:77)

Because the configuration won't pick up the necessary conversion data.

ratoaq2 commented 1 year ago

It would be good to have a 1.4.3 release when that blocker issue is completed.

About a 2.x release, because of this major jakarta namespace change, from my POV, it would be better that 2.0 is exactly the same as 1.4.3, with only the javax -> jakarta imports.

That makes upgrade paths easier.

keilw commented 1 year ago

The jakarta namespace is not necessarily up to a 2.x release because that requires a whole new JSR with a new number. Which so far there has not been enough interest and echo by JCP members and while @otaviojava and I are good to help as Maintenance Lead, we would not file a new JSR without a solid and committed EG. After the takeover by Accenture it looks like Trivadis has not renewed its JCP Membership: https://jcp.org/en/participation/members/T Nor has parent company Accenture although it used to be a JCP member supporting e.g. the Portlet Spec.

If either of you in this thread knows a company that would be happy and able to join as co-spec lead, then we could think of a new JSR, otherwise without at least one Corporate member involved, I guess it's maintenance only.

ratoaq2 commented 1 year ago

What I meant was to have a release that doesn't depend on old javax.annotation.

Not actually moving javax money to jakarta money

keilw commented 1 year ago

There will never be a jakarta money, the JSR keeps javax.

381 would involve using Jakarta EE 9,

The 4 failed tests must pass, please propose PRs if you find a solution, some may even have something to do with changes to reflection in later Java versions.

ratoaq2 commented 1 year ago

The 4 failing tests seems related to ServiceLoader not finding the org.javamoney.moneta.function.TestRoundingProvider configured in META-INF/services/javax.money.spi.RoundingProviderSpi from the tests.

I believe the issue might be related to how Java 9 module system changed the ServiceLoader. I'm still not familiar with java modules but if I find a solution I'll propose a PR for them

ratoaq2 commented 1 year ago

Just got my local env to quickly look at it Related to the failing tests in MonetaryRoundingsTest:

On java 17, the java.util.ServiceLoader is indeed ignoring TestRoundingProvider because it is in a named module ( (java.lang.Module) module org.javamoney.moneta)

image image

keilw commented 1 year ago

And that's not the only place.

loading resources like the IMF defaults also fails for JPMS related reasons, but that resolved (by putting the files into the package structure (similar to suggestions in https://stackoverflow.com/questions/61531317/how-do-i-determine-the-correct-path-for-fxml-files-css-files-images-and-other/61531318#61531318) still won't get past #353. The timeout persists

keilw commented 1 year ago

I'

Just got my local env to quickly look at it Related to the failing tests in MonetaryRoundingsTest:

On java 17, the java.util.ServiceLoader is indeed ignoring TestRoundingProvider because it is in a named module ( (java.lang.Module) module org.javamoney.moneta)

image image

@ratoaq2 Thanks for the research. Any remedy for that?

It looks like we cannot mix META-INF/services in test code with module-info declarations, and that means the test classes even require a different module and package structure, based on what https://stackoverflow.com/questions/46613214/java-9-maven-junit-does-test-code-need-module-info-java-of-its-own-and-wher states.

keilw commented 1 year ago

Guys, This is more or less an "epic" if you want, but I also added it to the 1.4.3 milestone issues.

Especially the two with "help wanted" can use some help, as both the proper configuration and hopefully a solution to fetching both ECB and IMF exchange rates at runtime, are the most critical issues, while others like Indian formatting mostly concern fixing unit tests and that seems very advanced.

Once those are fixed, we can release 1.4.3.

otaviojava commented 1 year ago

IMHO: I would put both services out. We don't provide those services, and we don't have any control, such as retrocompability.

keilw commented 1 year ago

@otaviojava No we shouldn't drop IMF either, but properly use the REST service, especially with all the Jakarta/Enterprise people involved ;-) https://datahelp.imf.org/knowledgebase/articles/1968408-how-to-use-the-api-python-and-r Let's use the API in Java instead of the old download option.

keilw commented 1 year ago

With #357 addressed (still waiting for your review) there are at least 2 major showstoppers that need to be addressed:

kewne commented 1 year ago

@keilw I've been looking into the test failures and it seems that, again, the issue is the JPMS... Some of the failing tests rely on a provider for ServiceLoader that is defined in the test sources, which is not loaded.

From what I've found, the tests apparently run in the main sources' module, which does not declare this test provider. While there are some solutions floating around that involve patching the module, IDEs don't seem to support them, which I think makes them unsustainable.

What I think could work is moving the tests that rely on service loaders to a separate source tree, with a separate module definition, which I've tested and works. Let me know if this is acceptable and I'll submit a PR for it.

keilw commented 1 year ago

@kewne If you can, please provide the PR.

keilw commented 7 months ago

Closing as the release is about to be published.

keilw commented 7 months ago

Just in case @odrotbohm the 1.4.3 release was published 2 days ago after all the critical issues, especially around exchange sources could be resolved. It took about a year since this ticket was filed, but everyone involved is very tied by other projects or day-to-day work with customers, so we had to be very careful with available resources. Given @joshlong also asked for travel funding to a conference I helped pick the program, it seems even some of the large vendors have to carefully consider their resources nowadays ;-)

keilw commented 7 months ago

@odrotbohm We don't plan to use an odd/even pattern like the Linux Kernel, but please use release 1.4.4 because in 1.4.3 the IMF exchange rate provider was still disabled to avoid the prior timeouts.

I assume not too many people started using 1.4.3 yet ;-)

saw303 commented 7 months ago

I assume not too many people started using 1.4.3 yet ;-)

@keilw well, in times of dependabot & co. your assumption might be wrong. Anyway, thank you for that information.

keilw commented 7 months ago

@saw303 is it possible to add upgrade recommendations for libraries like Moneta to Dependabot?

saw303 commented 7 months ago

@keilw Yes, I guess it is. At least according to the docs https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuring-dependabot-version-updates#checking-the-status-of-version-updates