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

Add ability to fetch artifact with a given url #774

Merged
merged 36 commits into from Feb 22, 2018

Conversation

Projects
None yet
3 participants
@dotordogh
Copy link
Contributor

dotordogh commented Feb 10, 2018

Problem

We want to be able to fetch an artifact given a url. See #710

Solution

Add a new field in Attribute and assign it as the url in Artifact.

I also added a few tests to make sure fetch behaves as expected.

@wisechengyi
Copy link
Collaborator

wisechengyi left a comment

Thanks @dotordogh, this is great work!

There is one major issue, i.e. the resolve still requires a org:name:version that exists in a repo somewhere for url to work. Given the url, org:name:version can be totally invalid/arbitrary, so the following should work instead of trying to fetch the pom and error.

$ fetch a:b:c,url=http%3A%2F%2Fcentral.maven.org%2Fmaven2%2Fjunit%2Fjunit%2F4.12%2Fjunit-4.12.jar
Error:
  a:b:c
    not found: /Users/yic/.ivy2/local/a/b/c/ivys/ivy.xml
    not found: https://repo1.maven.org/maven2/a/b/c/b-c.pom

Minor issues are in the inline comments.

val attributes = attrs.get("classifier") match {
case Some(c) => Attributes("", c)
case None => Attributes("", "")
val attributes = Attributes("", attrs.getOrElse("classifier", ""), decode(attrs.getOrElse("url", ""), "UTF-8"))

This comment has been minimized.

@wisechengyi

wisechengyi Feb 10, 2018

Collaborator

While you are at it, could you add a check to throw an error if attrs contains things other than classifier or url?

assert(dep.module.name == "avro")
assert(dep.version == "1.7.4")
assert(dep.configuration == "runtime")
assert(dep.attributes == Attributes("", "", "file://some/encoded/url"))

This comment has been minimized.

@wisechengyi

wisechengyi Feb 10, 2018

Collaborator

How about making "file://some/encoded/url" a variable, and then make the moduleVersionConfig to be "org.apache.avro:avro:1.7.4:runtime,url=" + encoded(...)?

Same for below

val filename = "coursier-fetch-test.jar"
val jarFileContent = "tada"
val file = new File(filename)
val bw = new BufferedWriter(new FileWriter(file))

This comment has been minimized.

@wisechengyi

wisechengyi Feb 10, 2018

Collaborator

You should be able to levearge withFile above, like withFile("tada", filename, ext). Slight modification to withFile is required, so you won't need to do all the file operations here.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Feb 10, 2018

@alexarchambault what would be your suggestion how to handle the incompatibility?

[info] coursier: found 5 potential binary incompatibilities while checking against io.get-coursier:coursier_2.10:1.0.0-RC3  (filtered 319)
[error]  * method apply(java.lang.String,java.lang.String)coursier.core.Attributes in object coursier.package#Attributes does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("coursier.package#Attributes.apply")
[error]  * the type hierarchy of object coursier.core.Attributes is different in current version. Missing types {scala.runtime.AbstractFunction2}
[error]    filter with: ProblemFilters.exclude[MissingTypesProblem]("coursier.core.Attributes$")
[error]  * method apply(java.lang.String,java.lang.String)coursier.core.Attributes in object coursier.core.Attributes does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("coursier.core.Attributes.apply")
[error]  * method copy(java.lang.String,java.lang.String)coursier.core.Attributes in class coursier.core.Attributes does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("coursier.core.Attributes.copy")
[error]  * method this(java.lang.String,java.lang.String)Unit in class coursier.core.Attributes does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("coursier.core.Attributes.this")
[error] java.lang.RuntimeException: coursier: Binary compatibility check failed!
@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 12, 2018

Thanks @dotordogh!

As @wisechengyi noted, this still needs the POM files to be available in a repository for that to work. Is that intended?

Also, did you try to do that using the same trick as the sbt plugin, that is relying on a custom repository? (mentioned in #710 IIRC) I think FallbackDependenciesRepository could be used almost as is to handle that. It could be moved to the extra submodule to be available from cli too. (It has a few quirks - it's doing blocking things where it shouldn't, it directly calls java.net.HttpURLConnection instead of relying on the cache, … - but it should do the job in a first time.)

The advantage of this approach is that one has to change very little things in core for that to work. (core is the most sensitive module of coursier, so the less concerns handled in it, the better.)

To keep the uri passed by the users, the signature of Parse.moduleVersionConfig can be changed so that it returns not only a Dependency in case of success, but also a Map[String, String] of extra parameters.

@dotordogh

This comment has been minimized.

Copy link
Contributor

dotordogh commented Feb 13, 2018

Thank you @wisechengyi and @alexarchambault! I overlooked the fact that the resolve still needs the POM, my apologies!

I've been looking at FallbackDependenciesRepository and how I can use it, but I think I may be missing a piece. Are you thinking of something like 1) filter out the dependencies that have a url defined, 2) use FallbackDependenciesRepository to create artifacts for the dependencies with urls, and combine those artifacts with the artifacts that were produced as a result of the regular resolve for the other dependencies and then 3) perform the fetch?

@alexarchambault does FallbackDependenciesRepository create a repo on disc or is it in memory?

Dorothy Ordogh
remove Attributes third parameter and refactor Parse's moduleVersionC…
…onfig to return a tuple of Dependency and a Map of String to String representing extra parameters for the dependency
@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 13, 2018

@dotordogh Dependencies passed on the command line can be added as usual, including those with a manual uri (the uri doesn't appear in their Dependency, but it doesn't matter there). If some dependencies have a manual uri, they can be added to the FallbackDependenciesRepository with their uri. FallbackDependenciesRepository can be prepended to the repository list before the resolution. That way, it will be picked first, and any dependency it has will be handled by it (and it will return Artifacts pointing at its URIs).

That assumes the provided URIs should be used even if the dependency can be found in a repo. Should they for you? In sbt, it's only a fallback, so the FallbackDependencyRepository is added in last position to the repo list.

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 13, 2018

@alexarchambault does FallbackDependenciesRepository create a repo on disc or is it in memory?

FallbackDependencyRepository is entirely in memory, yes.

@dotordogh

This comment has been minimized.

Copy link
Contributor

dotordogh commented Feb 15, 2018

There are some tests that are still failing, this is just a preliminary push to see if I'm on the right track. Will update when the tests pass.

@dotordogh

This comment has been minimized.

Copy link
Contributor

dotordogh commented Feb 15, 2018

I still need to address @wisechengyi's comments and there are two more tests I would like to add, but feeling much better about the diff.

Dorothy Ordogh added some commits Feb 16, 2018

Dorothy Ordogh
Dorothy Ordogh
@dotordogh

This comment has been minimized.

Copy link
Contributor

dotordogh commented Feb 16, 2018

@wisechengyi & @alexarchambault I think my changes are ready for another review if you have some time 😄

I am also really struggling with linking errors and importing java.net.URLEncoder into ParseTests. I tried adding pureJava to the tests project in the build.sbt file but it didn't work. Any suggestions? I'm awful at working sbt.

@alexarchambault
Copy link
Member

alexarchambault left a comment

I added a few comments (the one about URLDecoder.decode should help with the CI errors.)

Also, maybe we could check for unused entries in the extraParams, from cli. That could be added in a later PR though…

build.sbt Outdated
@@ -63,6 +63,7 @@ lazy val tests = crossProject
hasITs,
coursierPrefix,
libs += Deps.scalaAsync.value,
pureJava,

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

That shouldn't be necessary, it's only for Java-only projects. See below for how to address the scala-js error on the CI.

allDependenciesWithExtraParams
.filter(depWithExtraParams => depWithExtraParams._2.isDefinedAt("url"))
.map(depWithExtraParams =>
(depWithExtraParams._1.moduleVersion, (new URL(depWithExtraParams._2.getOrElse("url", "")), true))).toMap

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

It's possible to check the presence of and get the "url" value in one pass, like

allDependenciesWithExtraParams
  .flatMap {
    case (dep, extraParams) =>
      extraParams.get("url").map { url =>
        dep.moduleVersion -> (new URL(url), true)
      }
  }
depsWithUrls.foreach(dep =>
if (forceVersions.contains(dep._1._1) && forceVersions.getOrElse(dep._1._1, dep._1._1) != dep._1._2)
throw new Exception("Cannot specify a URL for a dependency for which a version is being forced")
)

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

It should be possible to avoid the _?._?, like

for (((mod, ver), _) <- depsWithUrls if forceVersions.get(mod).exists(_ != ver))
  throw
if (attrs.isDefinedAt("url"))
Map("url" -> decode(attrs.getOrElse("url", ""), "UTF-8"))
else
Map()

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

A bit like above, we can avoid the isDefinedAt / getOrElse, with

attrs.get("url") match {
  case Some(url) => Map("url" -> decode(url, "UTF-8"))
  case None => Map()
}
attributes,
transitive = transitive,
exclusions = localExcludes.getOrElse(mod.orgName, Set()) | globalExcludes),
extraDependencyParams)

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

As extraDependencyParams is added to all the right cases, we could do something like

val depOrError = parts match {
   …
}

depOrError.right.map(dep => (dep, extraDependencyParams))

(only adding extraDependencyParams at one place).

val extraAttributes = attrs.keys.toSet.diff(validAttrsKeys)

if (attrs.size > validAttrsKeys.size || extraAttributes.nonEmpty)
throw new ModuleParseError(s"The only attributes allowed are: ${validAttrsKeys.mkString(", ")}. ${

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

I'll probably deprecate ModuleParseError in a later PR. In the public methods of this file, errors should be returned via a Left("error message") rather than an exception. (I guess I underlooked it when it was added :| )

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Collaborator

my bad :)

@@ -5,6 +5,7 @@ import coursier.core.{Module, Repository}
import coursier.ivy.IvyRepository
import coursier.maven.MavenRepository

import java.net.URLDecoder.decode

This comment has been minimized.

@alexarchambault

alexarchambault Feb 18, 2018

Member

Couldn't we do the decode part only from where the "url" value is used, that is in the cli module for now? That way, we wouldn't have to do an encode for it in the tests. (URLEncoder.encode seems not to be available from scala-js, hence the current CI errors, so that would solve that to.)

@wisechengyi
Copy link
Collaborator

wisechengyi left a comment

