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.

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

Add plugins support #1854

Merged
merged 1 commit into from Feb 13, 2018

Conversation

Projects
None yet
2 participants
@shlevy
Copy link
Member

shlevy commented Feb 8, 2018

Alternative solution to #1841 / #1844.

@shlevy shlevy requested a review from edolstra Feb 8, 2018

@@ -64,6 +65,8 @@ int main (int argc, char * * argv)

settings.maxBuildJobs.set("1"); // hack to make tests with local?root= work

initPlugins();

This comment has been minimized.

@edolstra

edolstra Feb 8, 2018

Member

Why are all these initPlugins() calls necessary? Can't that be done in initNix() or some other common location?

This comment has been minimized.

@shlevy

shlevy Feb 8, 2018

Author Member

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));

This comment has been minimized.

@edolstra

edolstra Feb 8, 2018

Member

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?

This comment has been minimized.

@shlevy

shlevy Feb 8, 2018

Author Member

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".

This comment has been minimized.

@shlevy

shlevy Feb 8, 2018

Author Member

... Though I just realized an easy way to avoid the template altogether, patch incoming.

This comment has been minimized.

@edolstra

edolstra Feb 8, 2018

Member

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...

This comment has been minimized.

@shlevy

shlevy Feb 8, 2018

Author Member

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.

@shlevy shlevy force-pushed the shlevy:plugins branch from 9375776 to 0894c43 Feb 8, 2018

@shlevy

This comment has been minimized.

Copy link
Member Author

shlevy commented Feb 8, 2018

@edolstra Template crap deleted, second commit simplified (though still not needed)

@@ -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)

This comment has been minimized.

@shlevy

shlevy Feb 8, 2018

Author Member

This could be inlined with a lambda if desired.

@shlevy

This comment has been minimized.

Copy link
Member Author

shlevy commented Feb 8, 2018

@edolstra Continuing out of the source comments: plugins.hh is ready to support multiple plugin types for two reasons: 1) You mentioned wanting to move s3binarycachestore to plugins and 2) it's very cheap forward compatibility.

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Feb 8, 2018

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 dlopen on the .so files, the RegisterPrimOp and RegisterStoreImplementation constructors should take care of the registration.

@shlevy

This comment has been minimized.

Copy link
Member Author

shlevy commented Feb 8, 2018

OK, I'll switch tothat approach.

@@ -135,6 +136,7 @@ int main(int argc, char ** argv)
{
return handleExceptions(argv[0], [&]() {
initNix();
initPlugins();

This comment has been minimized.

@edolstra

edolstra Feb 8, 2018

Member

buildenv should not need plugins.

@@ -158,6 +158,7 @@ int main(int argc, char ** argv)
{
return handleExceptions(argv[0], [&]() {
initNix();
initPlugins();

This comment has been minimized.

@edolstra

edolstra Feb 8, 2018

Member

This command probably does not need plugins.

Add plugins to make Nix more extensible.
All plugins in plugin-files will be dlopened, allowing them to
statically construct instances of the various Register* types Nix
supports.

@shlevy shlevy force-pushed the shlevy:plugins branch from 0894c43 to 88cd2d4 Feb 8, 2018

@shlevy

This comment has been minimized.

Copy link
Member Author

shlevy commented Feb 8, 2018

@edolstra updated

@edolstra edolstra merged commit 88cd2d4 into NixOS:master Feb 13, 2018

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Feb 13, 2018

Merged, thanks!

Feature request: it would be useful to support loading all .so's in a directory, e.g.

plugin-files = ${config.system.path}/lib/nix/plugins

to load all plugins provided by installed packages.

@shlevy

This comment has been minimized.

Copy link
Member Author

shlevy commented Feb 13, 2018

@edolstra OK, will do! Thank you!

@shlevy shlevy deleted the shlevy:plugins branch Feb 13, 2018

@badi badi referenced this pull request Feb 13, 2018

Merged

Issue 2018/03 #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.