-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per previous discussion (sorry on mobile, don't have link on hand), we are not allowed to published binaries (the jar file). Basically Apache forbids that action for all of it's projects
@marcoabreu Please provide the link. I would consider reading answers in LEGAL-427 carefully as well. |
I don't understand how that link is relevant. The Apache release guidelines forbid binaries in source releases |
@nswamy Can you please take a look? |
@frankfliu you have any comments on the concerns from @marcoabreu ? |
@marcoabreu You can find many apache project with maven wrapper checked in github: |
@marcoabreu ping for a relook at this PR. |
} | ||
} | ||
|
||
private static void downloadFileFromURL(String urlString, File destination) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some retries in here to reduce the failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is needed. It's code coming in from Maven Wrapper package.
@frankfliu Please raise a discussion on dev@ to discuss about uploading the binary |
Thanks @lanking520 , that's a great idea. We have been advised by out mentors in the past to specifically remove the maven wrapper binary, but it might be good to bring it up again. |
Marco,
Agree you shouldn't check in binaries, but you can exclude the maven
wrapper binary from source control and it will be downloaded when needed.
See https://github.com/takari/maven-wrapper#usage-without-binary-jar.
Interested to read any links referencing those discussions if you have them.
Mike
…On Thu, Jan 24, 2019 at 2:56 PM Marco de Abreu ***@***.***> wrote:
Thanks @lanking520 <https://github.com/lanking520> , that's a great idea.
We have been advised by out mentors in the past to specifically remove the
maven wrapper binary, but it might be good to bring it up again.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#13702 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAJJsRHzUCRGh1cKWTB98CTVQN9lbhNks5vGg_bgaJpZM4Zcl_I>
.
|
Ah, that's an excellent alternative then :) There you go: https://lists.apache.org/thread.html/d409819b554745937103b60d36ad3f5b2b1300fe8ae826fb0e665f7f@%3Cdev.mxnet.apache.org%3E |
Open LEGAL ticket: https://issues.apache.org/jira/browse/LEGAL-288 |
Great, thanks for the links. I don't see the issue if the jar file is not
included in the source and not included in the distirbution, but interested
to hear other's interpretations. LEGAL-288 to me is about including that
jar file. Was this asked on the dev list?
…On Thu, Jan 24, 2019 at 8:46 PM Marco de Abreu ***@***.***> wrote:
Open LEGAL ticket: https://issues.apache.org/jira/browse/LEGAL-288
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#13702 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAJJuTxoSHcdzsyqmTKqghtgt-Yhbymks5vGmICgaJpZM4Zcl_I>
.
|
We had it in the past and were asked to remove it by mentors. I've linked both threads If there is an alternative to a binary file - which is the case according to your link - I'm also in favour to not have them checked in at all as otherwise users will have to manually download them and we have to manually remove them. |
@marcoabreu I removed jar file. The maven wrapper can compile MavenWrapperDownloader.java and download the .jar file on the fly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you! :)
@frankfliu Since we are good with this PR, please add the final changes on the
In order to coordinate this change. Thanks |
@mxnet-label-bot update [Scala, pr-awaiting-merge] |
@frankfliu please add the remaining items in JIRA as a backlog item |
Add maven wrapper to scala project, this removed maven dependency from host machine.
And this will also eliminate build script compatibility issue with different maven version.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.