Thanks @dotordogh. Looks good functionally!


depsWithUrls.foreach(dep =>
if (forceVersions.contains(dep._1._1) && forceVersions.getOrElse(dep._1._1, dep._1._1) != dep._1._2)
throw new Exception("Cannot specify a URL for a dependency for which a version is being forced")

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Collaborator

Could you add in the error message which module violated this assumption?
E.g.

Cannot force a version that is different from the one specified for the module <groupId:artifactId> with url
@@ -800,6 +825,7 @@ class Helper(

// Trying to get the main class of the first artifact
val mainClassOpt = for {

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Collaborator

xx

import java.io.{ File, FileNotFoundException, IOException }
import java.net.{ HttpURLConnection, URL, URLConnection }

import java.io.{File, FileNotFoundException, IOException}

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Collaborator

Let's leave this file untouched since there's no functional change. If you would like to format the file, a separate PR would be better.

@@ -146,12 +153,12 @@ object Parse {
* or
* org:name:version:config,attr1=val1,attr2=val2
*
* Currently only "classifier" attribute is used, and others are ignored.
* Currently only the "classifier" attribute is used, and others are ignored.

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Collaborator

Docstring needs update since 'classifier' and 'url' are in use. The return value is also changed.

val fileContents = Source.fromFile(urlInJsonFile).getLines.mkString
assert(fileContents == "tada")


This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Collaborator

nit: xx empty lines

Dorothy Ordogh added some commits Feb 21, 2018

@alexarchambault
Copy link
Member

alexarchambault left a comment

Apart from the isDefined / return Left(…), LGTM. Let's merge as soon as this is fixed!

// Only "classifier" and "url" attributes are allowed
val validAttrsKeys = Set("classifier", "url")

val maybeErrorMsg = validateAttributes(attrs, validAttrsKeys)

This comment has been minimized.

@alexarchambault

alexarchambault Feb 21, 2018

Member

Instead of the .isDefined / return Left(…), we can do something like

validateAttributes(attrs, validAttrsKeys) match {
  case Some(err) => Left(err)
  case None =>
    …
}

which uses neither .get nor return (.get is ~unsafe, as it can throw exceptions if ever we forgot it to ensure it's a Some(…), and return has many drawbacks in Scala).

s"'org${argSeparator}name[${argSeparator}version][${argSeparator}config]${attrSeparator}attr1=val1${attrSeparator}attr2=val2'")
}
val y = x.split("=")
if (y.length != 2) {
throw new ModuleParseError(s"Failed to parse attribute '$x' in '$s'. Keyword argument expected such as 'classifier=tests'")
return Left(s"Failed to parse attribute '$x' in '$s'. Keyword argument expected such as 'classifier=tests'")

This comment has been minimized.

@alexarchambault

alexarchambault Feb 21, 2018

Member

The returns could be avoided like the one below, although is less straightforward there. (Basically, we can map without using return, getting a sequence of eithers, then collect all the lefts on the one hand, and all the rights on the other, and check that the former is empty - but I can look into it later if you'd like, it's still currently better than throwing exceptions)

This comment has been minimized.

@dotordogh

dotordogh Feb 21, 2018

Contributor

Thank you so much for all of the tips!! It's been a while since I used scala and I forgot I could do all this stuff! I tried implementing what you suggested here but ran into some issues, so I thought I would fix the isDefined/return Left(...) issue and return to this when I have some more time if you haven't done it already. Does that sound alright to you?

}
(y(0), y(1))
}
}).toMap

