sbgn / libsbgn

Libraries for the Systems Biology Graphical Notation (SBGN); Java and C++
Other
15 stars 8 forks source link

Saxon upgrade #58

Closed piotr-gawron closed 4 years ago

piotr-gawron commented 4 years ago

Today I found out that maven release used quite old saxon library (from 2006). This old library was in conflict with other parts of my code, but upgrade of saxon to latest version solved the problem.

This pull request contains following changes:

Regarding last bullet point I have some questions because I was a bit confused when I hit the problem. It looks like the project use two independent built process: maven and ant. Dependencies in both built process are managed separately. Do you plan to keep it that way? I would recommend to drop ant build process. It requires to keep locally in the repo binary of the dependent libraries (and when you upgrade the library everything stays in the history so size of your repo can grow really quickly). Another issue is that you might have differences in dependency list between processes that could lead to confusion (for instance right now ant depends on saxon 9.1, junit 3 and maven depends on saxon 8.7, junit 4).

If you plan to keep the ant build process I will add missing binaries to the lib folder. However, if you plan to remove ant I would get rid of the .jar lib files and build*.xml scripts and make the project a bit cleaner.

fbergmann commented 4 years ago

Hello Piotr, thank you for this pull request. The changes look fine to me. as to why there were two independent build systems. Basically we added the maven based system only quite recently. And to still allow the build on systems that did not use maven, we left the old system in place. I have no immediate plans to remove the ant scripts.

Since I personally do not use the library, i'll request @cannin to have a look as well.

piotr-gawron commented 4 years ago

Thanks, If you are planning in keeping both build systems I can at least synchronize the versions of libraries used in both. It's more reproducible that way.

cannin commented 4 years ago

@piotr-gawron is the current failures only because the deploy elements?

piotr-gawron commented 4 years ago

@cannin, Yes, the errors are only on deploy job (my branch has no credentials to the repo). I need to fix it and then will update list of changes in the pull requests.

I talked yesterday with Frank and decided to improve my PR a bit, unfortunately I had to stop in the middle, sorry for that.

piotr-gawron commented 4 years ago

Dear @cannin, @fbergmann

I updated pull request to include three more modifications:

Please let me know if it looks fine for you.