Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow for globs in probe.resolvers.intellij.repositories config #285

Merged
merged 11 commits into from
Oct 19, 2022
Merged

Allow for globs in probe.resolvers.intellij.repositories config #285

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0cf9f75
Allow for globs in probe.resolvers.intellij.repositories config
LukaszKontowski Sep 22, 2022
4a5ef3b
Fix globes logic and allow for multiple globes in one pattern
LukaszKontowski Sep 26, 2022
aab9a1c
Add example with description in reference.conf
LukaszKontowski Sep 27, 2022
88413db
Fix misspell in reference.conf description
LukaszKontowski Sep 27, 2022
8319647
PR fixes - fix resolving logic and improve code style
LukaszKontowski Oct 17, 2022
cf8b83f
Merge branch 'feature/enable_globs_in_repositories_config' of github.…
LukaszKontowski Oct 17, 2022
b5f6fa7
Fi logic for checking las part of the candidate path
LukaszKontowski Oct 17, 2022
b7f601c
fix logic for non-local patterns
LukaszKontowski Oct 17, 2022
c6c43a4
Improve code style - use String#split
LukaszKontowski Oct 17, 2022
93ce1ec
Revert "Improve code style - use String#split"
LukaszKontowski Oct 17, 2022
dd834df
Improve test scanarios
LukaszKontowski Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions core/api/src/main/scala/org/virtuslab/ideprobe/ProbeExtensions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ trait ProbeExtensions {
path.getFileName.toString
}

def exists: Boolean = {
Files.exists(path)
}

def isFile: Boolean = {
Files.isRegularFile(path)
}
Expand Down
43 changes: 42 additions & 1 deletion .../driver/sources/src/main/scala/org/virtuslab/ideprobe/dependencies/IntelliJResolver.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
package org.virtuslab.ideprobe.dependencies

import java.nio.file.Paths

import scala.annotation.tailrec

import org.virtuslab.ideprobe.IntelliJFixture
import org.virtuslab.ideprobe.config.DependenciesConfig

import org.virtuslab.ideprobe.Extensions.PathExtension

