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

Initial Maven support #36

Merged
merged 5 commits into from
Sep 2, 2020
Merged

Initial Maven support #36

merged 5 commits into from
Sep 2, 2020

Conversation

pehala
Copy link
Collaborator

@pehala pehala commented Aug 26, 2020

  • Replace Ant with Maven
    • Add submodules for client, server, editor and shared.
    • Everything can be built by running mvn install in the root directory.
    • Inside client, editor and server directories you can run directly through mvn compile exec:java.
    • Builds .dmg and .exe for client submodule.
  • Update README.md
  • Replace Travis with Github Actions
    • I wanted to fix the CI but I never worked with Travis so I wrote Github Actions script which should do the same thing, including drafting a release with all files in it.
      • Sidenote: Currently it releases on every PR to showcase how it works and looks, the actual triggers are yet to be discussed.

Current problems:

  • I had a problem that the same version of morphia on maven had different package names and altogether seems different from the includes jar, I temporarily solved it by commenting more Database code and upgrading MongoDB-driver to a newer version. I do not like this change and I would like to fix it before this is ready to merge.
    • Since the database code is not working, this can stay in.
  • Client cannot be run from its directory through mvn compile exec:java because there is a problem with resource loading.
    • Fixed by changing how ImageManager loads images.
  • Tracks are packaged inside shared submodules even though they are present outside, it doesn't affect anything I just don't like it.
    • This is a minor issue, might be done with some restructuralization in a far future.

Fixes #35

@pehala pehala marked this pull request as draft August 26, 2020 08:55
@pehala pehala marked this pull request as ready for review August 26, 2020 08:55
@pehala
Copy link
Collaborator Author

pehala commented Aug 26, 2020

@PhilippvK This PR should be now ready for review.

I would say main issue with this is that I replaced your Travis CI with Github Actions. You can see how the result of this action looks like without any more configuration needed (https://github.com/pehala/playforia-minigolf/releases/tag/refs%2Fpull%2F1%2Fmerge). Of course the actual text and name of the release are yet to be discussed but I think it works fine. If you would like to see how the UI works for Github Actions jobs you do so here.

@PhilippvK
Copy link
Owner

PhilippvK commented Aug 26, 2020

Wow. I am very overwhelmed of your fast implementation and the quality of your work.

Actually migrating the build process to GitHub Actions was on my TODO list, but I only worked with Travis CI in the past. It looks great at a first look. I will have to check if the OS-specific executables/installers are still working fine before merging this PR.

Regarding the triggers for the CI runs. I would suggest to draft a release on Tags only (please let me know if you have arguments against that). Do you know if we can still update some sort of badge in the readme showing the status of the last build? (Especially checks before every merge request would be useful)

Another thing that would be great, but does not have to be tackled in this PR is maintaining the version if the releases with GItHub Actions. In detail there could be some sort of auto-incrementing mechanism which also makes sure that the version of the release is written in every metadata file of the project. If you do have any experiences with this, feel free to give it a try, else I will create an Issue for the feature.

@PhilippvK
Copy link
Owner

I had a problem that the same version of morphia on maven had different package names and altogether seems different from the includes jar, I temporarily solved it by commenting more Database code and upgrading MongoDB-driver to a newer version. I do not like this change and I would like to fix it before this is ready to merge.

I do not have any problem with this. I did the migration of the morphia library to the latest version already a year ago and also did manage to pull the tracks and user tables from the database which was still online. (Not the official plaforia DB, it was created by guy who created most of the codebase). I never pushed my results as I preferred the offline track storage (makes adding tracks later way easier) and the user login mechanism is not available anymore in the client. My plan was to be able to optionally pass the credentials of a private MongoDB server f.e. via command line parameters.

Recently was announced, that the mLab MongoDB Add-on which was used for hosting the database will be taken down (See https://devcenter.heroku.com/changelog-items/1823) which makes the situation a little bit more difficult. Please let me know if you think spending time on this topic is worth the time and effort.

@pehala
Copy link
Collaborator Author

pehala commented Aug 26, 2020

Regarding the triggers for the CI runs. I would suggest to draft a release on Tags only (please let me know if you have arguments against that). Do you know if we can still update some sort of badge in the readme showing the status of the last build? (Especially checks before every merge request would be useful)

I agree I will configure the release action to be run only when tag starting with v (e.g. v1.0) and then create sanity actions which will just try to do mvn install and will be run on every merge request to master.

We can configure the badge to show either one of them (https://docs.github.com/en/actions/configuring-and-managing-workflows/configuring-a-workflow#adding-a-workflow-status-badge-to-your-repository) but I would add the badge in another PR once we have some builds passing.

Another thing that would be great, but does not have to be tackled in this PR is maintaining the version if the releases with GItHub Actions. In detail there could be some sort of auto-incrementing mechanism which also makes sure that the version of the release is written in every metadata file of the project. If you do have any experiences with this, feel free to give it a try, else I will create an Issue for the feature.

Versions are defined in pom.xml and if you want I can include them in MANIFEST file in every jar. You can also bump all the versions manually with mvn release:update-versions -DautoVersionSubmodules=true. I would not do any auto bumping for now at least but we could do it in some future PR.

I had a problem that the same version of morphia on maven had different package names and altogether seems different from the includes jar, I temporarily solved it by commenting more Database code and upgrading MongoDB-driver to a newer version. I do not like this change and I would like to fix it before this is ready to merge.

I do not have any problem with this. I did the migration of the morphia library to the latest version already a year ago and also did manage to pull the tracks and user tables from the database which was still online. (Not the official plaforia DB, it was created by guy who created most of the codebase). I never pushed my results as I preferred the offline track storage (makes adding tracks later way easier) and the user login mechanism is not available anymore in the client. My plan was to be able to optionally pass the credentials of a private MongoDB server f.e. via command line parameters.

Recently was announced, that the mLab MongoDB Add-on which was used for hosting the database will be taken down (See https://devcenter.heroku.com/changelog-items/1823) which makes the situation a little bit more difficult. Please let me know if you think spending time on this topic is worth the time and effort.

I would also like to reintroduce support for MongoDB through command line parameters (which are next on my list of improvements I would like to introduce). We can also introduce a mechanism to import the files into the DB. If it's ok with you I will leave these changes in for now and we can fix it properly when we will deal with MongoDB.

@pehala
Copy link
Collaborator Author

pehala commented Aug 26, 2020

We can also add editor .exe and .dmg since it is pretty trivial to do so using maven.

@PhilippvK
Copy link
Owner

Thanks again for your work! Unfortunately I do not have much time for trying this out completely these days. I will have a look at it and will hopefully be able to merge it over the weekend.

- Remove external dependencies
- This includes two actions, one for release which is triggered on specific tags and one which just test the sanity by trying to build it.
- Fix few typos
- Replace Ant sections with Maven
- Add myself as a contributor
@pehala
Copy link
Collaborator Author

pehala commented Aug 29, 2020

Thanks again for your work! Unfortunately I do not have much time for trying this out completely these days. I will have a look at it and will hopefully be able to merge it over the weekend.

No problem, take your time. This is a big change, no need to rush it.

Also everything should be 100% ready now.

@pehala
Copy link
Collaborator Author

pehala commented Aug 30, 2020

I have managed to get the client working with maven compile exec:java as well by changing how the applet is loading images.

@pehala pehala changed the title [WIP] Initial Maven support Initial Maven support Aug 30, 2020
@PhilippvK
Copy link
Owner

I finally managed to try out the actual artifacts generated via the CI build. I do not see any problem with them and I also like the maven build system compared to ant.

One thing I would like to invest more time on by myself before finally merging this, is trying to port the Launch4j stuff you can find in build_dist.xml on the distributions branch. Meanwhile I would also create a wrapper script for the server packed into an .exe which allows to launch it on Windows without typing java -jar server.jar manually.

Will have a look at the version number handling as well.

Again I apologize for beeing slow. I do not expect to finish the aforementioned tasks before next week. Thanks for your patience.

@PhilippvK
Copy link
Owner

I realized that it may be a better idea to just merge the PR now and add the distributions stuff after that. Any arguments against that?

@PhilippvK
Copy link
Owner

Ahh the launch4j part seems to be already implemented but it has not made it into the github release workflow yet.

@pehala
Copy link
Collaborator Author

pehala commented Sep 2, 2020

Ahh the launch4j part seems to be already implemented but it has not made it into the github release workflow yet.

It should be already there, the release one should also include the .exe and .dmg, I am not sure if the .exe includes JRE packed in, but other than that it should work, if it doesn´t than thats a bug on my part.

@pehala
Copy link
Collaborator Author

pehala commented Sep 2, 2020

I realized that it may be a better idea to just merge the PR now and add the distributions stuff after that. Any arguments against that?

I think that would be the best approach, leave this with this functionality and add additional distribution options afterwards.

I also think that instead of .exe, we should do docker image for the server, but that is a discussion we can have in another issue.

@PhilippvK
Copy link
Owner

I agree with that.

Docker ist the way to go, I also used that for running the server on a google cloud instance earlier this year and also imagine that someone who forked my repo wrote a Dockerfile as well.

As a prerequisite for the docker support I would tackle adding command line arguments for the server to change the default ports and so on.

@pehala
Copy link
Collaborator Author

pehala commented Sep 2, 2020

I agree with that.

Docker ist the way to go, I also used that for running the server on a google cloud instance earlier this year and also imagine that someone who forked my repo wrote a Dockerfile as well.

As a prerequisite for the docker support I would tackle adding command line arguments for the server to change the default ports and so on.

Well, I already implemented that and I have PR ready to go once this is merged :D

@PhilippvK
Copy link
Owner

Hmm it seems like the DMG file for MacOS is not working right now. I will try to debug it later.

@PhilippvK
Copy link
Owner

Ahh the launch4j part seems to be already implemented but it has not made it into the github release workflow yet.

It should be already there, the release one should also include the .exe and .dmg, I am not sure if the .exe includes JRE packed in, but other than that it should work, if it doesn´t than thats a bug on my part.

Actually there was a separate .exe file for an windows installer which includes JRE 1.8 as this was requested.

The DMG file is for MacOS only but seems to be broken the moment. I imagine that I used an .img instead of a .dmg in the past but I can not remember what was the problem.

Copy link
Owner

@PhilippvK PhilippvK left a comment

Choose a reason for hiding this comment

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

Great work!

@PhilippvK PhilippvK merged commit 1eacd3a into PhilippvK:master Sep 2, 2020
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.

Migrate to a better build system
2 participants