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

documentation: add documentation for buildMaven #100660

Merged
merged 1 commit into from Nov 3, 2020

Conversation

@fzakaria
Copy link
Contributor

@fzakaria fzakaria commented Oct 15, 2020

Motivation for this change

Add nice markdown documentation for how to use mvn2nix plugin and the
buildMaven function within nixpkgs.

The goal is to have a thorough documentation similar to that of other languages.
ex. https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/ruby.section.md

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

CC @jerith666 @volth @zimbatm @charles-dyfis-net

@fzakaria fzakaria changed the title documentation: add documentation for maven documentation: add documentation for buildMaven Oct 15, 2020
@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from c439d08 to ee91f64 Oct 15, 2020
@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from ee91f64 to ffbb3a8 Oct 15, 2020
@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from ffbb3a8 to b424e0a Oct 15, 2020
Copy link
Contributor

@doronbehar doronbehar left a comment

A few comments:

  1. Double invocation

    • You mention twice that subsequent runs my result in different output hashes. If our situation is so awful, putting it in the docs is a kind of "recommendation" to keep using this method, so perhaps it'll be better not to document this?
    • Why do you use src = ./.;? I thought this is a guide that teaches people how to contribute java packages to Nixpkgs.
  2. buildMaven

    • It should be made clear that a Nixpkgs contributor needs to download the project's source code, and run mvn org... in the project's root directory, or where ever the mvn entry point file (pom.xml?) is.
    • You kind of mix the "build" sections.
  3. General

    • I'd start this document by writing that the derivation one would callPackage to should look like the derivation in the end.
    • Isn't pom.xml a file that all maven projects need?
    • So what build.repo returns is the value repository should get? (If the answer is yes, then you should make it clearer :))
    • Perhaps it'd be better to just give two full examples of using buildMaven and using the double invocation (if we'd decide it's good to even document it).

Oh and I know you hate docbook, so am I. But, there's 1 advantage in docbook that's not available in our (current) markdown format: Take a look here:

vendorSha256 = "1879j77k96684wi554rkjxydrj8g3hpp0kvxz03sd8dmwr3lh83j"; <co xml:id='ex-buildGoModule-1' />
runVend = true; <co xml:id='ex-buildGoModule-2' />
meta = with lib; {
description = "Simple command-line snippet manager, written in Go";
homepage = "https://github.com/knqyf263/pet";
license = licenses.mit;
maintainers = with maintainers; [ kalbasit ];
platforms = platforms.linux ++ platforms.darwin;
};
}
</programlisting>
</example>
<para>
<xref linkend='ex-buildGoModule'/> is an example expression using buildGoModule, the following arguments are of special significance to the function:
<calloutlist>
<callout arearefs='ex-buildGoModule-1'>
<para>
<varname>vendorSha256</varname> is the hash of the output of the intermediate fetcher derivation.
</para>
</callout>
<callout arearefs='ex-buildGoModule-2'>
<para>
<varname>runVend</varname> runs the vend command to generate the vendor directory. This is useful if your code depends on c code and go mod tidy does not include the needed sources to build.
</para>
</callout>
</calloutlist>
</para>

And here: https://nixos.org/manual/nixpkgs/unstable/#ex-buildGoModule-1

Docbook can link lines in source code to documentation, which is really nice.

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
version = "0.1";
name = "${pname}-${version}";
src = ./.;
buildInputs = [ jdk maven makeWrapper ];
Copy link
Contributor

@doronbehar doronbehar Oct 16, 2020

Technically they aren't needed as they are referenced thanks to the makeWrapper call.

Suggested change
buildInputs = [ jdk maven makeWrapper ];
buildInputs = [ makeWrapper ];

Copy link
Member

@roberth roberth Oct 16, 2020

We're invoking mvn in via PATH in buildPhase though. I figure a jdk command is also necessary for mvn compile

Copy link
Contributor Author

@fzakaria fzakaria Oct 16, 2020

I'll add a section on "Choosing the correct JDK"
The current mvn derivation allows overriding JAVA_HOME

❯ cat /nix/store/2hhmmj0vbb5d181nfx2mx3p7k54q44ij-apache-maven-3.6.3/bin/mvn

#! /nix/store/6737cq9nvp4k5r70qcgf61004r0l2g3v-bash-4.4-p23/bin/bash -e
export JAVA_HOME=${JAVA_HOME-'/nix/store/knh4n8fzrslyzgrw6p8nh5v07s0ycznc-openjdk-8u265-ga'}
exec "/nix/store/2hhmmj0vbb5d181nfx2mx3p7k54q44ij-apache-maven-3.6.3/maven/bin/mvn"  "$@"

What i've done in the past is just make sure to set JAVA_HOME to the correct JDK (i.e. JDK8 vs JDK11).
An alternative approach would also be to overrideAttrs

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 16, 2020

@doronbehar thank you for your in depth review -- I will incorporate most/all of your feedback.

For now, I will iterate using markdown purely due to my familiarity. I want to separate the issue of content vs. problems with how I use markup. A pragmatic approach may be to follow-up with another pull-request to convert it to docbook once the content is solid.

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
@roberth
Copy link
Member

@roberth roberth commented Oct 16, 2020

Looking already pretty good. Feel free to link to your own project.

@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch 2 times, most recently from 1a340ab to e279c12 Oct 16, 2020
@fzakaria fzakaria requested review from roberth, doronbehar and volth Oct 16, 2020
@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 16, 2020

Thanks for the feedback everyone (@roberth & @doronbehar)
I have made the documentation much more explicit by going through an example Maven project with a dependency.
I demonstrate building the Maven repository using two strategies, building the JAR for use as a library & wrapping the JAR for it to be runnable.

If the documentation is pretty close to the mark, I'd like to be able to submit it and continue to iterate on it.
Further improvements (docbook or smaller nits) can be done in a follow-up I hope.

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
Copy link
Contributor

@doronbehar doronbehar left a comment

It's getting better. But:

  • Again, I tend to think we should not document the "double invocation" method.. But even if we do, it should be mentioned after mvn2nix-maven-plugin.
  • Again, it would be easier IMO to understand it all, if the 1st expression the reader will see, is an expression of a derivation that uses buildMaven, in the let repository = .

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from e279c12 to 40a3228 Oct 19, 2020
@fzakaria fzakaria requested review from roberth and doronbehar Oct 19, 2020
@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 19, 2020

Thanks again for the suggestions.
I applied them directly mostly and reworked it according to the feedback @roberth & @doronbehar

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
@fzakaria fzakaria requested a review from roberth Oct 19, 2020
@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from 0c3be23 to 49b7bee Oct 19, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 21, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-0072-fcp-switch-to-commonmark-for-documentation/9560/4

Copy link
Contributor

@doronbehar doronbehar left a comment

I notice you use a lot _ instead of a ``` where I'd use the later. Why?

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
We will use the same repository we built above (either _double invocation_ or _buildMaven_) to setup a CLASSPATH for our JAR.
### CLASSPATH
Copy link
Contributor

@doronbehar doronbehar Oct 22, 2020

I'm a little bit confused about this section. In the derivation of it you create a wrapper, and that calls Main as it seems. What's the difference between this and the last example expression?

Copy link
Contributor Author

@fzakaria fzakaria Oct 22, 2020

So fundamentally, your Java invocation needs to have all dependencies on the CLASSPATH.

CLASSPATH is the search path Java uses to lookup classes

The CLASSPATH can be set explicitly within a JAR which the subsequent section does through the help of Maven via the plugin via the MANIFEST file.

Here is the contents of the JAR and we see the CLASSPATH is set and expected at a fixed location relative to the JAR.

❯ unzip -q -c /nix/store/8d4c3ibw8ynsn01ibhyqmc1zhzz75s26-maven-demo-1.0/share/java/maven-demo-1.0.jar
Manifest-Version: 1.0
Archiver-Version: Plexus Archiver
Built-By: nixbld
Class-Path: . ../../repository/com/vdurmont/emoji-java/5.1.1/emoji-jav
 a-5.1.1.jar ../../repository/org/json/json/20170516/json-20170516.jar
Created-By: Apache Maven 3.6.3
Build-Jdk: 1.8.0_265
Main-Class: Main

The first approach though instead builds the JAR normally and then augments the makeWrapper to set the CLASSPATH via the command line.

You've given feedback on providing context on which is appropriate which I will do.

Fundamentally, if you are a nixpkgs author wrapping another project, the method by setting the CLASSPATH via makeWrapper is preferable since it's a bit more of a pain to create a patch file to change the project's pom.xml

If you are a project writer and using nix-shell or nix-build to build your project, it would be easier for you to modify your pom to add the required plugin.

Copy link
Contributor

@doronbehar doronbehar Oct 23, 2020

I don't read your comments, as I expect these explanations to be in the documentation.

}
```
### MANIFEST file via Maven Plugin
Copy link
Contributor

@doronbehar doronbehar Oct 22, 2020

Is this a necessity to the above? It should be made clear in general, whether sections are targeted towards developers of a shell.nix with maven, or casual contributors to Nixpkgs that just want to add a Java program to Nixpkgs, and compile it from source. Hence, I don't understand in what case one should need to "augment" the pom.xml file.

Copy link
Contributor Author

@fzakaria fzakaria Oct 22, 2020

I added such sections.

{ pkgs ? import <nixpkgs> { }, stdenv ? pkgs.stdenv, lib ? pkgs.lib
, maven ? pkgs.maven, callPackage ? pkgs.callPackage
, makeWrapper ? pkgs.makeWrapper, jre ? pkgs.jre }:
Copy link
Contributor

