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

upgrade jasperreports to 7.0.0 #102

Merged

Conversation

stoufix
Copy link
Contributor

@stoufix stoufix commented Sep 5, 2024

No description provided.

@RainerGanss RainerGanss mentioned this pull request Sep 5, 2024
4 tasks
pom.xml Outdated Show resolved Hide resolved
@stoufix stoufix force-pushed the stouphix/upgrade-jasperreports-v7 branch from d32b88a to 6f33668 Compare September 5, 2024 15:03
@RainerGanss
Copy link

Hi @stoufix

I also think we should update the readme at two places. A migration part at the top and the updated table at the very bottom.

Here is a suggestion for these parts.

Migration to version 4
----------

The release of [JasperReports](https://github.com/TIBCOSoftware/jasperreports) version 7.0.0 introduced significant changes, including some breaking changes. As a result, this Maven plugin requires updates, and all reports must be manually migrated using Jasper Studio. For more details on the changes, please refer to the [release notes](https://github.com/TIBCOSoftware/jasperreports/releases/tag/7.0.0).

Due to classpath conflicts, it is not possible to compile reports for versions below 7.0.0 and versions 7.0.0 or above using the same version of this plugin. To find the correct plugin version for your JasperReports version, please refer to the information at the bottom of this README.
| 4.0.0          | 7.0.0                 |

Cheers,
Rainer

@rturner-edjuster
Copy link
Contributor

FYI: I've also given this branch a spin locally, and it seems to be working for me as well. However, the MOJO integration test is failing (but that seems unrelated to the changes).

Copy link
Contributor

@Postremus Postremus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stoufix for getting this migration started.

Some small points:

  • Remove xmlValidation from the example configuration in the Readme.md.
  • Please document in the migration section the removal of xmlValidation

I will at least wait till @RainerGanss gets a chance to run this PR against his production reports.

Copy link
Contributor

@Postremus Postremus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your recent changes. I had time to think about another point, see specific comment.

pom.xml Outdated Show resolved Hide resolved
@RainerGanss
Copy link

@Postremus Could you please look at the failed build? Not sure, why removing the test-scope failes the build. Seems like java 11 issue.

@Postremus Postremus force-pushed the stouphix/upgrade-jasperreports-v7 branch from fffe979 to 1c12c5c Compare September 9, 2024 10:53
@Postremus
Copy link
Contributor

@RainerGanss @stoufix as a heads up, I rebased this branch against main to include 8b8c1f2.

@RainerGanss
Copy link

@Postremus we could successfully verify the changes. Manually converting every report with Jasper Studio + using local version 4.0.0-SNAPSHOT of this plugin worked.

@rturner-edjuster
Copy link
Contributor

rturner-edjuster commented Sep 9, 2024

If anyone is going to make additional changes to the PR, and if you want to eliminate some warnings during the build, you can also make the following changes:

  • pom.xml -> maven-failsafe-plugin -> renamed the systemProperties element to systemPropertyVariables (I get a deprecation warning on this -- however, if the maven-failsafe-plugin version isn't controlled by the project, could be host environment dependent)
  • change the classes JasperMojoConfiguration and JasperReportCompiler to be "package protected" instead of "public" (eliminates Javadoc warnings as these classes do not need to be public as far as I can tell).

FWIW, I cannot get the Integration Tests to run / pass -- I'm not sure if there is something special I need to do to prep -- but just an FYI in case it's important.

@Postremus Postremus mentioned this pull request Sep 9, 2024
@Postremus
Copy link
Contributor

@rturner-edjuster

I'd prefer to merge this PR as is.

Any additional changes can be done at a later time. I have opened #104 to keep track of this.

Could you please create an issue about the failing integration tests? They should run without any special setup. Please attach the log files from the target\maven-it\com\pro_crafting\tools\jasperreport\JasperMojoIT\testConfigurationMapping directory, which get generated during an mvn verify run. Thanks

Copy link
Contributor

@Postremus Postremus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Postremus Postremus merged commit 89f4a77 into pro-crafting:main Sep 9, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants