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
Do not allow any more core c++ plugins #85
Comments
+1 Core C++ plugins make no sense for the the points raised in the QEP |
YES YES YES! |
I am not that familiar with differences between QGIS's core plugins and plugins or all of QGIS's many plugins. Is this going to restrict plugin development to Python only? Why not have a plugin that allows C++ extensions like R has, where the 'Rcpp' package (plugin) allows c++ extensions or 'inline' which wraps C++ to perform as an R function. Execution speed vs. development time makes sense especially when a process takes forever, as it can do in some GIS applications. Would it be possible to have one plugin to allow the easy addition of C++ or other language extensions to QGIS and then this discussion would be a moot point? pyQGIS could then make available those QGIS capabilities the developer wanted exposed. |
This QEP only effects the concept of core C++ plugins that we ship by default. Normal plugin development C++ or otherwise is not affected. |
+1 |
2 similar comments
+1 |
+1 |
But just for clarity: I think it is good to have a good clean cpp plugin api, so third parties can hook up certain functionality which is too heavy to be python, but also not desirable as core stuff. Let's not close possibilities which make QGIS expandable for the rest of the world. Is the possibilty @sildeag easier/better then our current api? +1 for the arguments to not pull it in QGIS/QGIS repo though... |
@rduivenvoorde that's why this qep explicitly states "It's important to note that this does not block 3rd parties from creating and distributing c++ plugins if desired. It just prevents these plugins being introduced into the master QGIS codebase." ;) |
Ah... duh..../me bad |
Accepted due to enough +1s |
With exceptions for existing grass and globe plugin? Doesn't that mean could be others as well? |
Exisiting plugins are not affected. Only no new ones
…On Sat, 17 Dec 2016 7:52 pm Jürgen Fischer ***@***.***> wrote:
With exceptions for existing grass and globe plugin? Doesn't that mean
could be others as well?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#85 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3H3duTSvbjahMjIywvuiJlWyRlrzks5rI7DFgaJpZM4LOzEq>
.
|
That doesn't answer the question. If there are existing plugins that can't (or shouldn't) be integrated into core, that might go for others as well (new plugins). I don't see why this needs to be explicetly regulated. Integration into core is preferable of course - but there might still be cases in which it's not desirable. E.g. even if globe and grass could be integrated, you might still not want to to avoid a strong runtime dependency to their dependencies. Or there might be plugins that offer interesting features, we want - but the contributor doesn't want to port them to core. Not that I know of any - but that also raises the question why there has to be a new rule. We'll always look into what we want to merge and what not anyway. |
@nyalldawson, et al., Just to clarify, this restriction does not extend to new authentication method plugins or data providers, correct? Currently, there is no means of creating/registering a new auth method plugin via PyQGIS (only C++ for now); however, this may be part of a QEP I am putting together for the refactoring and security-hardening of the auth system. |
Hey Larry,
I don't think it includes those as they aren't really plugins. it's more
referring to anything that uses the QGisInterface plugin interface e.g
these kinds of plugins:
[image: Inline image 1]
…On Mon, Dec 19, 2016 at 8:16 AM, Larry Shaffer ***@***.***> wrote:
@nyalldawson <https://github.com/nyalldawson>, et al.,
Just to clarify, this restriction does extend to new authentication
method plugins <https://github.com/qgis/QGIS/tree/master/src/auth> or
data providers, correct?
Currently, there is no means of creating/registering a new auth method
plugin via PyQGIS (only C++ for now); however, this may be part of a QEP I
am putting together for the refactoring and security-hardening of the auth
system.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#85 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3B9Lx9OcyHXNqycScbMlEjQI_y7Mks5rJbDCgaJpZM4LOzEq>
.
|
@dakcarto to clarify, this applies only to the plugins which are able to be replaced with standard python plugins. So providers and auth plugins are not affected. |
@jef-n my views:
Because (as as with the other general code QEPs) it helps define the guidelines of what's acceptable for inclusion in QGIS. Having a definite statement is easier for new contributors to understand and base their work around. I believe this is more helpful than having someone do work as a c++ plugin with the intention of merging it to master and only then being told they need to redo it as integrated in app/analysis. Take for instance the last time this happened, with sourcepole's geometry checker/snapper plugins. Great, highly desirable features and worthy of inclusion in the base QGIS package. But unfortunately implemented as c++ plugins, meaning that they couldn't be reused by things like processing. By the time the PRs were submitted it was too late to change this (too much work already done on the plugin version) and they've been stuck as c++ plugins since. It's only recently when I've donated time to porting the snapper to analysis which has allowed it to be added as a processing alg and so made infinitely more useful than the separate plugin. I don't want this situation EVER occurring again! Much easier to just have a blanket guideline: "you want it in master, integrate it yourself".
Globe:
Grass:
Regardless of how interesting, I'd rather put the requirement on them (while their code isn't merged and they still have motivation) to do this porting. As soon as something's merged this motivation is usually lost (eg due to payment of sponsorship) and it falls back to the handful of regularly contributors to maintain more of other people's work. (Off topic: but it's the same reason I'm usually so picky with PR reviews. Every bit of code we merge into master without good documentation, unit tests or messy API adds more burden to you, me and a couple of other overworked people! My philosophy is usually "make the contributor pay this time, because otherwise you'll have to pay it yourself".) . There's also the possibility that c++ plugins can still be written, packaged and deployed separately from QGIS, firmly leaving the maintaince burden with the original author. Phew... Hopefully that doesn't come across too rant-y, but it's just my explanation why we need guidelines like this. |
+1 on this approach. However, I do concur with @jef-n that it does not necessarily need to be so restrictive. For example, the guideline could read like: As a general rule, core C++ plugin contributions are no longer accepted. Such contributions should be integrated directly into QGIS's libraries or application. Any contribution that requires a core plugin, e.g. adds significant dependencies, will need project voting member approval, since this increases project maintenance overhead. I figure the restriction of voting member approval will be enough of a deterrent, unless the contributor feels the integration needs to be a plugin and is willing to put forth an argument. Side note: I've had success configuring my general and auth method plugin libraries to be statically linked to the CTest This method for unit testing is not enough to argue against this QEP, as there is still no simple way to create If there could be a reasonable means of unit testing and adding bindings, then should we not consider the main advantage of using plugins, e.g. when a plugin adds significant, though optional, build and runtime dependencies? A lack of linked dependencies at runtime will not crash QGIS, but just fail to load the plugin. |
you can write a c++ plugin, compile a c++ plugin...but, qgis will not load the plugin....It makes me confused. by the way, i used qt creator to write a plugin. And then i set the folder to settings->optional->system->plugin path. But it just didn't work. |
QGIS Enhancement: Do not allow any more core c++ plugins
Date 2016/12/16
Author Nyall Dawson (@nyalldawson)
Contact nyall dot dawson at gmail dot com
Version QGIS 3.0
Summary
It seems the general consensus that c++ core plugins are undesirable. Some reasons why include:
Generally, these core c++ plugins are a historical holdover from the time before python plugins were possible, and their functionality would now be better suited to be either part of the core QGIS code (eg in the analysis library) or as standard python plugins available via the plugin repository.
Proposed Solution
While informal consensus is that c++ plugins should be avoided, no formal decision has been made regarding their place in future QGIS releases. Accordingly, this QEP proposes that we mandate that no more c++ plugins can be introduced to the QGIS code, without exceptions. If someone wants to contribute a new c++ core plugin they must instead rework their code to introduce the changes in the analysis/app libraries or rewrite in python and publish via the plugin repository.
It's important to note that this does not block 3rd parties from creating and distributing c++ plugins if desired. It just prevents these plugins being introduced into the master QGIS codebase.
Example(s)
Specific examples about why the existing c++ plugins are not ideal and ideas for fixing them is available at qgis/qgis4.0_api#67
Performance Implications
Should speed up compilation and launch of QGIS
Further Considerations/Improvements
See qgis/qgis4.0_api#67 for plans regarding existing core c++ plugins.
Backwards Compatibility
N/A
Votes
(required)
The text was updated successfully, but these errors were encountered: