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

[ARIES-2161] update destroy method to be compatible with spring 6 #299

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

f2par0
Copy link

@f2par0 f2par0 commented Dec 2, 2024

Problem found : the class SpringApplicationContext inherits from org.springframework.context.support.AbstractApplicationContext , which had a destroy method in version 4, but was deprecated then removed in the version 6 of spring framework.

The name of the destroy method must be updated to match this new name

@alien11689
Copy link
Contributor

Hi @f2par0
could you create an jira issue and add jira id to the PR name and commit?
The blueprint-spring subproject has a dependency to spring - could you also check them? I think they should be updated to version 6.x

@f2par0 f2par0 changed the title update destroy method to be compatible with spring 6 [ARIES-2161] update destroy method to be compatible with spring 6 Dec 2, 2024
@@ -119,13 +119,13 @@
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
<version>4.2.2.RELEASE</version>
<version>6.1.15</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked that spring 6.x does not work with minimal java 8 that we should have in the repo
you can find the errors by running:

mvn -f blueprint/blueprint-spring clean package

or checking your branch with definition of blueprint build from #297

Copy link
Author

Choose a reason for hiding this comment

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

yes, spring 6 is for java 17, so if java 8 support is required, the latest version is 5.3.39.

Copy link

Choose a reason for hiding this comment

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

spring 5 is end of life, it does not really make sense to support spring 5. blueprint-spring should support spring 6 (therefore need to move to jdk17) is there a way to move one project without moving all blueprint projects ?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of possible users who stay with Java 8 so it's better to keep all the artifacts compatible with Java 8 if possible. Especially that Spring is not often used in OSGi projects.

But personally I see the value in using the newest version of Spring.
Maybe the solution here may be creating the new artifact blueprint-spring6. Existing blueprint-spring could use newest spring 5.x version supporting Java 8. Perfectly the code should be reused between modules so the new module should only override necessary parts (if possible).
WDYT?

Copy link

@amergey amergey Jan 5, 2025

Choose a reason for hiding this comment

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

Yes, but people using spring cannot stay with Java 8, so blueprint-spring itself will not be use in java 8.
Anyway I realized looking at this PR than same code can work for spring 5 and current spring 6 as well with no differences yet, so no need to separate artifacts yet, and blueprint-spring can stay with java 8

<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<target>1.7</target>
Copy link
Contributor

Choose a reason for hiding this comment

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

java 8 should be minimal supported version

@amergey
Copy link

amergey commented Jan 5, 2025

You may have a look to an old PR I opened for spring 5 which should work for spring 6 as well
https://github.com/apache/aries/pull/300/files

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.

3 participants