CWTSLeiden / networkanalysis

Java package that provides data structures and algorithms for network analysis.
MIT License
144 stars 33 forks source link

Use Gradle for building and publishing #11

Closed neesjanvaneck closed 3 years ago

neesjanvaneck commented 3 years ago

The most important changes are as follows:

vtraag commented 3 years ago

One other comment: it might be convenient to setup a version system based on GitHub tags: https://github.com/palantir/gradle-git-version. That way, you just have to update the tag, and the version information is automatically updated. Moreover, the versions are now not clearly distinguishable. That is: all different commits of this repository will be marked as version 1.1.0. Using version system based on GitHub tags addresses that as well.

This of course depends on how often we expect to make updates. If you expect more regular updates, this might be worth the effort.

neesjanvaneck commented 3 years ago

When building locally (gradle build) I am receiving the following warning:

src/main/java/nl/cwts/networkanalysis/run/package-info.java:13: warning - Tag @link: reference not found: nl.cwts.networkanalysis
1 warning

It seems that this is because the gradle that I have locally is version 6.6.1, whereas gradlew uses version 6.7. Should we also make sure that this works correctly with earlier versions of gradle, perhaps ensure compatibility with all versions 6? If that is not possible, it might be good to also explicitly state the minimal version that is supported (i.e. 6.7 in this case).

I'm not sure why you receive the warning using the gradle command. Do you understand the warning? I have tested it before with Gradle 6.4 and did not encounter any problems, but perhaps I missed the warning. Currently, we only refer to the use of the Gradle Wrapper command gradlew in the readme. The nice thing about the bundled Gradle Wrapper is that people don't have to rely on an installed version of Gradle. This also avoids version issues.

neesjanvaneck commented 3 years ago

One other comment: it might be convenient to setup a version system based on GitHub tags: https://github.com/palantir/gradle-git-version. That way, you just have to update the tag, and the version information is automatically updated. Moreover, the versions are now not clearly distinguishable. That is: all different commits of this repository will be marked as version 1.1.0. Using version system based on GitHub tags addresses that as well.

This of course depends on how often we expect to make updates. If you expect more regular updates, this might be worth the effort.

Great suggestion! I have setup the version system as indicated and I believe it works correct. Could you also test it?

vtraag commented 3 years ago

I have not looked at the warning myself. It is also just a warning, so it should not be a problem. Perhaps I wi try at some later time with other gradle versions still.

Good to see the version system is working! I think we actually still need to take the release commit as version 1.0.0, so that I will have a correct versioning once it gets merged. I will do that later.

vtraag commented 3 years ago

Oh, one more remark regarding versioning: I think it would be good if we stick to semantic versioning. I am not sure, but it might also be a requirement from Maven/Gradle. This essentially entails the following:

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards compatible manner, and PATCH version when you make backwards compatible bug fixes. Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

See https://semver.org/ for more details.

We would hence need to double check whether there have been any API incompatible changes between 1.0.0 and the code after having merged the layout code. There have definitely been some changes with regards to the run class, but we can discuss whether that should count or not with respect to the API.

vtraag commented 3 years ago

In the same spirit: it might be good to include a CHANGELOG. I am not sure if there is any specific format of that for Gradle?

vtraag commented 3 years ago

I think we actually still need to take the release commit as version 1.0.0, so that I will have a correct versioning once it gets merged. I will do that later.

It seems that we did correctly tag the version. However, the produced packages on GitHub actions do not have a correct version number (i.e. networkanalysis-b58601e.jar). When I build the packages locally the versions are correct. For some reason the CI is now also not triggered when I push a new commit to this branch/PR. Is perhaps something not correctly configured yet?

neesjanvaneck commented 3 years ago

Oh, one more remark regarding versioning: I think it would be good if we stick to semantic versioning. I am not sure, but it might also be a requirement from Maven/Gradle. This essentially entails the following:

Given a version number MAJOR.MINOR.PATCH, increment the: MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards compatible manner, and PATCH version when you make backwards compatible bug fixes. Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

See https://semver.org/ for more details.

We would hence need to double check whether there have been any API incompatible changes between 1.0.0 and the code after having merged the layout code. There have definitely been some changes with regards to the run class, but we can discuss whether that should count or not with respect to the API.

As far as I can seen, we have not made any API incompatible changes to the clustering classes. We only made an API relevant change in the runNetworkClustering class (we moved the method readEdgeList to the FileIO class and changed the parameters slightly). Since it is the run class, we could indeed discuss whether that should count or not. I'm inclined not to count it. I believe this means that we should only increase the minor version resulting in a release version number of 1.1.0. What do you think?

neesjanvaneck commented 3 years ago

I think we actually still need to take the release commit as version 1.0.0, so that I will have a correct versioning once it gets merged. I will do that later.

It seems that we did correctly tag the version. However, the produced packages on GitHub actions do not have a correct version number (i.e. networkanalysis-b58601e.jar). When I build the packages locally the versions are correct. For some reason the CI is now also not triggered when I push a new commit to this branch/PR. Is perhaps something not correctly configured yet?

Locally, everything indeed works as expected.

I have tested the same Github Actions workflow setup in another (private) repository. The GitHub Actions build workflow is indeed producing a package with incorrect version numbers. I have no idea why this is the case. Moreover, because the GitHub Actions publish workflow that is triggered after making a release is generating a package with a correct version number. Since release packages do have a correct version number, we perhaps don't have to worry about this issue?

neesjanvaneck commented 3 years ago

... For some reason the CI is now also not triggered when I push a new commit to this branch/PR. Is perhaps something not correctly configured yet?

I did some testing. I have no idea why the build workflow is not triggered anymore. Could the conflict with the master branch perhaps be the cause?

vtraag commented 3 years ago

OK, let's stick to 1.1.0 then.

It would be strange if the conflict with the master branch would prevent the CI from running. Anyway, hopefully that is something that will appear to be a fluke.

The automatic versioning might still be an issue that we need to check out. But let's simply merge this then and see how it all runs from the master branch directly.

Can you merge it (and solve the conflicts)? If you want my help, just let me know.

neesjanvaneck commented 3 years ago

After solving the conflict, the CI seems to work again. 😃

vtraag commented 3 years ago

Ha, great! Strange that it doesn't build if there are conflicts, that is not very convenient. Anyway, let's merge this into master then, and see if the versioning does work correct in the master branch.

vtraag commented 3 years ago

Hmm, it seems that the versioning information is still not correct on the master branch. Shall we perhaps just switch to manual versioning then?