// Only "classifier" and "url" attributes are allowed
val validAttrsKeys = Set("classifier", "url")

This comment has been minimized.

@alexarchambault

alexarchambault Feb 21, 2018

Member

Do we really want to restrict the attribute keys here? But we can loosen that in a later PR I guess.

This comment has been minimized.

@wisechengyi

wisechengyi Feb 21, 2018

Collaborator

Yeah I would lean toward restricting it, because it provides clear error msg when seeing things it cannot handle, rather than silently ignoring it.

Dorothy Ordogh added some commits Feb 21, 2018

val validAttrsKeys = Set("classifier", "url")

validateAttributes(attrs, validAttrsKeys) match {
case Some(err) => return Left(err)

This comment has been minimized.

@dotordogh

dotordogh Feb 21, 2018

Contributor

I added this return statement here so that this error would behave the same way as the above errors do and stops the function from proceeding. If the return is not present, it doesn't stop the function and parsing will continue even though the attributes are invalid.

@wisechengyi
Copy link
Collaborator

wisechengyi left a comment

Thanks @dotordogh lgtm! just a few nits


// Prepend FallbackDependenciesRepository to the repository list
// so that dependencies with URIs are resolved against this repo
val repositories: Seq[Repository] = depsWithUrlRepo ++ standardRepositories

This comment has been minimized.

@wisechengyi

wisechengyi Feb 22, 2018

Collaborator

nit: maybe more clear with the following

val depsWithUrlRepo = FallbackDependenciesRepository(depsWithUrls)

val repositories: Seq[Repository] = Seq(depsWithUrlRepo) ++ standardRepositories

pattern <- propertiesPattern.substituteProperties(properties)
metadataPatternOpt <- metadataPropertiesPatternOpt.fold(Option.empty[Pattern].right[String])(_.substituteProperties(properties).map(Some(_)))
metadataPatternOpt <- metadataPropertiesPatternOpt
.fold(Option.empty[Pattern].right[String])(_.substituteProperties(properties).map(Some(_)))

This comment has been minimized.

@wisechengyi

wisechengyi Feb 22, 2018

Collaborator

looks like only format change, could you revert it so it would be less conflict for #781 to resolve?


if (attrs.size > validAttrsKeys.size || extraAttributes.nonEmpty)
Some(s"The only attributes allowed are: ${validAttrsKeys.mkString(", ")}. ${
if (extraAttributes.nonEmpty) s"The following are invalid: ${extraAttributes.mkString(", ")}"

This comment has been minimized.

@wisechengyi

wisechengyi Feb 22, 2018

Collaborator

nit: I feel the error can be more descriptive, e.g. for junit:junit:4.12,abc=123:

The only attributes allowed are: classifier, url. The following attribute(s) are invalid: 'abc' in 'junit:junit:4.12,abc=123'

Dorothy Ordogh and others added some commits Feb 22, 2018

Dorothy Ordogh
@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 22, 2018

Not merging right now for you to see e0bd15b, but that can be merged IMO else.

@dotordogh

This comment has been minimized.

Copy link
Contributor

dotordogh commented Feb 22, 2018

@alexarchambault Ahhhhh I see what you mean now! Very cool! Thank you!!

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 22, 2018

Cool! Merging then, thanks again!

@alexarchambault alexarchambault merged commit 3e4a65d into coursier:master Feb 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment