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

5288 Introducing proper deps management and docs about it. #5289

Merged
merged 10 commits into from
Nov 30, 2018

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Nov 7, 2018

As suggested by @pdurbin on IRC (logs), this PR is primarily about the issues found in #5274. This also paves some of the way to #5288, but lets work in smaller chunks.

Please see my comment below. Splitted this up so it does not break AWS on current Glassfish 4.1 environments.


Related Issues

Pull Request Checklist

I could use a hand with testing the S3 stuff in some EC2 or so. Don't have this at hand and a deployment with Minio is currently quite an amount of work.

@coveralls
Copy link

coveralls commented Nov 7, 2018

Coverage Status

Coverage decreased (-0.0009%) to 16.215% when pulling 96ec306 on poikilotherm:5288-dependency-housekeeping into 0ee43fa on IQSS:develop.

@poikilotherm poikilotherm force-pushed the 5288-dependency-housekeeping branch 2 times, most recently from e00b23c to 0990d83 Compare November 14, 2018 15:40
@poikilotherm poikilotherm force-pushed the 5288-dependency-housekeeping branch 4 times, most recently from 2760fad to b5c12b6 Compare November 14, 2018 21:16
@poikilotherm poikilotherm changed the title WIP - 5288 Dependency Housekeeping 5274 Stripping down AWS, solving dependency conflicts Nov 14, 2018
@poikilotherm poikilotherm force-pushed the 5288-dependency-housekeeping branch 2 times, most recently from 23eb913 to 83031f3 Compare November 15, 2018 09:10
Copy link
Contributor

@matthew-a-dunlap matthew-a-dunlap left a comment

Choose a reason for hiding this comment

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

This is great! Both that you were able to deal with our huge aws dependency and the documentation on maven.

A few thoughts:

  • Maybe we should put a note in the top of our pom telling folks to read this doc section before doing edits.
  • I think the "direct dependency" rules section should be its own section. The background info is pretty vital to understanding this part of maven (and I've definitely learned things reading it) but if folks need to refresh themselves its better if its easier to get to.
  • In the rules discussing implicit usage, etc, its probably worth pointing to the helpful tools so folks know what they can do.
  • I feel the lists in Transitive dependencies & managing transitive dependencies overlap somewhat. I'm not sure we need to list all the ways maven can handle them in a list, maybe we can instead add the ways we want to our rules and then discuss the bad ways in a section of text?

Maybe I've missed this in the stories, but I'm curious what you see as the remaining tech debt in our dependency management. We should do our own investigation into maven but if you had some high level thoughts it would help us move forward.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I'm approving this but I'd be happy to fix a typo here and there once @dlmurphy is done making commits. Or we can merge it and fix the typos later.

@dlmurphy
Copy link
Contributor

I'm finished making commits, all looks good to me!

`adding options to the make call <https://groups.google.com/forum/#!topic/sphinx-users/yXgNey_0M3I>`_.

On Mac and Linux this should "just work", on Windows this is currently untested. The worst thing that might happen
is a warning and missing images in your local documentation build.
This has been tested and works on Mac, Linux, and Windows. If you have not properly configured GraphViz, then the worst thing that might happen is a warning and missing images in your local documentation build.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Good to know this works on a Windows box 😄

in the model. You might end up using legacy or repackaged classes because of a wrong scope.
.. [#ide2] This is going to bite back in modern IDEs when importing classes from transitive dependencies by "autocompletion accident".

.. rubric:: Footnotes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for adding this - I tried to find other examples, but didn't find one.

@poikilotherm
Copy link
Contributor Author

@matthew-a-dunlap

Maybe we should put a note in the top of our pom telling folks to read this doc section before doing edits.

Sure, good idea! Maybe that's even better than my explanation comments. Do feel like adding a commit for this?

I think the "direct dependency" rules section should be its own section. The background info is pretty vital to understanding this part of maven (and I've definitely learned things reading it) but if folks need to refresh themselves its better if its easier to get to.

I am not opinionated on this. @dlmurphy what do you think?

In the rules discussing implicit usage, etc, its probably worth pointing to the helpful tools so folks know what they can do.

Sure, good idea. Feel free to add a commit 👍

I feel the lists in Transitive dependencies & managing transitive dependencies overlap somewhat. I'm not sure we need to list all the ways maven can handle them in a list, maybe we can instead add the ways we want to our rules and then discuss the bad ways in a section of text?

The exclude option is currently in use within the real POM, but I wasn't in the mood to add more docs about that, as IMHO this is last resort stuff. IMHO a developer might enjoy a short overview, which is not easy to get from the detailed blogs and docs out there (took me a while to sort this out... 😉 )

I'm curious what you see as the remaining tech debt in our dependency management.

Let's discuss that in #5288, alright? Maybe the things I collected over there are an inspiration already 😉

@pdurbin
Copy link
Member

pdurbin commented Nov 16, 2018

Do feel like adding a commit for this?

Done in 5b49f50 (in pom.xml direct people to new dependencies page)

@poikilotherm poikilotherm force-pushed the 5288-dependency-housekeeping branch 3 times, most recently from 53e055f to 17648f2 Compare November 21, 2018 13:57
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Nov 21, 2018

As @kcondon said that this would have broken current setups based on Glassfish 4.1, I once more changed this PR to only address the dep management stuff. Thus reassigning this to #5288 again and hope this can be merged then.

EDIT: Please be aware that I force pushed things! I removed the commit that dealt with the TrueZIP stuff and changed the AWS dep, so this is now just about introducing dep management.

@poikilotherm poikilotherm changed the title 5274 Stripping down AWS, solving dependency conflicts 5288 Introducing proper deps management and docs about it. Nov 21, 2018
Copy link
Contributor

@matthew-a-dunlap matthew-a-dunlap left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for splitting off the AWS work

poikilotherm and others added 8 commits November 29, 2018 15:11
As JaCoCo has been a direct dependency in *compile* scope.

At a few places accidentially some classes from transitive dependencies from JaCoCo have been used
instead of the classes used commonly within the codebase.

Removing the JaCoCo direct dependency as only the plugin is used at build/test time. It is sufficient for Maven to have the
plugin included with its version number - this is handled as a transitive dependency during the build internally by Maven.

Hopefully this created no harm, but better fix this now.
Needed because the transitive dependency from AWS has been used before.
Thus this relates to IQSS#5274. As best practise, code should not rely on
those deps but add a direct dependency.

For the sake of working on IQSS#4260 (Java EE 8) at some point in the future, please
remember to refactor the code using Jackson (and Gson) and remove it
in favor of Java EE 8 native JSON-B and JSON-P support.
…follow.

Added a note about GraphViz requirement for building the docs.
Added that GraphViz works with Windows.
Made some typo fixes and such. Looks good!
Fixed a syntax problem that was throwing up sphinx errors.
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.

6 participants