Closed wisechengyi closed 5 years ago
Can you provide some context for this change? There are a few troubling things - I think pre-existing, but I'm just becoming aware of them now- that this change has no issues referenced tracking:
Additionally, this script does not follow the new protocol established by @cosmicexplorer where scripts live in bin/...
. It's probably best to include him in review.
@jsirois To both points: the tagged version predates coursier became more popular, and contains the fixes we wanted. We have not vetted any version afterwards. We would probably do that at some point.
Description updated.
@cosmicexplorer please kindly let me know where I need to put it, or if there's a doc to follow. Thanks!
Thanks for the context Yi. What constitutes vetting? Is this just a Twitter employee changing a bootstrap coursier jar url to a published one on maven central and verifying CI doesn't break? If so, that seems preferrable to propagating dependence on a branch of a project forward.
Is this just a Twitter employee changing a bootstrap coursier jar url to a published one on maven central and verifying CI doesn't break?
Yes, mostly. Agreed on the branch dependence part.
So I think this looks like it can instead be exposed with a really simple wrapper around the existing methods we have to pull down artifacts from specific urls -- see what we do for LLVM here. We may need to extend the BinaryToolUrlGenerator
or perhaps the code that consumes it in binary_util.py
to use git (we may even be able to use git as an archive_type
in BinaryTool
, which wouldn't be that much code at all).
The above solution then allows us or any user to modify the version of the tool from pants_release_1.5.x
to whatever sha or tag they want. Because the process here is so simple and automatic (and because I happen to have implemented this kind of logic in other pants plugins already) I would really love to see this logic being done in the BinaryTool
in pants. I would be extremely happy to implement this part myself because it seems very preferable (let me know if I'm missing something).
If that blocks a timeline that having this binary would help, then I would really appreciate it if you could follow the structure in https://github.com/pantsbuild/binaries/blob/master/README.md, even if it just means making a build-coursier.sh
and a build.sh
script to invoke it for both linux and osx, and making the osx symlinks as described. But I would love to talk with you about getting the above git solution working as I believe it will reduce significantly the effort required for you and others to make any further coursier upgrades.
Talked this over and for the purposes of this PR we can just make some small modifications to the current PR to make it conform to the binaries process. I will make an upstream issue about the above alternative approach which would circumvent going through pants s3.
@cosmicexplorer ready for another look. thanks
The code is great! Is there a pants PR which consumes this coursier jar? I was looking in the pants repo just now -- you could perhaps do something like:
from pants.binaries.binary_tool import Script
class Coursier(Script):
_release_version = '1.5.x'
options_scope = 'coursier-binary' # or something
default_version = 'pants_release_{}'.format(_release_version)
name = 'coursier-cli-{}'.format(_release_version)
suffix = '.jar'
# Within another Optionable which has a `scoped` subsystem dependency,
# this code will give you the downloaded jar path.
Coursier.scoped_instance(self).select()
CoursierSubsystem
(for example) could be made to subclass Script
as above, or you could isolate the coursier jar into its own subsystem as is done in the native backend.
If there's already a PR, could you link it in the description?
Not yet. To start we can just change the url https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/coursier/coursier_subsystem.py#L55-L57 once it is published into binaries.pantsbuild.org
We can change it to a more parameterized version like ivy (https://github.com/pantsbuild/pants/blob/master/src/python/pants/ivy/ivy_subsystem.py#L22-L25), but it should be trivial.
name = 'coursier-cli-{}'.format(_release_version)
Actually, this won't work, because if the user tries to change the version with the option, it will still look for a jar with the 1.5.x
string in it. I had typed out a more complex implementation, but I think if we just had DEST_JAR_NAME="coursier-cli.jar"
everything would work and allow for additional versions later, so I might suggest making that change to build-coursier.sh
unless there is a reason we need the version string in the jar filename itself.
Also like what John asked earlier, there should be no need to build our own coursier in the future, but only need to consume the published version.
No worries. Thanks for the quick look!
@jsirois if this looks good, would you kindly merge it and sync with s3?
@wisechengyi sure - although I'm slightly confused. As I read our conversation above - this just constitutes a move of the same branch version from dropbox to binaries.pantsbuild.org when we both agree the right thing is to point Pants' default coursier to a released version and run this through CI. If so, I'm not sure what this PR buys us. Pointing pants at a released version of coursier also gets us off dropbox.
Closing in favor of https://github.com/pantsbuild/pants/pull/6853
So coursier jar can exist in binaries.pantsbuild.org instead of being on dropbox.