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

improve BUILDING.md #5963

Closed
wants to merge 1 commit into from
Closed

Conversation

k77ch7
Copy link
Contributor

@k77ch7 k77ch7 commented Nov 7, 2019

Following commands does not work for me.
mvn -Pbootstrap
mvn -Pbootstrap-no-launcher
mvn -pl core

I guess using ./mvn instead of mvn is right.

My Env

$ ./mvnw -v
Apache Maven 3.6.0 (97c98ec64a1fdfee7767ce5ffb20918da4f719f3; 2018-10-25T03:41:47+09:00)
Maven home: /Users/xxx/.m2/wrapper/dists/apache-maven-3.6.0-bin/3rgjh30jneo7541hun7uggltkb/apache-maven-3.6.0
Java version: 11.0.2, vendor: Oracle Corporation, runtime: /Library/Java/JavaVirtualMachines/openjdk-11.0.2.jdk/Contents/Home
Default locale: ja_JP, platform encoding: UTF-8
OS name: "mac os x", version: "10.15", arch: "x86_64", family: "mac"

@headius
Copy link
Member

headius commented Dec 2, 2019

Hmm I notice we do use ./mvnw at the beginning of the doc, and at some point we switch back to regular mvn assuming it's installed in PATH somewhere. It seems like we should be consistent, or else make the mvn command lines clearly demarcate that this must be your working maven command, a la something like this:

<mvn command> -Pbootstrap

Thoughts? @enebo @kachick @mkristian

@enebo
Copy link
Member

enebo commented Dec 2, 2019

I feel like abstracting this is not helpful. If you are a maven user 'mvn' is a known thing. Seeing ./mvnw for them will know they can just type 'mvn'. If not then possibly just using ./mvnw everywhere will help the non-maven users more.

@mkristian
Copy link
Member

for me using ./mvnw also means that we know the maven version and all works. using different maven version may or may not have issues

@enebo
Copy link
Member

enebo commented Dec 3, 2019

@mkristian ah yeah good point. Another thing a maven user may figure out but a new user will likely get tripped up on.

@k77ch7
Copy link
Contributor Author

k77ch7 commented Dec 4, 2019

Thanks for your reply.
I had overlooked the following WARNING.

[WARNING] Rule 0: org.apache.maven.plugins.enforcer.RequireMavenVersion failed with message:
Detected Maven Version: 3.2.1 is not in the allowed range [3.3.0,).

After changing mvn version 3.2.1 to 3.3.9, it now works.

I think it's better to clarify the mvn version.
What do you think about this diff? @headius

diff --git a/BUILDING.md b/BUILDING.md
index ea56d7b2cc..d778304133 100644
--- a/BUILDING.md
+++ b/BUILDING.md
@@ -5,7 +5,7 @@ Prerequisites:

 * A [Java 8-compatible (or higher) Java development kit (JDK)](http://www.oracle.com/technetwork/java/javase/downloads/index.html).
   * If `JAVA_HOME` is not set on Mac OS X: `export JAVA_HOME=$(/usr/libexec/java_home)`
-* [Maven](https://maven.apache.org/download.cgi) 3+ (Maven Wrapper provided with `./mvnw`)
+* [Maven](https://maven.apache.org/download.cgi) 3.3.0+ (Maven Wrapper provided with `./mvnw`)
 * [Apache Ant](https://ant.apache.org/bindownload.cgi) 1.8+ (see https://github.com/jruby/jruby/issues/2236)
 * [Make](https://www.gnu.org/software/make/) and a C++ compiler for installing the jruby-launcher gem

@headius
Copy link
Member

headius commented Dec 9, 2019

@k77ch7 Yes I think that is a better option. We only mention mvnw as a way for people with no maven (or the wrong version) to get a working version, and I think it's cleaner for us to just refer to mvn from then on. Your change will help make it clear what our minimum version is.

If you are ok with it, I will commit your proposed version-update patch with attribution and close this PR. Ok?

@k77ch7
Copy link
Contributor Author

k77ch7 commented Dec 10, 2019

@headius It’s OK.

@headius
Copy link
Member

headius commented Dec 13, 2019

Committed the minimal version update in 9970478. Thanks!

@headius headius closed this Dec 13, 2019
@headius headius added this to the JRuby 9.2.10.0 milestone Dec 13, 2019
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