Dependency Housekeeping and how developers will benefit from it

poikilotherm commented 5 years ago

This issue is a story from or a blocker to #5292. This issue is related to #4260, #5274, #3921, #4172 and more. Please feel addressed @matthew-a-dunlap and @scolapasta :wink:

While working on #4172 I discovered the problems noted down in #5274, which are in turn a left-over of #3921. This lead me to the impression, that the current pom.xml could use some love in terms of dependency management, hopefully avoiding more problems down the road. More and more deps keep kicking in while functionality is added to Dataverse. (As @pdurbin said on IRC: Dataverse 4.0 was around 45MB, 4.9.4 is at 146 MB)

I made some first steps primarily to address #5274, but this might be a could starting point for more work in this. Reducing WAR size, find wrong imports, update dependencies, address convergence issues and more...

Done so far (mostly for the resolution of #5274)

[x] Add initial dependency management for this via BOMs
[x] Add direct dependencies to Jackson as it is in use within the code.
[x] Remove JaCoCo compile scope dependency and fix resulting import errors from transitive deps not present anymore.
[x] Add checksums to the local_lib JARs to make Maven happier
[x] Write usefull docs (and rules?) about dependency management for old and new Dataverse developers (placed in a new section within the dev docs)

Things open (and will stay open till we are on Glassfish 5+, see #5274)

[ ] Remove AWS SDK bundle and replace with "S3 only"
[ ] Test the S3 stuff against some server to be sure it works, as no integration tests are present (see #5068 and #4690)

Things to consider

[ ] Think and talk about moving local_lib into a GitHub based "poor mans" Maven repo.
[ ] Cleanup direct and transitive deps for the SWORDv2 code
[ ] Think and talk about adding Maven Enforcer as a plugin permanently, maybe add it to the build cycle and fail builds on convergence issues.
[ ] Address more/all conflicts from mvn enforcer:enforce
[ ] Address issues from mvn dependency:analyze
[ ] Think about replacing the retired Apache Abdera library with sth. like ROME - Note: ROME is used by Tika and included anyway.

poikilotherm commented 5 years ago

Please note that #5289 is not solving all of the aspects of this story. Those still need to be discussed. Ping to @matthew-a-dunlap and @scolapasta.

pdurbin commented 5 years ago

Over at http://irclog.iq.harvard.edu/dataverse/2018-11-27#i_80014 @poikilotherm let me know that he removed the commit that he believes was causing the exception @kcondon found at https://github.com/IQSS/dataverse/issues/5274#issuecomment-439496421 reported as "I'm able to build and use somewhat the resulting war file but it throws a stack trace about a missing method when I load the homepage and an exception (not logged) when clicking on a dataset card to view the dataset page."

@poikilotherm indicated that the change he made was the following:

"I removed the commit that removed the TrueZIP stuff and removed the commit that was changing the AWS dependency"

I moved this issue to code review at https://waffle.io/IQSS/dataverse

poikilotherm commented 5 years ago

@kcondon, thx for merging #5289.

As that PR adressed only a part of this issue, could you please reopen? Thx!

pdurbin commented 5 years ago

@poikilotherm I hate to say this, but I'd love for you to adjust a bit more to our process. "Small chunks" is something you'll hear us say all the time. "Incremental value." Agile, kanban, whatever. Every team has their own spin.

First of all, thank you very much for this pull request! We are close to a release, I think. Only one more issue to go. After we cut a release, we'll want to indicate which issues were addressed in that release. I would like this issue to remain closed and have a clear title of what was delivered. What I don't want is the confusion of release note pointing to this issue with a status of "open" and a check list of what's done and what's not done. "Definition of done" is something you'll hear us say a lot too. It's fine to create "epic" issues for organizational purposes but before an issue goes into dev, code review, and especially QA, the "definition of done" should be clear enough that it can be closed. I hope this is making sense.

Something else to think about is that buried in the pull request that was just merged is a new ability to use graphviz in the guides. However, there is no issue representing this improvement, nothing to link to from release notes that describes this new feature. Dataverse installations who build their own guides may or may not notice warnings after the next release. This would have been a good issue to split into a "small chunk".

I'm sure it's annoying to have to adjust to the process of this or that team and probably we should document more of the process so that people are surprised by how we operate.

I can't emphasize enough how much I appreciate the energy you've put into Dataverse already. I'm just trying to smooth things out in terms of how issues and pull requests make their way through QA.

To summarize:

small chunks
definition of done
incremental progress
thank you!!!

I hope this helps. :smile:

poikilotherm commented 5 years ago

Sorry to hear that @pdurbin . I am really trying to stick to your flow, but it seems I still need to adopt IQSS sense of "how small is small".

I am fine with leaving this closed if that is more inline with your procedure. I will just open a successor issue then, so the thoughts and ideas above don't get lost.

Would it make you feel better, if I open a separate issue for that Graphviz stuff plus make a new PR adding some stuff to release notes?

pdurbin commented 5 years ago

@poikilotherm thanks for opening #5360 as a successor issue!

IQSS / dataverse