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.

Sign up for GitHub

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

Jump to bottom

chore: remove aspectj-rt from the library #1408

Merged
merged 2 commits into from
Sep 1, 2023
Merged

chore: remove aspectj-rt from the library #1408

merged 2 commits into from
Sep 1, 2023

Conversation

jeromevdl
Copy link
Contributor

Issue #, if available: #1401

Description of changes:

  • remove aspectj-rt from the library itself => requires to add it in the project that uses powertools, but at least developers can choose their version and are not stick to 1.9.7 which does not work anymore with java 17.
  • update example to simplify the configuration (just set the aspectj version in the profile and keep the same plugin configuration). If we remove java8, we can remove the profile (default version of aspectj is 1.9.20)

Checklist

Breaking change checklist

requires the developers to add to their pom

        <dependency>
            <groupId>org.aspectj</groupId>
            <artifactId>aspectjrt</artifactId>
            <version>...</version> <!-- 1.9.7 for java 8 or higher otherwise -->
        </dependency>

RFC issue #:

  • Migration process documented ==> not yet
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jeromevdl
Copy link
Contributor Author

@roamingthings, what do you think about it?

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (v2@b5135c9). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files 
@@          Coverage Diff          @@
##             v2    #1408   +/-   ##
=====================================
  Coverage      ?   77.17%           
  Complexity    ?      522           
=====================================
  Files         ?       64           
  Lines         ?     2134           
  Branches      ?      223           
=====================================
  Hits          ?     1647           
  Misses        ?      415           
  Partials      ?       72

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

roamingthings
roamingthings reviewed Aug 30, 2023
View reviewed changes
Copy link
Contributor

@roamingthings roamingthings left a comment

Choose a reason for hiding this comment

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

The parent pom still uses 1.9.7 and I'm not sure if this would be treated as a transient dependency. I will try to do some testing.

examples/powertools-examples-batch/pom.xml Show resolved Hide resolved
@scottgerring
Copy link
Contributor

I haven't had a chance to think about this in depth yet, but my initial reaction is:

requires to add it in the project that uses powertools

Where does this fall on the spectrum of breaking changes? We'd have to at the very least message this clearly in the release notes, as it is likely to break most client's builds when they roll their PT dep(s) forward.

@roamingthings
Copy link
Contributor

roamingthings commented Aug 30, 2023 •
edited
Loading

as it is likely to break most client's builds when they roll their PT dep(s) forward

This relates to all project that don't use the AspectJ plugin.

@jeromevdl
Copy link
Contributor Author

The parent pom still uses 1.9.7 and I'm not sure if this would be treated as a transient dependency. I will try to do some testing.

@roamingthings, the parent pom references it but no module actually uses it, and we still need it for our own code. And since we support java8 (until @scottgerring decides to leave it 😄), we need to keep the 1.9.7 version. But it should not be retrieved in your application. Actually you need to explicitly specify one version (1.9.20 most probably as of now). I didn't try with Gradle yet...

@roamingthings
Copy link
Contributor

@jeromevdl

I didn't try with Gradle yet...

That's what I meant by

I will try to do some testing.

@roamingthings
Copy link
Contributor

I can confirm that Gradle will no longer add aspectj-rt as transient dependency with this change 👍

@jeromevdl jeromevdl marked this pull request as draft August 30, 2023 14:44
@jeromevdl
Copy link
Contributor Author

moving as draft, I want to merge main into v2 to get the gradle sample working with this

@scottgerring scottgerring linked an issue Aug 30, 2023 that may be closed by this pull request
Maintenance(v2): Update aspectj dependency to current version #1401
Open
2 tasks
@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jeromevdl jeromevdl marked this pull request as ready for review September 1, 2023 05:43
scottgerring
scottgerring approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

LGTM. And I am super glad we have the E2E tests for this sort of thing.

@jeromevdl jeromevdl merged commit 2d3e865 into v2 Sep 1, 2023
13 checks passed
@jeromevdl jeromevdl deleted the chore/remove-aspectj-rt branch September 1, 2023 06:58
jeromevdl added a commit that referenced this pull request Oct 12, 2023
@jeromevdl
chore: remove aspectj-rt from the library (#1408)
286a6c2 
* remove aspectj-rt from the library
* set aspectj.version 1.9.20
jeromevdl added a commit that referenced this pull request Oct 12, 2023
@jeromevdl
chore: remove aspectj-rt from the library (#1408)
317c5a7 
* remove aspectj-rt from the library
* set aspectj.version 1.9.20
jeromevdl added a commit that referenced this pull request Oct 13, 2023
@jeromevdl
chore: remove aspectj-rt from the library (#1408)
c9dd862 
* remove aspectj-rt from the library
* set aspectj.version 1.9.20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roamingthings roamingthings roamingthings left review comments

@scottgerring scottgerring scottgerring approved these changes

Assignees
No one assigned
Labels
size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Maintenance(v2): Update aspectj dependency to current version
4 participants
@jeromevdl @codecov-commenter @scottgerring @roamingthings