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

build warning about parameter "localRepository" after upgrading to maven 3.9.1 #223

Closed
XN137 opened this issue Mar 20, 2023 · 28 comments · Fixed by #230
Closed

build warning about parameter "localRepository" after upgrading to maven 3.9.1 #223

XN137 opened this issue Mar 20, 2023 · 28 comments · Fixed by #230
Assignees
Milestone

Comments

@XN137
Copy link

XN137 commented Mar 20, 2023

[INFO] --- forbiddenapis:3.3:check (default) @ mycompany-common ---
[WARNING] Parameter 'localRepository' is deprecated core expression; Avoid use of ArtifactRepository type. If you need access to local repository, switch to '${repositorySystemSession}' expression and get LRM from it instead.

the warning seems to come from:
https://issues.apache.org/jira/browse/MNG-7706
where there are suggestions on how mojos should address it

it would be nice to avoid these build warnings in the next forbidden-apis release
thank you!

@uschindler
Copy link
Member

I do not think this can be quickly fixed, as forbiddenapis needs to raise the minimum Maven version first.

Does the warning always appear? IMHO, it should only appear if you use the localRepository parameter in the signaturesArtifacts. If you don't refer to those artifacts and just use plain forbiddenapis with bundled sogatures or local files, it should not ptint any warning. If yes, I would recommend to open a bug report at Maven to only print the deprectaed warning when the feature in question is actually used.

@XN137
Copy link
Author

XN137 commented Mar 20, 2023

thanks for the quick feedback!

yes our build is using <signaturesArtifacts> in the parent pom for all other modules like this:

<signaturesArtifacts>
  <signaturesArtifact>
    <groupId>com.mycompany.tools</groupId>
    <artifactId>mycompany-build-tools</artifactId>
    <version>${project.version}</version>
    <type>jar</type>
    <path>mycompany-forbiddenapis/signatures.txt</path>
  </signaturesArtifact>
</signaturesArtifacts>

if you dont do that i doubt the warning will appear (but i havent tried that out)

I do not think this can be quickly fixed, as forbiddenapis needs to raise the minimum Maven version first.