trait IntelliJResolver {
def community: DependencyResolver[IntelliJVersion]
def ultimate: DependencyResolver[IntelliJVersion]
Expand Down Expand Up @@ -37,9 +43,44 @@ case class IntelliJPatternResolver(pattern: String) extends IntelliJResolver {
"version" -> version.releaseOrBuild,
"release" -> version.release.getOrElse("snapshot-release")
)
val replaced = replacements.foldLeft(pattern) { case (path, (pattern, replacement)) =>
val replacedBeforeResolvingGlobs = replacements.foldLeft(pattern) { case (path, (pattern, replacement)) =>
path.replace(s"[$pattern]", replacement)
}
val replaced = replaceGlobsWithExistingDirectories(List(replacedBeforeResolvingGlobs)).head
LukaszKontowski marked this conversation as resolved.
Show resolved Hide resolved
Dependency(replaced)
}

// solution below assumes that each * character is used to mark one part of the path (one atomic directory),
// for example: "file:///home/.cache/ides/com.jetbrains.intellij.idea/ideaIC/[revision]/*/ideaIC-[revision]/"
@tailrec
private def replaceGlobsWithExistingDirectories(paths: List[String]): List[String] =
if (paths.exists(pathMightBeValidResource))
paths.filter(pathMightBeValidResource)
else
replaceGlobsWithExistingDirectories(paths.flatMap(replaceFirstFoundWildcardWithDirectories))

private def pathMightBeValidResource(path: String): Boolean =
path.indexOf('*') == -1 &&
LukaszKontowski marked this conversation as resolved.
Show resolved Hide resolved
(path.endsWith(".zip") ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it too specific check? Filling extension is responsibility of pattern replacement that already happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the check is needed to find proper path from multiple directories that exist in place of * (some are not valid).
But I just realised that this check is not valid. This check should return true only when path ends properly, as defined in the pattern... Let me fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, logic of pathMightBeValidResource method is fixed now.

path.endsWith(".dmg") ||
Paths.get(path.replace("file:", "")).isDirectory)
LukaszKontowski marked this conversation as resolved.
Show resolved Hide resolved

private def replaceFirstFoundWildcardWithDirectories(path: String): List[String] = {
def removeLastFileSeparator(path: String): String = if (path.endsWith("/")) path.init else path

val pathUntilWildcard = Paths.get(path.substring(0, path.indexOf('*')).replace("file:", ""))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay... so a ready library for globs wasn't an adequate solution here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not manage to test these libraries yet. Wanted to have a working solution first. I'm not sure if we need them here. Suggested docs https://docs.oracle.com/javase/tutorial/essential/io/find.html don't seem helpful to me in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, leaving up to your judgement

val stringPathAfterWildcard = path.substring(path.indexOf('*') + 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use split for that? It allows to specify limit of splits.

Copy link
Contributor Author

@LukaszKontowski LukaszKontowski Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but then I would have to use three vals instead of two:

val Array(stringPathUntilWildcard, stringPathAfterWildcard) = path.split("*", 2)
val pathUntilWildcard = Paths.get(stringPathUntilWildcard.replace("file:", ""))

If you think that would be better than current solution, I can do that. Let me know, what do you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's do this. It is same number of lines, but 2 substring calls with some index offset are more complex for reading and understanding than a simple split.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem connected to replacing String#indexOf with String#split
The String#split(String regex, int limit) method treats the String parameter "*" as a regular expression, which leads to invalid results.

I would suggest to switch back to previous logic:

val pathUntilWildcard = Paths.get(path.substring(0, path.indexOf('*')).replace("file:", ""))
val stringPathAfterWildcard = path.substring(path.indexOf('*') + 1)

to be sure that * wildcard will not be used as a regular expression.

Copy link
Contributor

@lukaszwawrzyk lukaszwawrzyk Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be sure, just use regex as expected r"\*" or "[*]"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but the thing is - I don't want java String's method to treat the * as a regular expression. And String#split by default treats the String parameter as a regular expression. That's why I want to stick to current logic, without using split. So that * will never be treated as a regex.


if (pathUntilWildcard.exists) {
pathUntilWildcard
.directChildren()
.map { replaced =>
(if (path.startsWith("file:")) "file:" else "") + removeLastFileSeparator(
replaced.toString
) + stringPathAfterWildcard
}
} else {
Nil
}
}
}
16 changes: 16 additions & 0 deletions ...ver/sources/src/test/scala/org/virtuslab/ideprobe/dependencies/IntelliJResolverTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import org.junit.runners.JUnit4

import org.virtuslab.ideprobe.Config
import org.virtuslab.ideprobe.ConfigFormat
import org.virtuslab.ideprobe.IntelliJFixture
import org.virtuslab.ideprobe.config.DependenciesConfig

@RunWith(classOf[JUnit4])
Expand Down Expand Up @@ -47,6 +48,21 @@ class IntelliJResolverTest extends ConfigFormat {
assertExists(artifactUri)
}

@Test
def resolvesIntelliJPatternWithGlobesUsed(): Unit = {
val probeConfig = IntelliJFixture.readIdeProbeConfig(
Config.fromString(s"""
|probe.resolvers.intellij.repositories = [
| "$mavenRepo/com/*/intellij/*/$mavenArtifact/${mavenVersion.build}/$mavenArtifact-${mavenVersion.build}.zip"
|]
|""".stripMargin),
"probe"
)
val repo = IntelliJResolver.fromConfig(probeConfig.resolvers).head
val artifactUri = repo.resolve(mavenVersion)
assertExists(artifactUri)
}

private def assertExists(dependency: Dependency): Unit = dependency match {
case Dependency.Artifact(uri) =>
assertTrue(s"Resolved invalid artifact: $uri", Files.exists(Paths.get(uri)))
Expand Down