patrickzib/SFA

Moving to maven

mohataher opened this issue · 12 comments

mohataher commented

Nice work. In order to get this code standardised, why not using Maven? It's the most acceptable packaging, build and automation tool for Java. It will also help with wiring unit test cases for the project.

I'll be happy to help ;)

ChristianSch commented

I already did the heavy lifting in my fork (using gradle though). Would be awesome to have that upstream!

mohataher commented

Just had a look. It's a good start. I could happily contribute to yours until test cases mature before pulling it to Patrick's main repo.

What are you your thoughts @patrickzib ?

patrickzib commented

thanks a lot. I am not very familiar with maven nor gradle. Is gradle or maven the better tool to use?

really cool, @ChristianSch thank you. I just had a look at your fork. I will try to port the changes for unittests, hppc, etc. into the master.

mohataher commented

Yes, Patrick. Gradle and Maven are widely more accepted than just Ant build. It makes managing dependencies, packaging and deployment a lot easier for everyone. Porting @ChristianSch is a good start. Test cases need more work and I will be happy to help with that.

patrickzib commented

@mohataher: Sure, I would be very happy to get help with test cases!

Ok, i have moved to gradle based on @ChristianSch fork. thanks a lot.

Doing that,

  • I did change to the latest second party library versions used in @ChristianSch fork.
  • Changed to Junit test cases based on @ChristianSch's fork

I still have to look into a few things:

  • Standard source dir: I did change the standard source dir to match that of eclipse : "src/" instead of "/src/main/java". I might as well change the source dir in eclipse - still thinking about it.

  • I have kept the resource directory. Therefor, I did not use the classloaded in JUNIT test cases, yet. Is there a good reason to do it like that?

ChristianSch commented

Cool! I would've appreciated maintaining authorship of my work though.
If you're referring to the loading of datasets and such, I think having java handle the paths is the way to go as there might be problems with cross os support and future changes as hard coding things in many different places hurts future maintainability imho.

Please be aware that weasel fails in my fork! I haven't yet looked into it. And I couldn't run all tests, one was exceeding memory. Something to look into as well.

patrickzib commented

@ChristianSch I am really sorry, I didn't mean to take over your authorship. Do you have any preferences, how to acknowledge your work properly? And, if you prefer, I can backport the junit tests? What about adding you to the copyright notices?

  • I have fixed the issue with the exceeding memory in the test-cases by limiting the size of the test-datasets. another way would be to set -Xmx5g to the junit testruns.

  • Weasel should be fine in the latest junit-tests...?

mohataher commented

Hi Patrick:

Thank you for being open to us helping you out. That's really helpful. Here are few notes:

  1. @ChristianSch has to author a pull request from his fork to yours in order to maintain ownership. The pull request has to obviously include fully functioning code that passes all test cases. We need to work on @ChristianSch fork first before we merge it with yours.
  2. You must adhere to src/main/java and src/main/resources if you want to use Gradle or Maven. That is a standard and you could see that throughout all mature Java projects here in Github. Here's a beautiful example (https://github.com/JanusGraph/janusgraph/tree/master/janusgraph-core).

I'm going to start an issue on @ChristianSch fork, add list of actions and tag you Patrick on it.

ChristianSch commented

I'd totally accept @mohataher's idea. But if it's too much work, a note in the copyright file or something should suffice. I don't want to be a pain in the ass about it.
Are you, Patrick, okay with working in my fork first to get it up and running? I'm going to make you both contributors and shift the discussion (from my part) to the referenced issue. Cheers!

/e: please feel free to point out if you feel like we are overtaking your project! It's not my intention ;)

patrickzib commented

I'd very happy with any help :)

Do you still need any help in the fork?

ChristianSch commented

Oh yes, very much so! Maybe we should create a TODO file to have an overview of the things to be done.

patrickzib commented

closing this issue due to moving to gradle. thanks a lot to the two of you!

  • 🎉1