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

Switch to Non-Maven Jar plugin to vendor jars #206

Merged
merged 3 commits into from May 6, 2020

Conversation

stephenc
Copy link
Contributor

@stephenc stephenc commented May 5, 2020

  • This enables anyone to checkout and build the project without having to mess around with installing jar files

NOTE: I know when I wrote the non-maven jar maven plugin I included a warning about "use of the plugin acknowledging bad citizen, etc" but that is more reflecting that we (The Apache Maven PMC) want to nudge people towards publishing on Central... which is also why I published the plugin from my personal account rather than releasing it from the Apache Maven project itself. However, compared to what you were doing with the lib folders, this is - as a member of the Apache Maven PMC - an improvement.

- This enables anyone to checkout and build the project without having to mess around with installing jar files
@stephenc
Copy link
Contributor Author

stephenc commented May 5, 2020

Here's the details on the plugin I wrote a while back: https://github.com/stephenc/non-maven-jar-maven-plugin

@ggrossetie
Copy link
Member

Thanks @stephenc !
For reference, on another project, I ended up creating a local maven repository with mvn install:install-file.

mvn install:install-file -Dfile=src/main/lib/bigquery-jdbc/error_prone_annotations-2.1.3.jar -DgroupId=org.jdbc.bigquery -DartifactId=error_prone_annotations -Dversion=2.1.3 -Dpackaging=jar -DlocalRepositoryPath=src/main/lib -DcreateChecksum=true

And then, I added the local repository in my pom.xml:

<repository>
  <id>lib</id>
  <url>file://${project.basedir}/lib</url>
</repository>

It's a bit hacky because you have to commit a local maven repository but it's working... 😬
Out of curiosity, what is your opinion on this solution?

However, compared to what you were doing with the lib folders, this is - as a member of the Apache Maven PMC - an improvement.

It's definitely an improvement, and the changes are minimal so 👍

@stephenc
Copy link
Contributor Author

stephenc commented May 5, 2020

<repository>
  <id>lib</id>
  <url>file://${project.basedir}/lib</url>
</repository>

is also bad because most corporate users will have <mirrorOf>*</mirrorOf> in their settings.xml so that will nor work for them either. Hence why non-maven-jav is the least worst solution if you cannot get them into central

<artifactId>ditaa</artifactId>
<version>${ditaa.version}</version>
<version>${project.version}</version>
Copy link
Member

@ggrossetie ggrossetie May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to keep the version of the jar? If not I guess that we can safely remove the reference in the <properties> right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not if you want to use the Maven release tooling...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure there's a good reason but I'm not sure I understand why... we are using a "released" version no? Sorry, I'm sure I'm not using the correct terminology but I would expect this dependency to be resolved as a "standard" dependency.
But I guess the thirdparty/ditaa/pom.xml is not really the pom of the dependency but the pom of the project that contains a jar. So we cannot define <version>1.3.13</version> in thirdparty/ditaa/pom.xml and use it in here?

Anyway, I think it's fine, I'm mainly asking to learn 🤓

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you get the release plugin automating the version number bumps you’ll need all the poms to align their version number.

If you’re not using a release plugin and not using -SNAPSHOT versions on master then you could have different versions

@stephenc
Copy link
Contributor Author

stephenc commented May 6, 2020

Anything blocking merging this?

@ggrossetie
Copy link
Member

Anything blocking merging this?

@stephenc No it's looking good but I want to checkout the branch locally and play with it before merging 😄

@stephenc
Copy link
Contributor Author

stephenc commented May 6, 2020

For some IDEs you may need to use mvn install to have the 3rd party vendored libraries available in the IDE

Copy link
Member

@ggrossetie ggrossetie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, it's working just fine 👌

@ggrossetie ggrossetie merged commit 71232f0 into yuzutech:master May 6, 2020
@ggrossetie
Copy link
Member

ggrossetie commented May 9, 2020

For some IDEs you may need to use mvn install to have the 3rd party vendored libraries available in the IDE

@stephenc I spoke too soon, it's not working on Intellij IDEA... even if I do mvn install. Not sure what to do, but if it's not working I might revert this commit because that's a pretty big issue 😞

Edit: Manually adding the dependency on "Project Structure..." works but not ideal. The dependencies look fine but it looks like the non-maven jars are ignored by Intellij.

dep-umlet

@stephenc
Copy link
Contributor Author

stephenc commented May 9, 2020

Strange. Works for me in IntelliJ. I’ll wipe my cache and try again

@ggrossetie
Copy link
Member

Very strange indeed... I'm using:

IntelliJ IDEA 2019.3.4 (Ultimate Edition)
Build #IU-193.6911.18, built on March 17, 2020
Runtime version: 11.0.6+8-b520.43 amd64
VM: OpenJDK 64-Bit Server VM by JetBrains s.r.o
Linux 4.15.0-99-generic
GC: ParNew, ConcurrentMarkSweep
Memory: 1981M
Cores: 8
Registry: 
Non-Bundled Plugins: BashSupport, Batch Scripts Support, CMD Support, CheckStyle-IDEA, FindBugs-IDEA, JFormDesigner, com.intellij.ideolog, com.wix.eslint, org.asciidoctor.intellij.asciidoc, org.jetbrains.idea.grammar

I will try with the latest version...

@ggrossetie
Copy link
Member

@stephenc could you please confirm that it's working fine after the cache has been wiped? If so, could you please copy/paste your Intellij version? Thanks!

@stephenc
Copy link
Contributor Author

@Mogztter I am sorry, IntelliJ seems to have taken a step back in how they parse Maven project models. I may need to rework some hacks into the non-maven jar. For now just revert this commit and I'll re-submit the PR once I find some way to get IDEA working again...

The TL;DR https://youtrack.jetbrains.com/issue/IDEA-175538 is probably the issue here

@stephenc
Copy link
Contributor Author

When I reported https://youtrack.jetbrains.com/issue/IDEA-175538 everything worked in IDEA as long as you didn't add <type>non-maven-jar</type> to your dependencies... and that bug was about supporting the custom type... seems now they broke things even more that not even the default implicit <type>jar</type> works any more
sad panda2

@stephenc
Copy link
Contributor Author

A workaround is to run mvn install and then open only server/pom.xml in IDEA as that will force it not to see these side projects and resolve from the maven repo

@stephenc
Copy link
Contributor Author

Oooooh.... we could have a profile that is active unless maven is being parsed by intellij... that profile could add the 3rd party jar files

@stephenc
Copy link
Contributor Author

Something like

  <profiles>
    <profile>
      <id>hide-thirdparty-modules-from-IDEA</id>
      <activation>
        <property>
          <name>!idea.version</name>
        </property>
      </activation>
      <modules>
        <module>thirdparty</module>
      </modules>
    </profile>
  </profiles>

Though IDEA seems not to be correctly parsing profile activation either and is incorrectly enabling that profile... But at least putting third party in a profile allows you to deactivate the profile and get IDEA working again (after mvn install)

Screenshot 2020-05-10 at 11 38 30

@stephenc
Copy link
Contributor Author

Ok IDEA's pom parsing no longer exposes idea.version.... may need a new trick to detect IDEA's pom parser... I'll get during the week

@ggrossetie
Copy link
Member

Damn!

Though IDEA seems not to be correctly parsing profile activation either and is incorrectly enabling that profile... But at least putting third party in a profile allows you to deactivate the profile and get IDEA working again (after mvn install)

That could work!
I'm fine with the initial mvn install.

For now just revert this commit and I'll re-submit the PR once I find some way to get IDEA working again...

Ok.

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

Successfully merging this pull request may close these issues.

None yet

2 participants