Add plugins support #1854
Add plugins support #1854
Conversation
| @@ -64,6 +65,8 @@ int main (int argc, char * * argv) | |||
|
|
|||
| settings.maxBuildJobs.set("1"); // hack to make tests with local?root= work | |||
|
|
|||
| initPlugins(); | |||
There was a problem hiding this comment.
Why are all these initPlugins() calls necessary? Can't that be done in initNix() or some other common location?
There was a problem hiding this comment.
It needs to be called after --option flags are parsed, which is done in a bunch of different ways by different programs.
| state.addPrimOp("fetchMercurial", 1, prim_fetchMercurial); | ||
| } | ||
|
|
||
| static RegisterPlugin r(Plugin(PluginTag<PluginType::EvalPlugin>(), reg)); |
There was a problem hiding this comment.
I don't understand why these changes are necessary. Doesn't the RegisterPrimOp constructor run automatically when the plugin is loaded? Why the need for a RegisterPlugin with a lot of template crap?
There was a problem hiding this comment.
It's not necessary, this is all contained in the second commit that I explicitly said could be dropped ;) This was just to drop duplicate capabilities.
Also, a single template is hardly "a lot of template crap".
There was a problem hiding this comment.
... Though I just realized an easy way to avoid the template altogether, patch incoming.
There was a problem hiding this comment.
Well, I don't understand the need for all this over-engineering (e.g. Plugin(PluginTag<PluginType::EvalPlugin>()), plus all that complexity in plugins.hh, which for some reason has a forward-decl to EvalState...
There was a problem hiding this comment.
I removed the tag stuff, was a holdover from an earlier approach. plugins.hh needs to reference EvalState & to definte the EvalPlugin type, but #including eval.hh this low in the header chain could easily lead to accidental references to libnixexpr in libnixstore, if it even worked otherwise.
|
@edolstra Template crap deleted, second commit simplified (though still not needed) |
src/libexpr/primops/fetchGit.cc
Outdated
| @@ -238,6 +237,11 @@ static void prim_fetchGit(EvalState & state, const Pos & pos, Value * * args, Va | |||
| state.allowedPaths->insert(gitInfo.storePath); | |||
| } | |||
|
|
|||
| static RegisterPrimOp r("fetchGit", 1, prim_fetchGit); | |||
| static void reg(EvalState & state) | |||
There was a problem hiding this comment.
This could be inlined with a lambda if desired.
|
@edolstra Continuing out of the source comments: |
|
Well, I still don't understand why RegisterPlugin is needed at all (and why plugin.cc needs to know about EvalState). The only thing that should be needed for plugin support is to do |
|
OK, I'll switch tothat approach. |
src/buildenv/buildenv.cc
Outdated
| @@ -135,6 +136,7 @@ int main(int argc, char ** argv) | |||
| { | |||
| return handleExceptions(argv[0], [&]() { | |||
| initNix(); | |||
| initPlugins(); | |||
There was a problem hiding this comment.
buildenv should not need plugins.
| @@ -158,6 +158,7 @@ int main(int argc, char ** argv) | |||
| { | |||
| return handleExceptions(argv[0], [&]() { | |||
| initNix(); | |||
| initPlugins(); | |||
There was a problem hiding this comment.
This command probably does not need plugins.
All plugins in plugin-files will be dlopened, allowing them to statically construct instances of the various Register* types Nix supports.
|
@edolstra updated |
|
Merged, thanks! Feature request: it would be useful to support loading all .so's in a directory, e.g. to load all plugins provided by installed packages. |
|
@edolstra OK, will do! Thank you! |
Alternative solution to #1841 / #1844.