just wondering: what minimum version would we be needing?
according to my understanding the deprecation warning was only introduced in 3.9.1. however the alternative and more correct way to acquire the same information could be available in much earlier maven versions already (we'd need to try that out i guess).

for example https://github.com/apache/maven-help-plugin/pull/88/files seems to show that having a single MavenProject parameter can be used as a replacement...
and i am guessing that functionality has been available in maven for a longer time already

@uschindler
Copy link
Member

All fine, I will check this. The next release is going to come out the next days, so this won't be included.

just wondering: what minimum version would we be needing?

At moment it compiles against Maven 2.0 API, because nothing more was needed for this plugin to work correctly. Upgrading to minimum version 3.0 would be possible, but needs some work. I will check how this can be resolved, but I am on business trip at the moment, so I will only release the current "main" branch soon when ASM has been upgraded and the Java 20 release came out tomorrow.

@uschindler
Copy link
Member

It looks like this is the problem, correct?

@Component
private ArtifactFactory artifactFactory;
@Component
private ArtifactResolver artifactResolver;

@XN137
Copy link
Author

XN137 commented Mar 20, 2023

i am guessing it's:

@Parameter(defaultValue = "${localRepository}", readonly = true, required = true)
private ArtifactRepository localRepository;

but if you check the PR i've linked earlier it seems that both these parameters can be replaced by going through a MavenProject parameter

@kwin
Copy link
Contributor

kwin commented Apr 5, 2023

You need to leverage the Resolver API instead which ships with Maven since 3.1.0 (https://cwiki.apache.org/confluence/display/MAVEN/Maven+Ecosystem+Cleanup?focusedCommentId=217389433#MavenEcosystemCleanup-resolverMavenResolver(akaMavenArtifactResolver)). I would recommend to require a newer Maven 3.5 version (as the resolver moved then from Eclipse to ASF). Further details at https://maven.apache.org/resolver/api-compatibility.html#inside-of-maven

@uschindler
Copy link
Member

Thanks this information is helpful. I will look into this in the next days.

kwin added a commit to kwin/forbidden-apis that referenced this issue Apr 8, 2023
Increase Maven requirement to 3.5.0 (Resolver 1.0.3)
Enforce minimum Maven version via prerequisite

This closes policeman-tools#223
kwin added a commit to kwin/forbidden-apis that referenced this issue Apr 8, 2023
Increase Maven requirement to 3.5.0 (Resolver 1.0.3)
Enforce minimum Maven version via prerequisite

This closes policeman-tools#223
@Stephan202
Copy link
Contributor

Does the warning always appear? IMHO, it should only appear if you use the localRepository parameter in the signaturesArtifacts.

Our build uses bundledSignatures rather than signaturesArtifacts, and we also see the warning. The changes in #230 make it go away. 👍

@kwin
Copy link
Contributor

kwin commented May 16, 2023

@uschindler Gentle ping. Any chance you can look at the PR and afterwards spin a new release?

@uschindler
Copy link
Member

Hi, thanks. Sorry for the delay. It is busy to me at moment. Merging is one part, but the release won't come directly afterwards as this needs a bit of planning. I have not yet checked if this causes major API headaches (so 4.0 needs to be released, it that case done other features are on my list). If it can be merged and works with Java 7, no problem to have a release soon.

@kwin
Copy link
Contributor

kwin commented May 16, 2023

Yes, Java 7 is still supported: https://maven.apache.org/docs/history.html#maven-3-6-x-and-before

@bmarwell
Copy link

@kwin @uschindler Java 7 is very hypothetical. If you have up-to-date plugins, most will already require Java 8. But yes, Maven 3.x Core is still Java 7. May I ask why you would like to support Java 7?

@uschindler
Copy link
Member

@kwin @uschindler Java 7 is very hypothetical. If you have up-to-date plugins, most will already require Java 8. But yes, Maven 3.x Core is still Java 7. May I ask why you would like to support Java 7?

Actually I don't want to. It's just that I wanted more time to release a new version 4 with java 8, some rewritten parts.

Version 3 has just backwards compatibility, still signatures files for 7. It also compules with 7 and some commons projects of Apache were still using 7.

But actually we can still parse java 7 classes, just the runtime needs to be 8.

So in short: it's a temporal problem. I am very busy and people here want to get a new release just to get rid of warnings.... I do t understand that. Just ignore the warnings and give me some time! Unfortunately Maven is not my highest priority, the build system in version 4 of the plugin will be Gradle du to it's more flexible Scripting possibilities for building the plugin.

@uschindler uschindler self-assigned this May 20, 2023
@uschindler uschindler added this to the 3.6 milestone May 20, 2023
uschindler added a commit that referenced this issue May 20, 2023
Increase Maven requirement to 3.5.0 (Resolver 1.0.3); enforce minimum Maven version via prerequisite

This closes #223

Co-authored-by: Uwe Schindler <[email protected]>
@uschindler
Copy link
Member

I merged this but tests fail....

I also had to downgrade the maven-plugin-plugin to stay compatible with java 7.

@uschindler
Copy link
Member

I will fix the remaining problems on main branch, looks like nobody ran tests because those use an earlier version. There are also Maven 2 tests!

@uschindler uschindler reopened this May 20, 2023
@uschindler
Copy link
Member

I fixed the remaining issues after merging in multiple commits on main branch.

A release may take some time as I have to verify the test infrastructure and other stuff; also the minimum required Maven version in documentation and anywhere else.

The PR also used the "test" Maven version as minimum hardcoded into the plugin's minimum version. This is not related to each other.

So please don't hurry again with new releases, I have not much time at moment. You may use snapshot builds if you are annoyed by warnings...

@bmarwell
Copy link

I merged this but tests fail....

I also had to downgrade the maven-plugin-plugin to stay compatible with java 7.

I get why you would want run-time compatibility with Java 8, but trying to stay on Java 7 for builds is just unnecessary. Java 17 --release=7 and then test with 7 using toolchains will allow you to upgrade plugins.

Or is there any other reason?

@uschindler
Copy link
Member

The test failure happened on all versions. It is fixed now. The problem was that the Maven 3 tests were running against Maven 3.0.4.

@uschindler
Copy link
Member

This will change with forbiddenapis 4 and Gradle as it's build system. The old Ant based build is too inflexible.

I just don't want to rush that. I wanted to do that on a free weekend but people are in a hurry for unknown reasons just to get away with a stupid warning.

@uschindler
Copy link
Member

I will fix the remaining problems of the PR and fix the rest of inconsistencies and update documentation.

The main problem was not plugin versions (but actually I don't know why it had to be upgraded at all because the build was working and maven-plugin-plugin's only job is to setup the pom file and create documentation). The version change was IMHO an unrelated change that complicated everything more. I would wish people would concentrate on the required changes in PRs only.

The minimum Maven version change and the fixes for the Maven artifact download are well done, just the build system of the project is a bit crazy.

I want to change that, together with other version 4 features. I have a detailed plan for it ready, but just no time.

@uschindler
Copy link
Member

this one is also incorrect, because the POM variant that goes to Maven central should not contain properties: https://oss.sonatype.org/content/repositories/snapshots/de/thetaphi/forbiddenapis/3.6-SNAPSHOT/forbiddenapis-3.6-20230520.190405-4.pom

The minimum Maven version needs to be hardcoded in that file. I wonder why Maven accepts this when installing the plugin?

@uschindler
Copy link
Member

uschindler commented May 21, 2023

Hi,
I now changed the dependencies and did extensive testing:

  • Maven 3.1.0 is the minimum version now: It works with Java 7 and it is the first version using "Aether Resolver API".
  • The build time dependencies are now aether-api 0.9.0.M2 (as used by Maven 3.1.0). The utils package is not needed. Please don't complain here: That's just the minim version needed to be compatible with Maven 3.1.0. See the map of dependencies
  • I hardcoded the minimum Maven version also into the deployed POM
  • Testing is also done with minimum version

Please don't complain again about "outdated versions". Forbiddenapis does not ship with any of those "outdated" dependencies. It only compiles against those versions to only use those APIs that are provided.

I also restored the mavn-plugin-plugin version back to the original version. The changes done in #229 were not needed and totally unrelated to an update of Maven minimum versions (this PR was a documentation update only).

@kwin
Copy link
Contributor

kwin commented May 21, 2023

I also restored the mavn-plugin-plugin version back to the original version. The changes done in #229 were not needed and totally unrelated to an update of Maven minimum versions (this PR was a documentation update only).

Right, those relate to #228 as stated in #228 (comment).

@uschindler
Copy link
Member

@uschindler
Copy link
Member

I also restored the mavn-plugin-plugin version back to the original version. The changes done in #229 were not needed and totally unrelated to an update of Maven minimum versions (this PR was a documentation update only).

Right, those relate to #228 as stated in #228 (comment).

Rendering of links works. And links to Oracle's Javadocs are not needed, so 3.4 is fine. The reason why the build uses the plugin-plugin (v3.4) is to just render the documentation. This versions produces the minimal possible hassle, later versions then complain about missing source code and other things. So please don't change that version.

@uschindler
Copy link
Member

Thanks @ all for reporting this issue and the discussions. Thank you @kwin for implementing the aether resolver API.

@uschindler
Copy link
Member

P.S.: It looks like Sonatype snapshots repository is not working at moment. Jenkins gets HTTP errors 502 and 504 while uploading the artifacts. Please try the snapshot version later this day.

@uschindler
Copy link
Member

Build was now successful: https://jenkins.thetaphi.de/job/Forbidden-APIs/499/console

Artifacts are here: https://oss.sonatype.org/content/repositories/snapshots/de/thetaphi/forbiddenapis/3.6-SNAPSHOT/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants