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

Specify classifier on module level on CLI #735

Merged
merged 49 commits into from Jan 29, 2018

Conversation

Projects
None yet
2 participants
@wisechengyi
Copy link
Collaborator

wisechengyi commented Jan 12, 2018

Currently --classifier is applied to all modules requested on CLI, so this change add the capability to specify classifier for each module.

fetch -t \
org.apache.commons:commons-compress:1.5,classifier=tests \
org.apache.commons:commons-compress:1.5

Fix for #717

@wisechengyi wisechengyi referenced this pull request Jan 12, 2018

Closed

downloading fails #734

wisechengyi added some commits Jan 12, 2018

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Jan 12, 2018

@wisechengyi You can add tests for what you added in core/shared/src/main/scala/coursier/util/Parse.scala in ParseTests.

Also, I see you changed Resolution a bit… This needs a test somewhere.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 12, 2018

Thanks for the pointer. and yes I do plan to add tests :)

wisechengyi added some commits Jan 12, 2018

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 13, 2018

@alexarchambault the part changed in Parse actually does not concern anything but cli, can I move that component to cli instead?

$ find . | grep \.scala$ | xargs grep moduleVersionConfig -irn
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:171:  val scaladexModuleVersionConfigs: List[ParsedModule] =
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:235:  val (modVerCfgErrors: Seq[String], moduleVersionConfigs: Seq[ParsedModule]) =
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:236:    Parse.moduleVersionConfigs(otherRawDependencies, scalaVersion)
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:237:  val (intransitiveModVerCfgErrors: Seq[String], intransitiveModuleVersionConfigs: Seq[ParsedModule]) =
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:238:    Parse.moduleVersionConfigs(intransitive, scalaVersion)
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:240:  val allModuleVersionConfigs: Seq[ParsedModule] =
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:242:    scaladexModuleVersionConfigs ++ moduleVersionConfigs
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:359:  val baseDependencies: Seq[Dependency] = allModuleVersionConfigs.map {
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:364:  val intransitiveDependencies: Seq[Dependency] = intransitiveModuleVersionConfigs.map {
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:885:          parsedModule: ParsedModule <- allModuleVersionConfigs.headOption
./cli/src/main/scala-2.11/coursier/cli/Helper.scala:898:          parsedModule: ParsedModule <- allModuleVersionConfigs.headOption
./core/shared/src/main/scala/coursier/util/Parse.scala:117:  def moduleVersionConfig(s: String): Either[String, ParsedModule] =
./core/shared/src/main/scala/coursier/util/Parse.scala:118:    moduleVersionConfig(s, defaultScalaVersion)
./core/shared/src/main/scala/coursier/util/Parse.scala:139:  def moduleVersionConfig(s: String, defaultScalaVersion: String): Either[String, ParsedModule] = {
./core/shared/src/main/scala/coursier/util/Parse.scala:204:  def moduleVersionConfigs(l: Seq[String]): (Seq[String], Seq[ParsedModule]) =
./core/shared/src/main/scala/coursier/util/Parse.scala:205:    moduleVersionConfigs(l, defaultScalaVersion)
./core/shared/src/main/scala/coursier/util/Parse.scala:212:  def moduleVersionConfigs(l: Seq[String], defaultScalaVersion: String): (Seq[String], Seq[ParsedModule]) =
./core/shared/src/main/scala/coursier/util/Parse.scala:213:    valuesAndErrors(moduleVersionConfig(_, defaultScalaVersion), l)
./core/shared/src/main/scala/coursier/util/Parse.scala:117:  def moduleVersionConfig(s: String): Either[String, ParsedModule] =
./core/shared/src/main/scala/coursier/util/Parse.scala:118:    moduleVersionConfig(s, defaultScalaVersion)
./core/shared/src/main/scala/coursier/util/Parse.scala:139:  def moduleVersionConfig(s: String, defaultScalaVersion: String): Either[String, ParsedModule] = {
./core/shared/src/main/scala/coursier/util/Parse.scala:204:  def moduleVersionConfigs(l: Seq[String]): (Seq[String], Seq[ParsedModule]) =
./core/shared/src/main/scala/coursier/util/Parse.scala:205:    moduleVersionConfigs(l, defaultScalaVersion)
./core/shared/src/main/scala/coursier/util/Parse.scala:212:  def moduleVersionConfigs(l: Seq[String], defaultScalaVersion: String): (Seq[String], Seq[ParsedModule]) =
./core/shared/src/main/scala/coursier/util/Parse.scala:213:    valuesAndErrors(moduleVersionConfig(_, defaultScalaVersion), l)

Edit: correct Resolution to Parse

wisechengyi added some commits Jan 13, 2018

cmt
Revert "linux only"
This reverts commit 5eacf7d.
Revert "make it no container based"
This reverts commit e64ac0d.
ini
@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Jan 18, 2018

@alexarchambault the part changed in Parse actually does not concern anything but cli, can I move > that component to cli instead?

I think I'd prefer to keep it here, along with the other methods of Parse. These might be useful in other contexts (for users of coursier via its API).

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Jan 18, 2018

About ParsedModule, couldn't it be replaced with just Dependency?

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 19, 2018

Ok. The old interfaces are added back to manage the deprecation check.

wisechengyi added some commits Jan 19, 2018

@wisechengyi wisechengyi changed the title [wip] collect classifer attribute on CLI Specify classifier on module level on CLI Jan 19, 2018

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 19, 2018

ready for review when you get a chance @alexarchambault . thank you

wisechengyi added some commits Jan 19, 2018

@alexarchambault
Copy link
Member

alexarchambault left a comment

Added some comments.

@@ -113,44 +113,115 @@ object Parse {
}
}

case class ModuleParseError(private val message: String = "",

This comment has been minimized.

@alexarchambault

alexarchambault Jan 21, 2018

Member

Exceptions shouldn't be case classes. Two ModuleParseError with the same message and cause would be considered equal (because case class), but may not actually be equal (thrown from different places, so different stack trace). But a simple class is ok.

This comment has been minimized.

@wisechengyi

wisechengyi Jan 23, 2018

Collaborator

corrected.

extends Exception(message, cause)

@deprecated("use the variant accepting a default scala version", "1.0.0-M13")
def moduleVersionConfig(s: String, scalaVersion: String): Either[String, Dependency] =

This comment has been minimized.

@alexarchambault

alexarchambault Jan 21, 2018

Member

The return types shouldn't be changed, to keep binary compatibility. (Don't know if mima complains about that, but it's sure it would lead to ClassCastException later on.)

This comment has been minimized.

@wisechengyi

wisechengyi Jan 22, 2018

Collaborator

mima did not complain about that. guess we just found out what limitation it has :) will correct.

This comment has been minimized.

@wisechengyi

wisechengyi Jan 22, 2018

Collaborator

Oh also, how do you think we can be clear about when this can be removed?

This comment has been minimized.

@wisechengyi

wisechengyi Jan 23, 2018

Collaborator

reverted the interface

@deprecated("use the variant accepting a default scala version", "1.0.0-M13")
def moduleVersionConfig(s: String): Either[String, (Module, String, Option[String])] =
moduleVersionConfig(s, defaultScalaVersion)
def moduleVersionConfig(s: String): Either[String, Dependency] =

This comment has been minimized.

@alexarchambault

alexarchambault Jan 21, 2018

Member

Same as above.

This comment has been minimized.

@wisechengyi

wisechengyi Jan 23, 2018

Collaborator

done.

module,
version,
attributes = Attributes("", ""),
configuration = configOpt.getOrElse(defaultConfiguration),

This comment has been minimized.

@alexarchambault

alexarchambault Jan 21, 2018

Member

It seems defaultConfiguration isn't taken into account in the new code.

(Yes, the original cli code has poor test coverage :| )

This comment has been minimized.

@wisechengyi

wisechengyi Jan 22, 2018

Collaborator

good catch. will correct this and add a test

wisechengyi added some commits Jan 22, 2018

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 23, 2018

Addressed all the comments, except CI is running into the same error as master does. Please let me know if it's good to go @alexarchambault

wisechengyi added a commit to pantsbuild/pants that referenced this pull request Jan 24, 2018

Single resolve with coursier (#5362)
### Problem

Earlier `jar_dependency`s are divided into classifiers for coursier to resolve because coursier's `--classifier` option is applied globally.

### Solution
Add the classifier specification on module level coursier/coursier#735. This is the pants side of the change which removes the classifier grouping.

### Result

See added back test case. 

Note: finding the correct strict subset jars for transitive dependencies is still an issue coursier/coursier#743. However, it is very, very rare.

stuhood added a commit to pantsbuild/pants that referenced this pull request Jan 24, 2018

Single resolve with coursier (#5362)
### Problem

Earlier `jar_dependency`s are divided into classifiers for coursier to resolve because coursier's `--classifier` option is applied globally.

### Solution
Add the classifier specification on module level coursier/coursier#735. This is the pants side of the change which removes the classifier grouping.

### Result

See added back test case. 

Note: finding the correct strict subset jars for transitive dependencies is still an issue coursier/coursier#743. However, it is very, very rare.
@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 29, 2018

Planning on merging this one in the next 24 hours if no further feedback given.

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Jan 29, 2018

So, merging.

@alexarchambault alexarchambault merged commit 849d128 into coursier:master Jan 29, 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