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

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

Open
lread opened this issue Mar 30, 2023 · 5 comments
Open

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

lread opened this issue Mar 30, 2023 · 5 comments

Comments

@lread
Copy link
Member

lread commented Mar 30, 2023

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 [email protected]: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
Copy link
Member Author

lread commented Mar 30, 2023

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
Copy link
Collaborator

holyjak commented Mar 31, 2023

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
Copy link
Member Author

lread commented Mar 31, 2023

Thanks for looking into this @holyjak.

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

@lread
Copy link
Member Author

lread commented Mar 31, 2023

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
Copy link
Collaborator

holyjak commented Mar 31, 2023

It does!

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

No branches or pull requests

2 participants