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

Enforce supported Maven versions #13023

Merged
merged 1 commit into from
Sep 1, 2015
Merged

Enforce supported Maven versions #13023

merged 1 commit into from
Sep 1, 2015

Conversation

jasontedor
Copy link
Member

Our builds are currently not compatible with Maven versions < 3.1.0 and
>= 3.3.0. This commit will enforce Maven 3 versions that our builds are
known to be compatible with. We will consider these our supported Maven
versions.

Closes #13022

Our builds are currently not compatible with Maven versions < 3.1.0 and
>= 3.3.0. This commit will enforce Maven 3 versions that our builds are
known to be compatible with. We will consider these our supported Maven
versions.

Closes #13022
@jasontedor jasontedor added :Delivery/Build Build or test infrastructure review v2.0.0 labels Aug 20, 2015
@dadoonet
Copy link
Member

See #13013

@jasontedor
Copy link
Member Author

Closing as #13013 was already opened earlier to achieve the same.

@jasontedor jasontedor closed this Aug 20, 2015
@jasontedor jasontedor deleted the enforce-maven-version branch August 20, 2015 20:40
@jasontedor jasontedor restored the enforce-maven-version branch August 21, 2015 00:22
@jasontedor jasontedor reopened this Aug 21, 2015
@jasontedor
Copy link
Member Author

Reopened as #13013 was only aimed at the distribution module.

@rmuir
Copy link
Contributor

rmuir commented Aug 21, 2015

I agree, this is needed, otherwise we will get strange questions or wacky artifacts being built.

Also we need it going forward too, this stuff changes over time, we want build servers to fail hard if they are misconfigured and not leave us debugging something complex.

@dadoonet
Copy link
Member

I'm -1 on this change sorry for all the reasons I gave there: #13013 (comment)

The most important to my heart is this one:

I think that external contributors could become annoyed if we don't allow them to contribute a patch on core or a plugin because they are running maven 3.3 although mvn install will just work fine...

I'm perfectly fine of failing (or warning only is even better) a single "minor" module such as a rpm one. Basically it will be only a constraint for people having rpmbuild installed (most likely Ubuntu users).
So forcing for example windows users to downgrade their maven version does not make sense to me.

Most likely they won't run QA tests when contributing to the project. So they won't hit any maven issue.

@jasontedor
Copy link
Member Author

I think that external contributors could become annoyed if we don't allow them to contribute a patch on core or a plugin because they are running maven 3.3 although mvn install will just work fine...

I'm not sure that I see the issue with that. Analogously, we have upper bounds on Java language versions (right now Java 7 even though JDK 7 is past end of life).

I'm perfectly fine of failing (or warning only is even better) a single "minor" module such as a rpm one. Basically it will be only a constraint for people having rpmbuild installed (most likely Ubuntu users). So forcing for example windows users to downgrade their maven version does not make sense to me.

People do build and package elasticsearch from source. We get questions it. We even call out the instructions for doing so in our README that is displayed on our project page. If we are going to be supporting and answering issues regarding such topics, we should enforce versions that we know our build works with top to bottom with. Those will be our supported versions.

@dakrone
Copy link
Member

dakrone commented Aug 26, 2015

I agree with @jasontedor for this, Maven versions should be bounded just like JDK versions are. Users can and will build their own distribution packages and we should ensure they use a non-broken version of Maven to do that.

@nik9000
Copy link
Member

nik9000 commented Aug 26, 2015

I'm not sure that I see the issue with that. Analogously, we have upper bounds on Java language versions (right now Java 7 even though JDK 7 is past end of life).

Just because Oracle has end-of-life-ed 1.7 doesn't mean its dead. RedHat will continue to support the OpenJDK 7 for a long time. But we've wandered off the point pretty far.

People do build and package elasticsearch from source. We get questions it. We even call out the instructions for doing so in our README that is displayed on our project page. If we are going to be supporting and answering issues regarding such topics, we should enforce versions that we know our build works with top to bottom with. Those will be our supported versions.

I'm all for this but in that case we have to work harder to get the build working with maven 3.3 because its what you get when you brew install maven. I suspect its what you get with apt-get install maven and yum install maven pretty frequently as well.