@doronbehar doronbehar Oct 22, 2020

If it'll be clearer when a nix expression is targeted at contributors or shell.nix writers, I should tell you when to remove these ? defaults, since in many cases these are not recommended as callPackage takes care of these defaults.

Copy link
Contributor Author

@fzakaria fzakaria Oct 22, 2020

These defaults exist because for the purpose of demonstration each file (https://github.com/fzakaria/nixos-maven-example) is individually runnable via nix-build

mkdir -p $out/share/java
# create a symbolic link for the repository directory
ln -s ${repository} $out/repository
Copy link
Contributor

@doronbehar doronbehar Oct 22, 2020

Why is this needed?

Copy link
Contributor Author

@fzakaria fzakaria Oct 22, 2020

Please see the comment above about the CLASSPATH value in the jar's MANIFEST file.

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 22, 2020

I notice you use a lot _ instead of a ``` where I'd use the later. Why?

I think you mean *; it might be a habit I picked up from running prettier (an opinionated formatter).

Copy link
Contributor Author

@fzakaria fzakaria left a comment

(Oops, I did a bunch of single comments instead of review ; sorry)

mkdir -p $out/share/java
# create a symbolic link for the repository directory
ln -s ${repository} $out/repository
Copy link
Contributor Author

@fzakaria fzakaria Oct 22, 2020

Please see the comment above about the CLASSPATH value in the jar's MANIFEST file.

@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from 159cf98 to a812da3 Oct 22, 2020
@fzakaria fzakaria requested a review from doronbehar Oct 22, 2020
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 23, 2020

I notice you use a lot _ instead of a ``` where I'd use the later. Why?

I think you mean *; it might be a habit I picked up from running prettier (an opinionated formatter).

No, I meant ``` - i.e code keywords like `buildMaven`.

doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.md Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 23, 2020

@fzakaria I hope my review isn't to harsh :). I just want to make it clear for everyone in the future. Thanks for doing this anyway!

@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from b05139d to dcda7ad Oct 26, 2020
@fzakaria fzakaria requested a review from doronbehar Oct 26, 2020
@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Oct 26, 2020

@doronbehar thank you for the review.

I'm hoping we can get this in soon; it's seen quite a few revisions. Improvements can continue to happen in subsequent pull-requests. It's definitely a place that will help others who want to use Java + Nix.

The documentation is pretty thorough but does expect minimal understanding of how Java works which I consider acceptable since each language-support documentation isn't meant to expect 0 understanding of the language it's building upon.
(This is with respect to the portion involving CLASSPATH)

I'm happy to explain things 1:1 over some comments but I would like to push back a little on expecting 0 knowledge in the documentation.

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Nov 2, 2020

@roberth @doronbehar ping 👍

@roberth
Copy link
Member

@roberth roberth commented Nov 2, 2020

Content looks good, but it isn't incorporated into the manual yet.
Seems like you'll need to rename it to maven.section.md so make can write the xml file

%.section.xml: %.section.md

and reference it.

<xi:include href="lua.section.xml" />

Building the manual is documented here https://nixos.org/manual/nixos/stable/index.html#sec-writing-docs-building-the-manual

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Nov 2, 2020

@roberth I didn't even know that's how it works -- thanks for teaching me 👍

I've made the changes you requested and confirmed it works.
image

@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from 914e0da to 3ae7bac Nov 2, 2020
Copy link
Contributor

@doronbehar doronbehar left a comment

Content really looks great now! Thanks @fzakaria 🎉.

Only some slight formatting nitpicks, if you don't mind :).

doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/maven.section.md Outdated Show resolved Hide resolved
Add nice markdown documentation for how to use mvn2nix plugin and the
buildMaven function within nixpkgs.

Update doc/languages-frameworks/maven.md

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

Apply suggestions from code review

Co-authored-by: Doron Behar <doron.behar@gmail.com>

Apply suggestions from code review

Co-authored-by: Doron Behar <doron.behar@gmail.com>

Apply suggestions from code review

Co-authored-by: Doron Behar <doron.behar@gmail.com>
@fzakaria fzakaria force-pushed the faridzakaria/maven-documentation branch from e92f82c to b9321ad Nov 3, 2020
@fzakaria fzakaria requested a review from doronbehar Nov 3, 2020
roberth
roberth approved these changes Nov 3, 2020
Copy link
Member

@roberth roberth left a comment

🚀

@doronbehar doronbehar merged commit 80b96cf into NixOS:master Nov 3, 2020
16 checks passed
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Nov 3, 2020

Thanks again @fzakaria 🐳 .

@fzakaria
Copy link
Contributor Author

@fzakaria fzakaria commented Nov 4, 2020

We did it!

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 4, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/maintained-usable-tooling-for-building-clojure-projects-in-nix/1556/6

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 24, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/gnu-guix-1-2-0-released-2020-blog-gnu-guix/10167/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants