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

Project maven cleanup #50

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Project maven cleanup #50

wants to merge 4 commits into from

Conversation

sgdc3
Copy link
Contributor

@sgdc3 sgdc3 commented Aug 12, 2018

TODO:

  • Update docs

I also added NCP to my CI setup, i added admin rights over the folder to any member of the NCP github org (authentication works via github oauth, remember allow access to the org on first login).

Jenkins build: https://ci.codemc.org/job/NoCheatPlus/job/NoCheatPlus/
Maven repository: https://repo.codemc.org/repository/maven-public/

@sgdc3 sgdc3 changed the title Project cleanup Project maven cleanup Aug 12, 2018
@asofold
Copy link
Member

asofold commented Aug 13, 2018

What are the changes for NMS compat modules aiming for?

Did you have trouble building?

@sgdc3
Copy link
Contributor Author

sgdc3 commented Aug 13, 2018

It was not following the maven standards, i also provided a repository containing NMS, so it will be possible to build it without having to run buildtools first.

@asofold
Copy link
Member

asofold commented Aug 14, 2018

I currently have no space for reviewing which parts are really "necessary"...

Specifically:

  • A public spigot binary repository is out of the question, i'd prefer an option to build spigot right on the spot an then use it rather than that.
  • Standards/conform... there seem to be many changes (e.g. spaces vs tabs), I'd prefer an overview for why which part isn't conform, perhaps an issue would've been better to start discussion.
  • Not so keen to change anything concerning maven unless it doesn't build.
  • (1.13 Material changes have more impact, so i'll have to focus on that.)

@sgdc3
Copy link
Contributor Author

sgdc3 commented Aug 15, 2018

  1. Why? you don't have any risk using it.
  2. Space/Tabs:
    https://maven.apache.org/developers/conventions/code.html
  3. That's not a very good idea, if something can be improved why not?

@asofold
Copy link
Member

asofold commented Aug 18, 2018

"Improve something" means i have to look through every line - especially if it's not at all crucial, it's probably no option at this very moment :).

There is too many type of changes mixed in the commits and this PR. With MC 1.13 in the queue there is no way to look further into it.

Best would be to separate:

  • Spaces vs tabs. For a larger changeset this should be one commit for all, so an idiot can see the difference easily.
  • ncpcompatnonfree as parent (good! but: did you check that certain profiles still build at all? mostly concerns latest/minimal)
  • NCPCompatAggregated - due to standards, or just a fancy idea? Altering profiles and Project structure is not the type of things i do within a "spaces vs tabs commit" :p.
  • (Binary repository <- not / extra pr / different topic).

This'll have to wait until i'm through with most of 1.13.

@sgdc3
Copy link
Contributor Author

sgdc3 commented Aug 19, 2018

Ok, i'll wait the 1.13 release first and then start splitting my changes into more PRs ;)

@asofold
Copy link
Member

asofold commented Aug 20, 2018

Thank you :).

@phoenixd21
Copy link

who wants to get taht crpto hacl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants