search

m-m-m / code

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

Removed M2_Repo to determine local repository #30

Closed maybeec closed 3 years ago

maybeec commented 3 years ago

Currently, M2_REPO is taken as the primary source for determining the maven repository path. Nevertheless, this variable is eclipse specific and has not been standardized by Maven at all. In GitHub Actions this causes trouble determining the correct maven repository path.

I removed it as I don't see the benefit of having it in.

hohwille commented 3 years ago

I disagree and have a different PoV: There is no generic way to find out the location of the local maven repository without completly embedding maven, parsing settings.xml, etc. We so far have 3 ways to guess the local repo location:

  1. Via M2_REPO variable
  2. From CodeSource file path via relative lookup
  3. Via getDefaultLocalRepository() from ~/.m2/repository

I am OK to change the order/priorities but removing 1. is IMHO simply wrong. Option 2. can point to something like ./target/classes what will never lead us to the local maven repo. And Option 3. is a default guess but may also be wrong. After removing 1. that is no chance left for the user to take control if 2. does not work and 3. comes to the wrong result.

Further for 1. I see two options: A. The variable is not set and then it will make no harm. B. The variable is set and then I would strongly assume that the value is pointing to the correct local repo. Why would someone set M2_REPO=banana?

Can you let me know what the value of M2_REPO is in your github action and why this is wrong?

hohwille commented 3 years ago

BTW: A real alternative would be to parse the settings.xml via Maven library and get the local repository properly from the configuration and default fallbacks. However, this will be slower and therefore increase the time to "boostrap".

maybeec commented 3 years ago

I finally implemented the save, but slower solution: https://github.com/devonfw/cobigen/blob/master/cobigen/cobigen-core-api/src/main/java/com/devonfw/cobigen/api/util/MavenUtil.java#L82 and ask maven directly to not need to reimplement the correct mvn repository path in all situations.

maybeec commented 3 years ago

Github Actions runners come with a set of pre defined environment variables. https://www.theserverside.com/blog/Coffee-Talk-Java-News-Stories-and-Opinions/environment-variables-full-list-github-actions

See the values for windows machines, which in my case break the build as the repository used by mvn is different to what is specified in M2_REPO. I was searching for a specification of M2_REPO but it's not at all related to conventions of maven, only relates to eclipse. Therefore I think we definitely need to get it out of first priority in path determination.

hohwille commented 3 years ago

@maybeec thanks for your analysis and feedback. I am still thinking to use maven-core in order to resolve settings so you would not need to run such quirks from outside. Shall we close this PR and I create a new issue for the improvement of reliable repository resolution? Or would you suggest something else to proceed?

maybeec commented 3 years ago

Totally fine, we can close this. If there is a programmatic way of finding the current repository, this definitively would be preferable.