-
Notifications
You must be signed in to change notification settings - Fork 323
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
Register services in package.yaml
instead via ServiceLoader
#11868
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
private | ||
|
||
import project.System.File.File_System_SPI | ||
import project.Enso_Cloud.Enso_File | ||
|
||
type Enso_File_System_Impl | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Registration of implementation can be done in a |
||
|
||
File_System_SPI.from (_:Enso_File_System_Impl) = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ensure this works, you should remove EnsoPathFileSystemImpl.java? |
||
File_System_SPI.new "enso" Enso_File |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,11 +47,6 @@ polyglot java import java.nio.file.StandardOpenOption | |
polyglot java import java.time.ZonedDateTime | ||
polyglot java import org.enso.base.DryRunFileManager | ||
polyglot java import org.enso.base.file_system.File_Utils | ||
polyglot java import org.enso.base.file_system.FileSystemSPI | ||
|
||
## PRIVATE | ||
file_types : Vector | ||
file_types = Vector.from_polyglot_array (FileSystemSPI.get_types False) | ||
|
||
## Represents a file or folder on the filesystem. | ||
@Builtin_Type | ||
|
@@ -81,13 +76,13 @@ type File | |
new path = case path of | ||
_ : Text -> if path.contains "://" . not then resolve_path path else | ||
protocol = path.split "://" . first | ||
file_system = FileSystemSPI.get_type protocol False | ||
file_system = File_System_SPI.get_type protocol False | ||
if file_system.is_nothing then Error.throw (Illegal_Argument.Error "Unsupported protocol "+protocol) else | ||
file_system.new path | ||
_ : File -> path | ||
_ -> | ||
## Check to see if a valid "File" type. | ||
if (file_types.any file_type-> path.is_a file_type) then path else | ||
if ((File_System_SPI.get_types False).any file_type-> path.is_a file_type) then path else | ||
Error.throw (Illegal_Argument.Error "The provided path is neither a Text, nor any recognized File-like type.") | ||
|
||
|
||
|
@@ -924,3 +919,16 @@ local_file_move (source : File) (destination : File) (replace_existing : Boolean | |
File_Error.handle_java_exceptions source <| | ||
copy_options = if replace_existing then [StandardCopyOption.REPLACE_EXISTING.to_text] else [] | ||
source.move_builtin destination copy_options | ||
|
||
# PRIVATE | ||
type File_System_SPI | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First step, @radeusgd, is to define an SPI type. It can contain any information needed to process the registered instances. In this case it is |
||
private Entry protocol:Text typ | ||
|
||
new protocol:Text typ = File_System_SPI.Entry protocol typ | ||
|
||
private get_type protocol:Text _:Boolean = | ||
vec = Self.get_types . filter (_.protocol == protocol) | ||
if vec . not_empty then vec.first else Nothing | ||
JaroslavTulach marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private get_types _:Boolean = | ||
Meta.lookup_services File_System_SPI | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To look the implementations up one will be allowed to use |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
package org.enso.interpreter.test.interop; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertNotNull; | ||
|
||
import java.net.URI; | ||
import org.enso.interpreter.node.callable.InteropApplicationNode; | ||
import org.enso.interpreter.runtime.EnsoContext; | ||
import org.enso.interpreter.runtime.callable.UnresolvedConversion; | ||
import org.enso.interpreter.runtime.data.Type; | ||
import org.enso.interpreter.runtime.state.State; | ||
import org.enso.test.utils.ContextUtils; | ||
import org.graalvm.polyglot.Context; | ||
import org.graalvm.polyglot.Source; | ||
import org.junit.AfterClass; | ||
import org.junit.BeforeClass; | ||
import org.junit.Test; | ||
|
||
public class MetaServicesTest { | ||
private static Context ctx; | ||
|
||
@BeforeClass | ||
public static void prepareCtx() { | ||
ctx = ContextUtils.createDefaultContext(); | ||
} | ||
|
||
@AfterClass | ||
public static void disposeCtx() { | ||
ctx.close(); | ||
ctx = null; | ||
} | ||
|
||
@Test | ||
public void loadFileSystemServices() throws Exception { | ||
final URI uri = new URI("memory://services.enso"); | ||
final Source src = | ||
Source.newBuilder( | ||
"enso", | ||
""" | ||
import Standard.Base.System.File.File_System_SPI | ||
import Standard.Base.Meta | ||
import Standard.Base.Enso_Cloud.Enso_File_System_Impl | ||
|
||
data = | ||
Meta.lookup_services File_System_SPI | ||
""", | ||
"services.enso") | ||
.uri(uri) | ||
.buildLiteral(); | ||
|
||
var mod = ctx.eval(src); | ||
var ensoCtx = ContextUtils.leakContext(ctx); | ||
|
||
for (var p : ensoCtx.getPackageRepository().getLoadedPackagesJava()) { | ||
p.getConfig() | ||
.services() | ||
.foreach( | ||
pw -> { | ||
var spiType = findType(pw.provides(), ensoCtx); | ||
var implType = findType(pw.with(), ensoCtx); | ||
var fsImpl = | ||
ContextUtils.executeInContext( | ||
ctx, | ||
() -> { | ||
var conversion = | ||
UnresolvedConversion.build(implType.getDefinitionScope()); | ||
var state = State.create(ensoCtx); | ||
var node = InteropApplicationNode.getUncached(); | ||
var fn = conversion.resolveFor(ensoCtx, spiType, implType); | ||
var conv = node.execute(fn, state, new Object[] {spiType, implType}); | ||
if (conv != null) { | ||
return conv; | ||
} | ||
return null; | ||
}); | ||
assertNotNull("Some implementation found", fsImpl); | ||
assertEquals("Protocol", "enso", fsImpl.getMember("protocol").asString()); | ||
assertEquals( | ||
"Type", | ||
"Standard.Base.Enso_Cloud.Enso_File", | ||
fsImpl.getMember("typ").getMetaQualifiedName()); | ||
return null; | ||
}); | ||
} | ||
} | ||
|
||
private Type findType(String name, EnsoContext ensoCtx) { | ||
var moduleName = name.replaceFirst("\\.[^\\.]*$", ""); | ||
var typeName = name.substring(moduleName.length() + 1); | ||
var module = ensoCtx.getTopScope().getModule(moduleName).get(); | ||
var implType = module.getScope().getType(typeName, true); | ||
return implType; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,53 @@ import org.yaml.snakeyaml.nodes.{MappingNode, Node} | |
import java.io.{Reader, StringReader} | ||
import java.util | ||
import scala.util.Try | ||
import java.io.IOException | ||
|
||
/** Information about registered service. | ||
* | ||
* @param provides name of SPI type | ||
* @param with name of implementation type | ||
*/ | ||
case class ProvidesWith(val provides: String, val `with`: String) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change |
||
|
||
object ProvidesWith { | ||
|
||
/** Fields for use when serializing the [[ProvidesWith]]. */ | ||
object Fields { | ||
val Provides = "provides" | ||
val With = "with" | ||
} | ||
|
||
implicit val decoderSnake: YamlDecoder[ProvidesWith] = | ||
new YamlDecoder[ProvidesWith] { | ||
override def decode(node: Node): Either[Throwable, ProvidesWith] = | ||
node match { | ||
case mappingNode: MappingNode => | ||
val str = implicitly[YamlDecoder[String]] | ||
val bindings = mappingKV(mappingNode) | ||
for { | ||
p <- bindings | ||
.get(Fields.Provides) | ||
.map(str.decode) | ||
.getOrElse(Left(new IOException("Missing `provides` field"))) | ||
w <- bindings | ||
.get(Fields.With) | ||
.map(str.decode) | ||
.getOrElse(Left(new IOException("Missing `with` field"))) | ||
} yield ProvidesWith(p, w) | ||
} | ||
} | ||
|
||
implicit val encoderSnake: YamlEncoder[ProvidesWith] = | ||
new YamlEncoder[ProvidesWith] { | ||
override def encode(value: ProvidesWith) = { | ||
val elements = new util.ArrayList[(String, Object)]() | ||
elements.add((Fields.Provides, value.provides)) | ||
elements.add((Fields.With, value.`with`)) | ||
toMap(elements) | ||
} | ||
} | ||
} | ||
|
||
/** Contact information to a user. | ||
* | ||
|
@@ -110,7 +157,8 @@ case class Config( | |
maintainers: List[Contact], | ||
edition: Option[Editions.RawEdition], | ||
preferLocalLibraries: Boolean, | ||
componentGroups: Option[ComponentGroups] | ||
componentGroups: Option[ComponentGroups], | ||
services: List[ProvidesWith] | ||
) { | ||
|
||
/** Converts the configuration into a YAML representation. */ | ||
|
@@ -160,6 +208,7 @@ object Config { | |
val Edition: String = "edition" | ||
val PreferLocalLibraries = "prefer-local-libraries" | ||
val ComponentGroups = "component-groups" | ||
val Services: String = "services" | ||
} | ||
|
||
implicit val yamlDecoder: YamlDecoder[Config] = | ||
|
@@ -171,6 +220,7 @@ object Config { | |
val normalizedNameDecoder = | ||
implicitly[YamlDecoder[Option[String]]] | ||
val contactDecoder = implicitly[YamlDecoder[List[Contact]]] | ||
val servicesDecoder = implicitly[YamlDecoder[List[ProvidesWith]]] | ||
val editionNameDecoder = implicitly[YamlDecoder[EditionName]] | ||
val editionDecoder = | ||
implicitly[YamlDecoder[Option[Editions.RawEdition]]] | ||
|
@@ -233,6 +283,10 @@ object Config { | |
.get(JsonFields.ComponentGroups) | ||
.map(componentGroups.decode) | ||
.getOrElse(Right(None)) | ||
services <- clazzMap | ||
.get(JsonFields.Services) | ||
.map(servicesDecoder.decode) | ||
.getOrElse(Right(Nil)) | ||
} yield Config( | ||
name, | ||
normalizedName, | ||
|
@@ -243,7 +297,8 @@ object Config { | |
maintainers, | ||
edition, | ||
preferLocalLibraries, | ||
componentGroups | ||
componentGroups, | ||
services | ||
) | ||
} | ||
} | ||
|
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.
To register an implementation one adds
services
section that specifies pair of SPI type and implementation type.@jdunkerley: If you want the IDE to show a drop box with all available providers sooner than the library is loaded, then we might need (localized) display name explaining why such a library should be
import
ed. Engine+ls would deliver list of all possible implementations to the IDE via suggestion db or etc.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.
James would like
Meta.lookup_services
to be able to return even the not enabled services. Then one can create a drop down with a selection of all possible implementation. Question: what happens when a not enabled service is selected? How does one turn that library on?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.
Shall Meta provide something like
Meta.is_library_imported
? Then if not, we could give some feedback to the language server and ask it to insert a missing import.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.
package.yaml
instead viaServiceLoader
#11868 originally tend to achieveE.g. if we continue to insist on solving the problem with not enabled services we effectively put this PR on hold.