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
Enable to specify include and exclude resource patterns with glob #3562
Enable to specify include and exclude resource patterns with glob #3562
Conversation
This commit enables scala-native to specify include and exclude resource patterns with glob. This will allow users to reduce the size of their executables by excluding unnecessary resources. To use this feature, users can specify the include and exclude patterns using the `NativeConfig`'s `withResourceIncludePatterns` and `withResourceExcludePatterns` APIs. For example, to exclude the `rootdoc.txt` file, users would specify the following pattern: ```scala _.withResourceIncludePatterns(Seq("**.txt")) .withResourceExcludePatterns(Seq("rootdoc.txt")) ``` The default behavior is to include all resources (`**`), excluding `/scala-native/**`. (Note that, scala-native will exclude the resources `/scala-native/**` anyway, even users update the exclude patterns). Also, note that this featuer is using Java's PathMatcher, which behave a bit different from the posix glob. https://docs.oracle.com/javase/tutorial/essential/io/find.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, great that you've include additional scripted tests. I have a few comments with possible improvements
def withResourceIncludePatterns(value: Seq[String]): NativeConfig = { | ||
copy(resourceIncludePatterns = value) | ||
} | ||
|
||
def withResourceExcludePatterns(value: Seq[String]): NativeConfig = { | ||
copy(resourceExcludePatterns = value) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add variants for appending a single pattern, these might be sometimes easier to use, eg:
config
.withResourceIncludePattern("foo")
.withResourceIncludePatterns(config.resourceIncludePatterns :+ "foo")
However, at that time we would need to handle the default **
pattern. We can do it here by checking if resourceIncludePatterns eq DefaultIncludePattern
and then replace the sequence, or otherwise append to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally feel having two APIs: "appending a single pattern" and "replace the patterns with the given seq" seems confusing, and even complicated with the special handling with replacing the default pattern.
Maybe we can compromise with the API that takes varargs instead of Seq so users can write something like .withResourceIncludePatterns("foo")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stay with the current solution then using Seq, that way it would be aligned with remaining settings
def apply(config: Config): Seq[Defn.Var] = Scope { implicit scope => | ||
val classpath = config.classPath | ||
val includePatterns = | ||
config.compilerConfig.resourceIncludePatterns.map(p => | ||
FileSystems.getDefault().getPathMatcher(s"glob:$p") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can factor out default file system to common val
s"Include patterns: ${config.compilerConfig.resourceIncludePatterns | ||
.mkString(", ")}; " + | ||
s"Exclude patterns: ${(config.compilerConfig.resourceExcludePatterns :+ ScalaNativeExcludePattern) | ||
.mkString(", ")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part could be cached as it needs to be computed only once, so it can be appended to s"Did not embed: $pathName - "
But, even better we can refactor it out check which exactly pattern was applied to exclude given path. So we'd have something like this
val notInIncludePatterns = s"Not matched by any include path ${includePatterns.mkString}"
type IgnoreReason = String
def shouldIgnore(path: Path): Option[IgnoreReason] =
includePatterns.find(_.matches(path)).fold(notInIncludePatterns){ includePattern =>
excludePatterns.find(_.matches(path).map(excludePattern => s"Matched by $includePattern by excluded by $excludePattern")
...
shouldIgnore(path) match {
case Some(reason) => config.logger.debug("Did not embed: $path - $reason")
case None => ???
}
Weird that the |
Co-authored-by: Wojciech Mazur <wojciech.mazur95@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I've just spotted as single typo
tools/src/main/scala/scala/scalanative/codegen/ResourceEmbedder.scala
Outdated
Show resolved
Hide resolved
…r.scala Co-authored-by: Wojciech Mazur <wojciech.mazur95@gmail.com>
😍 can't wait to see it released and ported into scala-cli as well |
fix #3433
This commit enables scala-native to specify include and exclude resource patterns with glob. This will allow users to reduce the size of their executables by excluding unnecessary resources.
To use this feature, users can specify the include and exclude patterns using the
NativeConfig
'swithResourceIncludePatterns
andwithResourceExcludePatterns
APIs. For example, to exclude therootdoc.txt
file, users would specify the following pattern:The default behavior is to include all resources (
**
), excluding/scala-native/**
. (Note that, scala-native will exclude the resources/scala-native/**
anyway, even users update the exclude patterns).Also, note that this featuer is using Java's PathMatcher, which behave a bit different from the posix glob.
https://docs.oracle.com/javase/tutorial/essential/io/find.html