Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Would you mind wrapping up a 1.4.3 release? #383

Closed
odrotbohm opened this issue Jan 17, 2023 · 23 comments
Closed

Would you mind wrapping up a 1.4.3 release? #383

odrotbohm opened this issue Jan 17, 2023 · 23 comments
Labels
Milestone

Comments

@odrotbohm
Copy link

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
Copy link
Member

keilw commented Jan 17, 2023

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
Copy link
Member

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

@keilw
Copy link
Member

keilw commented Jan 18, 2023

@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
Copy link

ratoaq2 commented Jan 18, 2023

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
Copy link
Member

keilw commented Jan 18, 2023

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
Copy link

ratoaq2 commented Jan 18, 2023

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
Copy link
Member

keilw commented Jan 18, 2023

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
Copy link

ratoaq2 commented Jan 18, 2023

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
Copy link

ratoaq2 commented Jan 18, 2023

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
Copy link
Member

keilw commented Jan 20, 2023

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 keilw added this to the 1.4.3 milestone Jan 28, 2023
@keilw
Copy link
Member

keilw commented Jan 28, 2023

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
Copy link
Member

keilw commented Jan 28, 2023

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.

@keilw keilw added the epic label Jan 28, 2023
@otaviojava
Copy link
Member

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

@keilw
Copy link
Member

keilw commented Jan 30, 2023

@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 keilw added this to Backlog Feb 12, 2023
@keilw keilw moved this to Todo in Backlog Feb 14, 2023
@keilw keilw moved this from Todo to In Progress in Backlog Feb 14, 2023
@keilw
Copy link
Member

keilw commented Feb 23, 2023

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

@kewne
Copy link
Contributor

kewne commented Mar 18, 2023

@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
Copy link
Member

keilw commented Aug 22, 2023

@kewne If you can, please provide the PR.

@keilw
Copy link
Member

keilw commented Jan 24, 2024

Closing as the release is about to be published.

@keilw keilw closed this as completed Jan 24, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Backlog Jan 24, 2024
@keilw
Copy link
Member

keilw commented Jan 26, 2024

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
Copy link
Member

keilw commented Jan 27, 2024

@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
Copy link

saw303 commented Jan 27, 2024

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
Copy link
Member

keilw commented Jan 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

No branches or pull requests

6 participants