[Macros] Plugin search options group #66650
Conversation
rintaro
force-pushed
the
macros-plugins-searchopts
branch
from
f18d241 to
090fcda
Compare
| @@ -34,7 +35,6 @@ enum class ModuleSearchPathKind { | |||
| Framework, | |||
| DarwinImplicitFramework, | |||
| RuntimeLibrary, | |||
| CompilerPlugin, | |||
There was a problem hiding this comment.
ModuleSearchPathKind::CompilePlugin was incomplete (only set for -load-plugin-library), and was not actually used.
| enum class SearchPathKind : uint8_t { | ||
| Import = 1 << 0, | ||
| Framework = 1 << 1, | ||
| CompilerPlugin = 1 << 2 |
There was a problem hiding this comment.
Ditto: SearchPathKind::CompilerPlugin wasn't actually used.
| void setCompilerPluginLibraryPaths( | ||
| std::vector<std::string> NewCompilerPluginLibraryPaths) { | ||
| CompilerPluginLibraryPaths = NewCompilerPluginLibraryPaths; | ||
| Lookup.searchPathsDidChange(); |
There was a problem hiding this comment.
Plugin search doesn't use Lookup, and adding plugin search path won't affect import module search path. So we don't need to call Lookup.searchPathsDidChange() when updating PluginSearchOpts.
| pluginLibBasename.append(moduleName.str()); | ||
| pluginLibBasename.append(LTDL_SHLIB_EXT); | ||
|
|
||
| // FIXME: Should we create a lookup table keyed by module name? |
There was a problem hiding this comment.
Currently we create a lookup table for -load-plugin-executable only. But we could lazily create a lookup table for all keyed by module names. But that's need to scan (iterate directory entries) all search paths, so I'm not sure it worth it.
There was a problem hiding this comment.
It's probably better to do the one scan rather than the continual exists checks, but not doing that in this PR is fine IMO.
| @@ -15,7 +15,6 @@ | |||
| // CHECK: -plugin-path: {{.*}}plugins | |||
| // CHECK: -plugin-path: {{.*}}plugins | |||
| // CHECK: -plugin-path: {{.*}}plugins | |||
| // CHECK: -plugin-path: {{.*}}plugins | |||
There was a problem hiding this comment.
Previously manually specified -plugin-path was duplicated in libDriver.
'load-plugin-library', 'load-plugin-executable', '-plugin-path' and '-external-plugin-path' should be searched in the order they are specified in the arguments. Previously, for example '-plugin-path' used to precede '-external-plugin-path' regardless of the position in the arguments.
rintaro
force-pushed
the
macros-plugins-searchopts
branch
from
090fcda to
5791a2c
Compare
| pair.ModuleNames, [&](auto &name) { optStr += name; }, | ||
| [&]() { optStr += ","; }); | ||
| serializationOpts.CompilerPluginExecutablePaths.push_back(optStr); | ||
| // FIXME: Preserve the order of these options. |
There was a problem hiding this comment.
Are you intentionally trying to avoid changing the module serialization format here?
There was a problem hiding this comment.
Yes, my intension is doing it in a followup.
|
@swift-ci Please smoke test |
| pluginLibBasename.append(moduleName.str()); | ||
| pluginLibBasename.append(LTDL_SHLIB_EXT); | ||
|
|
||
| // FIXME: Should we create a lookup table keyed by module name? |
There was a problem hiding this comment.
It's probably better to do the one scan rather than the continual exists checks, but not doing that in this PR is fine IMO.
| case OPT_external_plugin_path: { | ||
| // '<plugin directory>#<plugin server executable path>'. | ||
| // FIXME: '#' can be used in the paths. | ||
| StringRef dylibPath; |
| // RUN: -module-name test \ | ||
| // RUN: -load-plugin-executable %t/libexec/MacroDefinitionPlugin#MacroDefinition \ | ||
| // RUN: -plugin-path %t/lib/plugins \ |
There was a problem hiding this comment.
Add -load-plugin-library here too?
| // RUN: -external-plugin-path %t/external#%swift-plugin-server \ | ||
| // RUN: -plugin-path %t/lib/plugins \ | ||
| // RUN: -load-plugin-executable %t/libexec/MacroDefinitionPlugin#MacroDefinition \ | ||
| // RUN: -o %t/main4 |
|
swiftlang/llvm-project#6998 |
rintaro
requested review from
ahoppen,
artemcm,
hborla,
slavapestov and
xedin
as code owners
load-plugin-library,load-plugin-executable,-plugin-pathand-external-plugin-pathshould be searched in the order they are specified in the arguments.Previously, for example
-plugin-pathused to precede-external-plugin-pathregardless of the position in the arguments.