You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/11/05 07:58:12 UTC
[GitHub] [ignite] rpuch opened a new pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
rpuch opened a new pull request #9554:
URL: https://github.com/apache/ignite/pull/9554
…d process
This allows us to achieve the following:
* Make it easier to build on machines where Maven is not installed yet
* Reduce possibility to get different build results due to different Maven versions
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] rpuch commented on pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
rpuch commented on pull request #9554:
URL: https://github.com/apache/ignite/pull/9554#issuecomment-970293882
@vveider I've added a paragraph about Maven Wrapper to the very beginning of DEVNOTES.
Should I change anything else (apart from *.adoc-related concerns)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] rpuch commented on pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
rpuch commented on pull request #9554:
URL: https://github.com/apache/ignite/pull/9554#issuecomment-970192040
@vveider thank you for the advice, I will contact Nikita.
### About special paragraph
As for Maven Wrapper specific prerequisites: according to https://github.com/takari/maven-wrapper , they should run on JDK 7, while Ignite requires JDK 8 itself, so no problem here. Also, they seem to support any decent `sh` flavour (I mean, Unix-like shells). Apart from this, Maven Wrapper requires Java 8 and `JAVA_HOME` variable set, but this is exactly what is needed for vanilla Maven, so nothing new here.
So it seems that we simply do not need to mention anything specific about Maven Wrapper prerequisites: it will just work everywhere where Maven would work. Its usage is also straightforward: you just say `./mvnw` instead of `mvn`, and it transparently works in the same way as for `mvn`, but handles Maven management itself.
Given all this, are you sure that we need to have that special paragraph in DEVNOTES?
### About `./mvnw` spell
As for `./mvnw` on different systems, there are two groups.
* Unix-like systems (Linux, Mac OS, etc): it works in sh, bash, dash, zsh, so basically 'everywhere'
* Windows. Here we have a zoo! I carried out a quick poll in our Slack channel about Windows users preferences. They named PowerShell, cmd, Idea builtin shell, cygwin, git-bash, WSL (Windows Subsystem for Linux).
- In PowerShell (Win10), `./mvnw`, `.\mvnw`, `./mvnw.cmd`, `.\mvnw.cmd` all work, but `mvnw` and `mvnw.cmd` do not work with default settings
- In cmd (Win10), only `.\mvnw` and `.\mvnw.cmd` work, while `mvnw` and `mvnw.cmd` still do not work with default settings
- In the Maven Wrapper documentation https://github.com/takari/maven-wrapper , they mention `mvnw.cmd` spell to invoke it on Windows. This does not seem to work on modern Windows versions, but it probably did in earlier ones. But they also say `mvn` where it clearly should be `./mvnw`, so there is a probability of a mistake in their documentation...
- In Idea, the built-in shell seems to default to the system default (in my case, it was PowerShell)
- cygwin, git-bash, WSL all have `bash` in them, so `./mvnw` works
With all this, it looks like `./mvnw` should work for *nearly* everyone, but not for all. I could add a paragraph in the beginning of DEVNOTES about this discrepancy, like 'For most users on Unix-like systems and in PowerShell, `./mvnw` will work, so we use `./mvnw` spell everywhere in the docs, but if you use `cmd`, write `\.mvnw`'. Would this be enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] rpuch commented on pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
rpuch commented on pull request #9554:
URL: https://github.com/apache/ignite/pull/9554#issuecomment-970193556
Hi @Nikita-tech-writer! Could you please take a look at the changes in *.adoc files in this PR? What are your thoughts on them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] vveider commented on pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
vveider commented on pull request #9554:
URL: https://github.com/apache/ignite/pull/9554#issuecomment-970241039
1. Special paragraph — we should only mention that project uses Maven Wrapper by default with description on how to run Maven Wrapper on different operation systems.
2. Spelling in commands — let's keep `./mvwn` considering special paragraph in Devnotes will have description about that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] vveider merged pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
vveider merged pull request #9554:
URL: https://github.com/apache/ignite/pull/9554
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] Nikita-tech-writer commented on pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
Nikita-tech-writer commented on pull request #9554:
URL: https://github.com/apache/ignite/pull/9554#issuecomment-979854352
@rpuch I'm absolutely OK with the changes!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] rpuch commented on pull request #9554: IGNITE-15872 Add Maven Wrapper to facilitate and standardize the buil…
Posted by GitBox <gi...@apache.org>.
rpuch commented on pull request #9554:
URL: https://github.com/apache/ignite/pull/9554#issuecomment-968052819
@vveider thank you for your review!
I've changed `mvn` command invocation to `./mvnw` everywhere where it makes sense (only documentation is concerned).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org