Skip to content

Conversation

@khos2ow
Copy link
Contributor

@khos2ow khos2ow commented Nov 28, 2017

Use maven wrapper to be used as ./mvnw foo bar -Pbaz instead of mvn foo bar -Pbaz.

  • wrapper is checked in directly in the repository
  • no need to have maven installed on OS

@khos2ow khos2ow changed the title [WIP] Use Maven wrapper [WIP] CLOUDSTACK-10161: Use Maven wrapper Nov 28, 2017
@khos2ow khos2ow mentioned this pull request Nov 28, 2017
4 tasks
@yadvr
Copy link
Member

yadvr commented Nov 29, 2017

This will replace one tool dependency (mvn) to another (mvnw) and also addition of library (.jar file) in codebase is not recommended.

@khos2ow
Copy link
Contributor Author

khos2ow commented Nov 29, 2017

IMO It's not just replacing one tool with another, it:

  • requires less dependency to packages installed on ones machine, given that there's no universal way to install Maven even in one specific OS/distro and one specific version of maven itself.
  • gives consistency, kind of the same way we have .java-version and .python-version. So everyone has and uses the exact same version of maven, everywhere, everytime.
  • and gives us control of that version. If -for whatever reason- Maven needs to be upgraded we will only need to change one jar file and one properties in the codebase itself.

@rafaelweingartner
Copy link
Member

Can't we download that binary (jar file) in the script execution?
I agree with @rhtyd, it is not common to version binary files.

@khos2ow
Copy link
Contributor Author

khos2ow commented Nov 30, 2017

@rafaelweingartner that defeats the whole point of using mvnw. And I totally agree we should never check in jar files but this is the only generally accepted exception. Modern maven or gradle based projects usually have this, to name a few:

@rafaelweingartner
Copy link
Member

@khos2ow that is very interesting. They are indeed versioning this jar file.

I always had Spring projects as an example of quality; I find versioning binary files a bit unusual though. However, if we zoom out the issue we see that we already version binary files when versioning pictures (jpg, png, and others) for instance. There are even ".class" files in our repo (https://github.com/apache/cloudstack/blob/master/tools/appliance/convert/Convert.class).

I would be ok with this change.

@rafaelweingartner
Copy link
Member

@khos2ow can you rebase this PR to latest master commit, and then we can check Jenkins and Travis status again. Let's make sure both are green to proceed with reviews

@khos2ow
Copy link
Contributor Author

khos2ow commented Feb 20, 2018

@rafaelweingartner I already rebased today after I saw your comment, but the thing is this PR is really in WIP and I haven't completed it yet. I'll try to do it Soon™!

@rafaelweingartner
Copy link
Member

Ok, thanks!

@khos2ow
Copy link
Contributor Author

khos2ow commented Feb 21, 2018

@rafaelweingartner actually I noticed that going forward with this PR depends on #2433, because they both touch building and packaging scripts (mostly).

@rafaelweingartner
Copy link
Member

no problem, so we focus there first.

@rafaelweingartner
Copy link
Member

@khos2ow are you planning to pick this up again?

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

i am not convinced that we should package build tools, though I agree we should ensure build tool versions in our build scripts. Having people ensure their build environment doesn't seem like a stranger thing to demand of developers or devops engineers. We could even provide a install-tools script to facilitate this, ensuring correct versions.
i am -0 (as opposed to +0) on this

# yum -y install mysql-server
# yum -y install git
# yum -y install genisoimage
$ yum -y update
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not valid. the prompt you could run yum from on most syustems would be '#'

@khos2ow
Copy link
Contributor Author

khos2ow commented Jul 24, 2018

@rafaelweingartner yes, I still think this would make sense to go forward with, but since I created a couple of docker cloudstack-builder containers, both for RPM and DEB, and since there are more tooling required to have an actual build (besides maven itself) we might be able to let this one go.

@rafaelweingartner
Copy link
Member

Can I close this then?

@khos2ow
Copy link
Contributor Author

khos2ow commented Jul 24, 2018

Sure, closed it.

@khos2ow khos2ow closed this Jul 24, 2018
@khos2ow khos2ow deleted the maven-wrapper branch July 24, 2018 16:24
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.

4 participants