@jasontedor
Copy link
Member Author

Just because Oracle has end-of-life-ed 1.7 doesn't mean its dead. RedHat will continue to support the OpenJDK 7 for a long time. But we've wandered off the point pretty far.

That is not germane to this discussion.

I'm all for this but in that case we have to work harder to get the build working with maven 3.3 because its what you get when you brew install maven.

That's easily solved with brew install homebrew/versions/maven32.

I suspect its what you get with apt-get install maven

It's not. You actually get 3.0.5.

and yum install maven pretty frequently as well.

Out of the box this won't do anything. You have to set up a repo manually. Done using a common repo gives version 3.2.5. To the best of my knowledge, there is no official repo.

As a last point, I don't think the defaults much matter.

@dakrone
Copy link
Member

dakrone commented Aug 26, 2015

Also, brew, apt-get, yum, or dnf installing anything Java-related is not necessarily great for production systems. Remember when install java on older Linux systems used to symlink java to gcj? :) It's the whole reason for the JAVA_HOME confusion. I don't think we should rely on what package managers are providing.

Regardless, the whole point is that if someone does apt-get install maven and gets a bad version of Maven that we will prevent them from building ES with a version that doesn't work, so I still think this is necessary.

@jasontedor
Copy link
Member Author

Regardless, the whole point is that if someone does apt-get install maven and gets a bad version of Maven that we will prevent them from building ES with a version that doesn't work, so I still think this is necessary.

+1

@dadoonet
Copy link
Member

I'm not totally sure that contributors will appreciate having to change their maven version unless they work only on elasticsearch which I doubt.

Again, I'm totally +1 to limit the lower value to at least 3.1.0. I'm -1 to limit the upper value.
If you do it, then please exclude explicitly 3.3.0 to 3.3.3. But not 3.4 or 3.3.4 please.

And what is wrong with 3.3? Only QA multinode module fails if you run it from root dir.
Contributors who really really want to run the full tests can run until we fix the issue we have seen:

mvn clean install -pl !smoke-test-multinode
mvn clean install -pl smoke-test-multinode

My cents.

@nik9000
Copy link
Member

nik9000 commented Aug 26, 2015

As a last point, I don't think the defaults much matter.

Given your points above about ubuntu and rhel's default versions I think we can get away with just failing the build on Maven 3.3. I think defaults matter very much as they are the most likely thing to be on a new contributor's laptop. But if 3.0.5 and 3.2.5 are what you normally end up with in deb and rpm land that'll cover at least half of the folks and we aren't leaving too many people having to downgrade.

@jasontedor
Copy link
Member Author

I think defaults matter very much as they are the most likely thing to be on a new contributor's laptop.

To add another data point, as recently as OS X 10.8, Macs shipped with Maven 3.0.3 out of the box, no install required (although you did have to accept the Java 6 download).

The defaults vary wildly from platform to platform which is why they should only play a small role in determining which versions we support.

@rmuir rmuir added the blocker label Sep 1, 2015
@rmuir
Copy link
Contributor

rmuir commented Sep 1, 2015

This is a blocker. Being lenient here is not helpful. Asking to widen the support beyond what works already is irrelevant. If its a pain in the ass to get the versions we support that is irrelevant, it is what it is.

When I see stuff like https://discuss.elastic.co/t/error-building-rpm-against-2-0-branch/, then I know we need this PR like, yesterday.

Repeat after me: we will start making software that works, we will start making software that works...

jasontedor added a commit that referenced this pull request Sep 1, 2015
@jasontedor jasontedor merged commit edac404 into elastic:master Sep 1, 2015
@jasontedor jasontedor deleted the enforce-maven-version branch September 1, 2015 10:37
jasontedor added a commit that referenced this pull request Sep 1, 2015
This commit backports the pull request #13023 from master to 2.0.
@jasontedor jasontedor removed the review label Sep 1, 2015
@jasontedor
Copy link
Member Author

Removing the blocker label since this has been integrated into master and 2.0.

@jasontedor jasontedor removed the blocker label Sep 1, 2015
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v2.0.0-beta2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce supported Maven versions
7 participants