cljdoc / cljdoc-check-action

GitHub Action for checking that CljDoc will be able to analyze the project successfully
Eclipse Public License 2.0
3 stars 0 forks source link

Why not just use the pom from the jar? #4

Open lread opened 1 year ago

lread commented 1 year ago

On Slack, @hlship was wondering why the check action doesn't use the pom.xml within the built jar.

The cljdoc analyzer, when used as a clojure tool, seems to want the pom.xml to also exist in the current dir:

Analyze a deps-based library in the current directory

cd git clone git@github.com:fulcrologic/fulcro.git
cd fulcro
clojure -Tcljdoc analyze-local
# provided ./pom.xml and ./target/*.jar exist

Maybe because technically the jar does not have to contain the pom? But 99% of folks do bundle it in.

Also, these days, a pom.xml, if it exists in a project root, is often a template for the generated pom.xml. So it would not be appropriate.

lread commented 1 year ago

Proposal

Always use pom.xml in the jar if it exists. This covers the typical use case.

When pom.xml is not in jar:

Option 1

Fall back to pom.xml in project root. Log that we are doing this.

Option 2

Force user to specify path to pom.xml. Technically a breaking option.

holyjak commented 1 year ago

I believe it boils down to the fact that I used existing capabilities in the analyzer, and there was (is?) no support for reading the pom file from the jar. This https://github.com/cljdoc/cljdoc-analyzer/blob/f10878e83ca85f758faf21ee5555524394edbac7/src/cljdoc_analyzer/runner.clj#L174 is where cljdoc tries to read the POM.

If we want the analyzer to be able to use the pom in the jar, then we could perhaps change the line to st. like

- pom-str (slurp pompath) 
+ pom-str (or (slurp pompath) (slurp (str  jar-contents-dir "/" pompath))) ; does this break on windows?

Update: Oh, I see it is a little more complicated, since the pom in the jar is at META-INF/maven///pom.xml so we’d need to be smarter at finding it…

lread commented 1 year ago

Thanks for looking into this @holyjak.

If updating the cljdoc-analyzer itself would help, we can do that.

lread commented 1 year ago

Ok @holyjak, I've poked around and thought it over.

  1. We want the check action to mimic cljdoc prod. Cldjoc prod currently always uses the deployed pom outside the jar.

  2. It is unlikely that pom.xml bundled in a jar would be different from a deployed pom.

  3. It is legal for a jar to not contain a pom. A small minority of folks, for whatever reason, strongly prefer this. And cljdoc current supports this use case.

  4. When using tools.build, the generated pom.xml is currently readily available on the file system.

To me, it looks like we have ease-of-use competing with replicating cljdoc production.

So... how about this?

When cljdoc-analyzer is run as a tool the analyze-local command will:

  1. Never default to ./pom.xml, this will trip up too many people who now use this file as a template for their generated pom. Technically a breaking change but given point 2 above, probably not one that folks will notice.
  2. Add a new pompath argument
    1. if specified, then we will NOT look inside the jar for the pom.
    2. else we'll look inside the jar for the pom. If we find one match for /META-INF/maven/$group-id/$artifact-id/pom.xml we'll log that we are using that, else we will fail with a clear message. (We'll match any path dirs for $group-id and $artifact-id and assume they are correct).
  3. Add a new jarpath argument
    1. if specified, use it
    2. else, like today, automatically find the artifact jar under ./target, but different from today, fail with a clear error message if there is more than one match.
  4. The new args will be reflected in the check action

Seem good?

holyjak commented 1 year ago

It does!