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

Copy local artifact to cache #831

Merged
merged 3 commits into from Apr 26, 2018

Conversation

Projects
None yet
3 participants
@dotordogh
Contributor

dotordogh commented Apr 1, 2018

@alexarchambault, is this what you meant in #798? I just want to see if I'm on the right track.

@alexarchambault

This comment has been minimized.

Member

alexarchambault commented Apr 4, 2018

@dotordogh Yep, CachePaths.localFile should be changed like you did. The code around here should be adjusted too (it decides whether a url should be downloaded and written to the destination file, or assumed to be already there).

Also, maybe that feature could be put behind a flag in a first time… Support for local:/ could be enabled no matter what (as it doesn't break things). And iff the flag is enabled, things from file:/ would be copied to the cache (else current behavior). That way, in the cli tool, users could be warned if they use file:/ without the flag, that they should use local:/ instead, but that wouldn't change anything else for them for now.

There might be other stuff to change too (ivy2HomeUri, file:/ repos from sbt should be changed to local:/, …)

@dotordogh

This comment has been minimized.

Contributor

dotordogh commented Apr 5, 2018

@alexarchambault got it! Thank you!!

@dotordogh

A few questions for you @alexarchambault, see the inline comments. Thank you in advance!!

// testFile.delete()
//
// assertThrows[Exception]({

This comment has been minimized.

@dotordogh

dotordogh Apr 6, 2018

Contributor

@alexarchambault what is the best way to catch an error in a test. This doesn't seem to work with the way errors are handled.

This comment has been minimized.

@alexarchambault

alexarchambault Apr 9, 2018

Member

@dotordogh It depends on what you want to test… If you just want to test that the warning about file:/ is printed, I guess tests for that particular point aren't needed…

This comment has been minimized.

@dotordogh

dotordogh Apr 12, 2018

Contributor

@alexarchambault I was hoping to test that the fetch would fail because the artifact would be missing after deleting it. Is there a way to do that with how we handle errors?

This comment has been minimized.

@alexarchambault

alexarchambault Apr 13, 2018

Member

I though something like would work, but the assert seems false…

val artifactOptions = ArtifactOptions(force = true)
val commonOpt = CommonOptions(jsonOutputFile = jsonFile.getPath)
val fetchOpt = FetchOptions(common = commonOpt, artifactOptions = artifactOptions)

// fetch with encoded url set to temp jar
val fetch = Fetch(
  fetchOpt,
  RemainingArgs(
    Seq(
      "e:f:g,url=" + encodedUrl
    ),
    Seq()
  )
)

assert(fetch.helper.anyError)

The idea would be just to try to avoid the sys.exit that makes the test exit.

Overall, the cli module should be reworked so that it doesn't call sys.exit too deep in the code this way, but that's how it is for now… :|

@@ -73,7 +73,13 @@ object CacheFetchTests extends TestSuite {
* - {
// test that everything's fine with basic file protocol
val repoPath = new File(getClass.getResource("/test-repo/http/abc.com").getPath)
check(MavenRepository(repoPath.toURI.toString))
// check(FallbackDependenciesRepository(Map((Module("com.github.alexarchambault", "coursier_2.11"), "1.0.0-M9-test") -> (repoPath.toURI.toString, true))))

This comment has been minimized.

@dotordogh

dotordogh Apr 6, 2018

Contributor

@alexarchambault I also wanted to test the file/local protocol path in the cache tests but I can't seem to import FallbackDependenciesRepository to be able to resolve the local file path. Am I doing this wrong? Is there a different way you would suggest?

@wisechengyi

Thanks @dotordogh! Please see comments.

There is another story in additional to artifacts with urls specified, i.e. apply the same concept to repos.

E.g.

fetch -t junit:junit:4.12 -r local:///.m2/repository

vs

fetch -t junit:junit:4.12 -r file:///.m2/repository

but we can prioritize it separately.

@@ -796,10 +796,10 @@ object Cache {
val file = localFile(url, cache, artifact.authentication.map(_.user))
def res =
if (url.startsWith("file:/")) {
if (url.startsWith("local:/")) {

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

local:/ should probably be a constant somewhere in the core, in order to avoid a lot of duplications in the rest of the review as well.

nit: like http:// being the protocol, local:// (double /) would follow the convention better.

This comment has been minimized.

@alexarchambault

alexarchambault Apr 9, 2018

Member

Not necessarily… file:/path/to/something is valid (no "authority" part), and doesn't start with file://.

}
}.toMap
private def convertStringToUrl(url: String): URL = {

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

might need some docstring for this. iiuc this defines the behavior to obtain URLConnection for anything with local://?

if (url.startsWith("local"))
new URL(null, URLDecoder.decode(url, "UTF-8"), new URLStreamHandler {
override def openConnection(u: URL): URLConnection = {
val toFileUrl = new URL(url.toString.replace("local", "file"))

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

bug: replace -> replaceFirst

depsWithUrls.foreach { case (moduleVersion: (Module, String), urlBool: (URL, Boolean)) =>
if (!cacheFileArtifacts && urlBool._1.toString.startsWith("file:/"))
errPrintln(s"Warning: Please use the 'local:/' protocol for ${urlBool._1.toString}")

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

Example error msg I saw:

Warning: Please use the 'local:/' protocol for file:/Users/me/.m2/repository/junit/junit/4.12/junit-4.12.jar

nit:

  • local:/ -> local://
  • file: -> file://
  • to make the message a bit more clear, append to prevent it from being copied into cache.

This comment has been minimized.

@alexarchambault

alexarchambault Apr 9, 2018

Member

Same as above, local:/ and file:/ are ok.

@Help("Specify if artifacts with urls with 'file://' protocol should be cached. " +
"Defaults to false, meaning artifacts with urls beginning with 'file://' will " +
"not be cached unless this is set to True. In this case, please use the 'local://' protocol.")

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

In this case, please use the 'local://' protocol.

It could be a bit more clear. If the flag is true, please use the 'local://' protocol to prevent artifacts from being fetched to cache.

"Defaults to false, meaning artifacts with urls beginning with 'file://' will " +
"not be cached unless this is set to True. In this case, please use the 'local://' protocol.")
@Short("cla")
cacheFileArtifacts: Boolean = false

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

By cla, do you mean cacheLocalArtifacts?

This comment has been minimized.

@dotordogh

dotordogh Apr 12, 2018

Contributor

Maybe cfa is better here? I just wanted something shorter than cacheFileArtifacts for when I was testing it on the command line. I'm open to suggestions!

This comment has been minimized.

@wisechengyi

wisechengyi Apr 12, 2018

Collaborator

Either works as long as it's consistent :)

(jsonFile, _) => {
withFile("tada", "coursier-fetch-test", ".jar") {
(testFile, _) => {
val path = testFile.getAbsolutePath
val encodedUrl = encode("file://" + path, "UTF-8")
val commonOpt = CommonOptions(jsonOutputFile = jsonFile.getPath)
val commonOpt = CommonOptions(jsonOutputFile = jsonFile.getPath, cacheFileArtifacts = true)

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

Given this the first run should put the artifact in cache already, right? so there's no need for second run.

This comment has been minimized.

@dotordogh

dotordogh Apr 9, 2018

Contributor

The second part of this test checks to see that what was fetched was actually from the cache and not the local file.

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

Ah I see. sounds good!

* Result:
* |└─ e:f:g
*/
"local dep url" should "have coursier-fetch-test.jar but should fail when file is removed" in withFile() {

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Collaborator

This is a great test case!

On the other hand, there should be another case with a:b:c,url=file://<path> --cla, so

  1. the first run put the file into cache
  2. then remove the file locally
  3. re-resolve should still work because it's in the cache.

However it seems to fail currently:

$ fetch -t junit:junit:4.12,url=file%3A%2F%2F%2FUsers%2Fme%2F.m2%2Frepository%2Fjunit%2Fjunit%2F4.12%2Fjunit-4.12.jar%20  --no-default  --cla  --json-output-file x.out
  Result:
└─ junit:junit:4.12
/Users/me/Library/Caches/Coursier/v1/file/Users/me/.m2/repository/junit/junit/4.12/junit-4.12.jar

# move the repo, so the file://... isn't valid anymore
$ mv ~/.m2/repository/ ~/.m2/repository2

# this should still succeed by looking at the cache, instead of trying to look at file://...
$ fetch -t junit:junit:4.12,url=file%3A%2F%2F%2FUsers%2Fyic%2F.m2%2Frepository%2Fjunit%2Fjunit%2F4.12%2Fjunit-4.12.jar%20  --no-default  --cla  --json-output-file x.out
  Result:
└─ junit:junit:4.12

Error:
  junit:junit:4.12
    junit-4.12.jar not found under file:/Users/yic/.m2/repository/junit/junit/4.12/

FAILURE: java coursier.cli.Coursier ... exited non-zero (1)

This comment has been minimized.

@dotordogh

dotordogh Apr 9, 2018

Contributor

Strange, the previous test checks this and passes. I'll look into this tomorrow. I'm not sure why it's behaving this way.

This comment has been minimized.

@dotordogh

dotordogh Apr 12, 2018

Contributor

Alright, so looked into this a bit deeper and strangely, the fetch in the test above doesn't fail, but on the command line it does. I think (from what I can tell) the hack of using the FallbackDependenciesRepository is what is causing the problem because it doesn't have caching behavior associated with it. @alexarchambault, could you confirm this?

This comment has been minimized.

@dotordogh

dotordogh Apr 13, 2018

Contributor

I've resolved this with my most recent commit, but I'm still stuck on trying to assert that a fetch failed without the artifact present. At the moment its causing the tests to exit, and asserting that an exception or error are thrown are not enough to catch the state. Additionally, I'd also love to get the test in CacheFetchTests working but I can't figure out how to import the FallbackDependenciesRepository. Hopefully @alexarchambault can help with those issues.

@wisechengyi

This comment has been minimized.

Collaborator

wisechengyi commented Apr 20, 2018

Hey @alexarchambault, I was wondering if adding --cache-file-artifacts flag should be sufficient in this case, so there is no need to invent a new protocol local://, reason beings:

  1. local:// might cause some confusions
  2. --cache-file-artifacts default being false does not alter the existing behavior
@alexarchambault

This comment has been minimized.

Member

alexarchambault commented Apr 20, 2018

@wisechengyi That would be fine this way too, yes.

Dorothy Ordogh

@dotordogh dotordogh changed the title from [Work In Progress] Copy local artifact to cache to Copy local artifact to cache Apr 21, 2018

@dotordogh

This comment has been minimized.

Contributor

dotordogh commented Apr 21, 2018

I removed the test that checked that the fetch would fail because I can't catch the sys.exit.

@alexarchambault

This comment has been minimized.

Member

alexarchambault commented Apr 24, 2018

@dotordogh The CI failure seems legit.

Dorothy Ordogh
@dotordogh

This comment has been minimized.

Contributor

dotordogh commented Apr 24, 2018

@alexarchambault sorry, I missed this! Updated it! Hopefully CI will be green after this 😄

Dorothy Ordogh
@alexarchambault

This comment has been minimized.

Member

alexarchambault commented Apr 26, 2018

So merging, thanks!

@alexarchambault alexarchambault merged commit c469899 into coursier:master Apr 26, 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