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

Dependencies #3906

Closed
wants to merge 8 commits into from
Closed

Dependencies #3906

wants to merge 8 commits into from

Conversation

stefan-kolb
Copy link
Member

#3897

I sorted some of the dependencies in build.gradle.
Some dependencies are grouped by context, e.g., JavaFX.
I think however that we need to sort all dependencies alphabetically otherwise synching the external-libs.txt with the dependencies is impossible in my opinion. @JabRef/developers WDYT?
We might add some comments for deps that do not reveal their usage via their name.

@stefan-kolb stefan-kolb requested a review from koppor March 30, 2018 18:53
@stefan-kolb stefan-kolb added the type: code-quality Issues related to code or architecture decisions label Mar 30, 2018
@Siedlerchr
Copy link
Member

I thought it would be a nice idea to auto-generate this file and fortunately someone created a gradle plugin:
https://github.com/jk1/Gradle-License-Report

@lenhard
Copy link
Member

lenhard commented Apr 1, 2018

Alphabetical sorting will more or less result in a context-sorting anyway, won't it? After all, the libraries have a url-style naming pattern and we can just add blank lines here and there to make the context stand out.

@stefan-kolb
Copy link
Member Author

stefan-kolb commented Apr 1, 2018

In many cases yes. For a lot of JavaFX libraries unfortunately, no. I kept logging and JavaFX grouped for now. But this makes no real sense if we want alphabetical sorting in the future.

@Siedlerchr
Copy link
Member

Why not using that plug in I suggested?
Iz seems to auto generate such a list of dependencies to a file. We then need only invoke it then when deps changes
https://github.com/jk1/Gradle-License-Report

@stefan-kolb
Copy link
Member Author

Have not tried it out yet.

@koppor
Copy link
Member

koppor commented Apr 2, 2018

I added it. Run following command

./gradlew generateLicenseReport

Afterwards, find following files:

  • build/reports/dependency-license/index.html
  • build/reports/dependency-license/THIRD-PARTY-NOTICES.txt

Excerpt:


Dependency License Report for JabRef

Dependency License Report for JabRef 4.2-dev

1. Group: com.airhacks  Name: afterburner.fx  Version: 1.7.0

POM Project URL: http://afterburner.adam-bien.com

POM License: The Apache Software License, Version 2.0 - http://www.apache.org/licenses/LICENSE-2.0.txt

--------------------------------------------------------------------------------

2. Group: com.github.bkromhout  Name: java-diff-utils  Version: 2.1.1

--------------------------------------------------------------------------------

3. Group: com.github.tomtung  Name: latex2unicode_2.12  Version: 0.2.2

POM Project URL: https://github.com/tomtung/latex2unicode

POM License: Apache 2 - http://www.apache.org/licenses/LICENSE-2.0.txt

grafik

When it comes to embedded licenses, the text renderer is not that good:

--------------------------------------------------------------------------------

15. Group: com.microsoft.azure  Name: applicationinsights-core  Version: 2.0.2

POM License: The Apache Software License, Version 2.0 - http://www.apache.org/licenses/LICENSE-2.0.txt

Embedded license: 

                    ****************************************                    


                                 Apache License
                           Version 2.0, January 2004
                        http://www.apache.org/licenses/

   TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION

The HTML one is better in this case:

grafik

Since, the tool does not use SPDX License Identifiers, we need to make a PR to the tool proposing to use the accepted standard for identifying licenses.

We also need to provide an importer for unknown licenses - or we need to contact these authors and ask for a proper license in the POM.

grafik

Comparison

Field external-libraries.txt Gradle-License-Report
Id Y Y
Project Y -
Version Y Y
Project Y -
URL Y only-if-embedded-in-the-POM
License Y y
Note Y -

That means, we skip the human readable project name (which is OK) and the project-URL (in most cases).

Sometimes, we have different project URLs:

Id:      de.undercouch.citeproc-java
Project: Citeproc-Java
URL:     http://michel-kraemer.github.io/citeproc-java/
Licence: Apache-2.0

vs.

25. Group: de.undercouch  Name: citeproc-java  Version: 1.0.1

POM Project URL: http://www.michel-kraemer.com

POM License: The Apache Software License, Version 2.0 - http://www.apache.org/licenses/LICENSE-2.0.txt

So, in summary, I think, it is OK to include the dependency, but we should use it only as cross-check for external-libraries.txt.

@koppor
Copy link
Member

koppor commented Apr 2, 2018

I updated external-libraries.txt and an initial ADR-0003 reflecting this discussion.

@koppor
Copy link
Member

koppor commented Apr 2, 2018

I grouped external-libraries.txt according to the groups of build.gradle (and added the group "Citation Style Language"). I removed all testing dependencies from external-libraries.txt as testing dependencies are not required to be re-packaged by linux distributions.

I also added following sections to external-libraries.txt:

## Transitive dependencies

## Dependencies included as JARs in the source tree

compile 'org.apache.pdfbox:fontbox:2.0.9'
compile 'org.apache.pdfbox:xmpbox:2.0.9'

// required for reading write-protected PDFs - see https://github.com/JabRef/jabref/pull/942#issuecomment-209252635
Copy link
Member

Choose a reason for hiding this comment

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

Should I convert the comment to an ADR and link it (in a comment // see ADR-0004)? I added that comment, because someone might wonder why bouncycastle is integrated.

@stefan-kolb
Copy link
Member Author

Hm, so it doesn't look like we can make any use of the plugin right now? So I propose to remove it and sort all dependencies alphabetically. And keep the manual workflow for now.

@koppor
Copy link
Member

koppor commented Apr 3, 2018

I like the grouping very much, because it eases packaging. Especially in the context of Debian (refs koppor#135).

It is also helpful to have the plugin included in build.gradle so that maintainers working on external-libraries.txt don't have to modify build.gradle when working on that.

@stefan-kolb
Copy link
Member Author

Probably need to base this on the maintable-beta branch if we want to merge it. Otherwise, we end up with a lot of conflicts there.

@koppor
Copy link
Member

koppor commented Jun 29, 2018

Decision: cherry-pick adr-003 into a new PR. Remainder: Leave as is.

@koppor koppor deleted the dependencies branch June 29, 2018 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants