search

m-m-m / code

Library to parse, analyze, transform and generate code
Apache License 2.0
2 stars 3 forks source link

35 fix mavenbridgeimpl #38

Closed jan-vcapgemini closed 2 years ago

jan-vcapgemini commented 3 years ago

First test using dependencyManagement and springboot framework. Could be simplified in next iteration.

hohwille commented 3 years ago

mhm, so all checks passed. JUnit does not reveal any bug and no fix is included.

jan-vcapgemini commented 3 years ago

mhm, so all checks passed. JUnit does not reveal any bug and no fix is included.

There seems to be an issue with the unit tests in mvn. I can only run the failing test with some adjustments (test runner JUnit 4 instead of JUnit 5) in my Eclipse.

hohwille commented 3 years ago 
Error:  testResolveDependencyManagementVersionFromParent  Time elapsed: 0.001 s  <<< ERROR!
java.lang.IllegalStateException: Failed to read effective model for src/test/resources/testdata/localmavenproject/maven.project/core/pom.xml. Try to run 'mvn help:effective-pom' in that folder manually. If that fails, fix your pom.xml. Otherwise report a bug at https://github.com/m-m-m/code/issues/new/choose
    at net.sf.mmm.code.java.maven.impl.MavenBridgeImplTest.testResolveDependencyManagementVersionFromParent(MavenBridgeImplTest.java:68)
Caused by: org.apache.maven.model.building.ModelBuildingException: 
2 problems were encountered while building the effective model for testing:core:1.0.0-beta7-SNAPSHOT
[FATAL] Non-readable POM /home/runner/.m2/repository/org/springframework/boot/spring-boot-dependencies/2.5.5/spring-boot-dependencies-2.5.5.pom: /home/runner/.m2/repository/org/springframework/boot/spring-boot-dependencies/2.5.5/spring-boot-dependencies-2.5.5.pom (No such file or directory) @ /home/runner/.m2/repository/org/springframework/boot/spring-boot-dependencies/2.5.5/spring-boot-dependencies-2.5.5.pom
Error:  'dependencies.dependency.version' for org.springframework.boot:spring-boot-starter-jdbc:jar is missing. @ 

    at net.sf.mmm.code.java.maven.impl.MavenBridgeImplTest.testResolveDependencyManagementVersionFromParent(MavenBridgeImplTest.java:68)

Good news is the improved error handling seems to work as expected (#36). Strange thing is the claimed missing dependency. The referenced POM is: https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/2.5.5/spring-boot-dependencies-2.5.5.pom

And it clearly contains this block:

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-jdbc</artifactId>
<version>2.5.5</version>
</dependency>

So in the first place this looks like a bug in maven to me. Maybe we need to update the version of maven and retest as a first try.

hohwille commented 3 years ago

OK, reading more carefully:

(No such file or directory) @ /home/runner/.m2/repository/org/springframework/boot/spring-boot-dependencies/2.5.5/spring-boot-dependencies-2.5.5.pom

So the problem seems to be that the according POM is not present in local maven repo. This is a pre-requisite. That is why cg is telling to build the project with maven first. Most likely this is the problem here...

jan-vcapgemini commented 3 years ago

OK, reading more carefully:

(No such file or directory) @ /home/runner/.m2/repository/org/springframework/boot/spring-boot-dependencies/2.5.5/spring-boot-dependencies-2.5.5.pom

So the problem seems to be that the according POM is not present in local maven repo. This is a pre-requisite. That is why cg is telling to build the project with maven first. Most likely this is the problem here...

I've already built the project with "mvn clean install" before executing the test. I could add the target directory too if you want. The test is still failing with the same message though.

hohwille commented 3 years ago

I've already built the project with "mvn clean install" before executing the test. I could add the target directory too if you want. The test is still failing with the same message though.

OK. May it be possible that mvn clean install and JUnit with mmm-code are using different local maven repos? Is the file in the local repository after mvn clean install? If yes, we need to debug why the code does not find it even though it is there. If no, we need to understand why maven does not download it even though it seems to be required to build the effective POM.

jan-vcapgemini commented 3 years ago

I've already built the project with "mvn clean install" before executing the test. I could add the target directory too if you want. The test is still failing with the same message though.

OK. May it be possible that mvn clean install and JUnit with mmm-code are using different local maven repos? Is the file in the local repository after mvn clean install? If yes, we need to debug why the code does not find it even though it is there. If no, we need to understand why maven does not download it even though it seems to be required to build the effective POM.

Maven and JUnit are using the same repository path. Debugging shows that buildingRequest/modelResolver/localRepository/path is the same as mvn help:evaluate -Dexpression=settings.localRepository -q -DforceStdout

jan-vcapgemini commented 2 years ago

I've found out that the issue only occurs, when a .mvn folder with a maven.config file and following content is present: -Drevision=1.0.0-SNAPSHOT

The resolveProperties method completely overwrites the System Properties with the revision property. https://github.com/m-m-m/code/blob/0470b864e3077d3d439dcfed87b8be8fdcd76a0e/java/maven/src/main/java/net/sf/mmm/code/java/maven/impl/MavenBridgeImpl.java#L217-L221

jan-vcapgemini commented 2 years ago

I've added a maven.config to the test project and merged all of @maybeec 's maven-config-fix changes to this PR. The test succeeds now and the issue seems to be fixed.

maybeec commented 2 years ago

Please change it to become a non draft PR now

hohwille commented 2 years ago

There are two test errors:

  1. https://github.com/m-m-m/code/blob/c9bde3af9ba82bf5dd992bca51043f89bdd16b15/java/maven/src/test/java/net/sf/mmm/code/java/maven/impl/MavenBridgeImplTest.java#L51
  2. https://github.com/m-m-m/code/blob/c9bde3af9ba82bf5dd992bca51043f89bdd16b15/java/maven/src/test/java/net/sf/mmm/code/java/maven/impl/MavenBridgeImplTest.java#L93 which is verifying the dependencies of the module itself which is https://github.com/m-m-m/code/blob/c9bde3af9ba82bf5dd992bca51043f89bdd16b15/java/maven/pom.xml
maybeec commented 2 years ago

are pr builds blocked by intention? It would have been valuable to see directly the test case failures on GA at push time to not delay further the work on this topic.

hohwille commented 2 years ago

are pr builds blocked by intention?

Nope. Seems to be a newer github feature. I did not enable this and could not find any option in the settings to change it either. There is Fork pull request workflows from outside collaborators in Settings > Actions but it gives me three options and none of them allows to prevent approval. Maybe cause there is no billing plan associated with my org...

maybeec commented 2 years ago

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

hohwille commented 2 years ago

I just had a couple of minutes but 1. is not caused by this PR for sure. Version is only present in the parent https://github.com/m-m-m/code/blob/c9bde3af9ba82bf5dd992bca51043f89bdd16b15/java/maven/pom.xml#L8 And this seems to read the raw model without inheritance and interpolation. Maybe this also changed with the maven update and dependency changes lately. Version was earlier present in pom itself but never just as ${revision}: https://github.com/m-m-m/code/blob/eaf3980db2baa147acd77e48c33a83763944e6d3/java/maven/pom.xml#L12

So why are the tests not failing on master?

[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] 
[INFO] --- maven-jar-plugin:3.1.2:jar (default-jar) @ mmm-code-java-maven ---
[INFO] Building jar: /home/runner/work/code/code/java/maven/target/mmm-code-java-maven-1.0.0-beta7-SNAPSHOT.jar

Seems that no test is executed there. Also currently not yet any clue why... Need some time but have none. Pity.

maybeec commented 2 years ago

Test did not run as @jan-vcapgemini already told above, it seemed to be an issue of the migration to junit5 in your code. It was fixed by him in this PR, therefore you see the issue raising here right now @hohwille. I think @jan-vcapgemini can shortly have a look into these issues and clarify them to help out.

hohwille commented 2 years ago

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

That is exactly what I found but it does not help in any way. If you find a concrete option that would prevent this manual approval I would be happy but there is none.

hohwille commented 2 years ago

fixing 1. is easy: https://github.com/m-m-m/code/blob/c9bde3af9ba82bf5dd992bca51043f89bdd16b15/java/maven/src/test/java/net/sf/mmm/code/java/maven/impl/MavenBridgeImplTest.java#L51 Change to:

assertThat(model.getParent().getVersion()).isEqualTo("${revision}");

And we are done. IMHO 2. is more tricky as I would first like to understand what triggered this change. Seems that when this test was written no transitive dependencies have been included and this now changed. If that is confirmed, we could change verifyDependencies to accept a flag that tells that it should check for contains rather than containsExactly and set this flag to true for testReadProject but leave it as false for testReadModel.

hohwille commented 2 years ago

For the GHA workflows, I will follow devonfw best practices and separate CI from PR builds.

jan-vcapgemini commented 2 years ago

The testReadProject() verifyDependencies method is a bit too restrictive I think. https://github.com/m-m-m/code/blob/8bf499fe980220ff1b33d57d2029eb0c8928bbcc/java/maven/src/test/java/net/sf/mmm/code/java/maven/impl/MavenBridgeImplTest.java#L70-L72

I've adjusted it now to fix this PR but it should be checked again as it might fail with other changes.

jan-vcapgemini commented 2 years ago

There is still an issue with the resolveProperties method. Currently the revision of the test project gets overwritten with the version of the main project.

hohwille commented 2 years ago

There is still an issue with the resolveProperties method. Currently the revision of the test project gets overwritten with the version of the main project.

This is not a bug of mmm-code but the same behaviour that maven has on commandline:

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for maven.project 1.0.0-beta7-SNAPSHOT:
[INFO]
[INFO] maven.project ...................................... SUCCESS [  3.663 s]
[INFO] core ............................................... SUCCESS [  6.665 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  11.268 s
[INFO] Finished at: 2021-11-18T08:50:46+01:00
[INFO] ------------------------------------------------------------------------
hohwille commented 2 years ago

However, what confuses me is the "fix" for reading .mvn/maven.config by @maybeec.

https://github.com/m-m-m/code/blob/a990208101354eb6838328183f244867a55ac259/java/maven/src/test/resources/testdata/localmavenproject/maven.project/pom.xml

There is no pom.xml file in its parent folder. So I thought it should not read the .mvn/maven.config. However, it only prevents reading it when there is a pom.xml in the parent folder, but still it otherwise traverses up to the root folder of the filesystem. I thought the only thing for a correct "fix" would have been to stop traversing after the first .mvn/maven.config has been found and read. Aint this the behaviour of the official maven?

IMHO the only real "fix" we would have needed here is to add a return after the .mvn/maven.config has been read and nothing else. This checking for pom.xml and also for parent pom.xml is missleading and has nothing to do with the official behaviour of maven. I vote for chaning this logic once more.

hohwille commented 2 years ago

Mhm, maven's behaviour still seems to be different to what I thought: https://github.com/apache/maven/blob/2c37a7a38e8a4bb2b60e67d3169d4e660f477abf/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java#L393

maybeec commented 2 years ago

Mhm, maven's behaviour still seems to be different to what I thought: https://github.com/apache/maven/blob/2c37a7a38e8a4bb2b60e67d3169d4e660f477abf/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java#L393

That's actually a new behavior of maven 3.6 I think to get to know the root of a multi module project and it's based on the location of the .mvn folder: https://github.com/apache/maven/blob/46410d3df2458af9b2118ace6cc1a68b75c5d262/.mvn/readme.txt So you have a loop in your arguments

hohwille commented 2 years ago

unable to build the release for now:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-gpg-plugin:1.6:sign (sign-artifacts) on project mmm-code-parent: Exit code: 2 -> [Help 1]