Added QGIS Resource Sharing Plugin and deps

[elpaso]

• dulwich is build with --pure (no C binaries)
• no tests, they can be added later but I'm not sure about where they should go in the source tree

[m-kuhn]

Thanks @elpaso

Just one question, do we need the ext-libs in our source tree? Can we have them as dependencies for the application package instead?

And for the tests, if there are any, I would keep them in the source.

[nyalldawson]

I'm a strong -1 on merging this into master without any unit tests. Doing so places too large a maintenance burden on the project and I'd prefer to see this remain a plugin.

[elpaso]

The tests for the plugin do exist, I only did not know where they should be added (keeping them in the plugin directory as @m-kuhn makes sense to me) and how to tell cmake to build them and travis to execute them (they would not be like the QGIS core python test and I couldn't find any tests for the recently added MetaSearch to get inspired from).
@nyalldawson I think that it was discussed long time ago that it was desirable to have this plugin as core.

[luipir]

hi @nyalldawson I suppose the question by @elpaso is, how to integrate core plugin tests in the QGIS project? Have to be as for Processing tests? this can create misalignment problem with plugin/test version because tests are disjoint from the plugin code.

[elpaso]

@m-kuhn the ext-libs can be wherever you like, actually, they were in the plugin folder and I did some work to move them in the parent ext-libs folder, since all QGIS python libs are there: I just followed an existing pattern.
Just guessing here, because I did not set up this configuration, that the advantage to have them in the parent folder is that the libs can be shared across plugins (this of course does much more sense for requests and six than for dulwich).

[m-kuhn]

@elpaso ext-libs is better than plugin folder, pip install is better than ext-libs

[m-kuhn]

Have to be as for Processing tests? this can create misalignment problem with plugin/test version because tests are disjoint from the plugin code.

Sorry, I think I'm missing the point. What's wrong with the way processing tests are set up?

[elpaso]

@m-kuhn why? pip install is definitely not a viable solution for a core plugin: it must be runnable in any circumstances.

[m-kuhn]

@elpaso we also don't include the Qt sources in the QGIS source tree and that also needs to be around under all circumstances.

It's only a matter of calling it a dependency.

Maybe we should add a requirements.txt file.

[elpaso]

@m-kuhn Sorry, I'm lost, I thought we were talking about python dependencies of core python plugins, are you proposing to remove all ext-libs fom https://github.com/qgis/QGIS/tree/master/python/ext-libs and let the packagers add them?
If this is the case, this has nothing to do with this PR.

[m-kuhn]

I would try to keep ext-libs as small as possible. In doubt, just keep the libs out of the source. Then they might eventually be added by @jef-n because they are missing from osgeo4w.
I'm mainly -1 on prophylactic addition of python deps.

[luipir]

nothing wrong with Processing test env... but just a note about test running detail

ctest -N| grep -i processing
Test #1: ProcessingParametersTest
Test #2: ProcessingQgisAlgorithmsTest
Test #3: ProcessingGdalAlgorithmsTest

ctest -R ProcessingParametersTest
Test project /home/ginetto/PROGRAMMING/QGIS/QGIS-2.14-boundless/build
Start 1: ProcessingParametersTest

1/1 Test <<<<< #1: ProcessingParametersTest ......... Passed 1.07 sec

but there are may unit tests in https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/ParametersTest.py

or something is hided during ctest reporting Processing tests as a whole. I agree that the Processing way IS the way, is there a way to have more unit test detail other than using ctest -V

btw this is out of thread

[m-kuhn]

No, I think it doesn't get any better than ctest -V. But the amount of info you get there depends on CMAKE_BUILD_TYPE=Debug etc.

[elpaso]

@m-kuhn of course, in this particular case the libraries are missing from osgeo4w, do you think I should remove them and let @jef re-add them?
In case I remove them the libs will be needed by the tests anyway, where should I add them in the travis prepare/install ?

[jef-n]

@elpaso not osgeo4w - all distributions might not have packages (at all or recent enough) - running pip isn't an option for packages.

[m-kuhn]

@jef-n "might not" or "might", osgeo4w is normally the first one to trigger when something lacks. As long as it's not only prophylactic it's ok for me.

[jef-n]

@m-kuhn in xenial and yakkety dulwich is too old. unstable and stretch have 0.16.3. gitparseurl isn't available at all. but both are also currently not available on osgeo4w - but could be added (probably easily if pure python)

[wonder-sk]

Whoops, I think that was a bit too early to merge 34 thousands lines of new code!

No QEP, no review ... ?

[luipir]

@wonder-sk about QEP the plugin is (partially or not) covered in: qgis/QGIS-Enhancement-Proposals#58
@wonder-sk do you think it's necessary a specific QEP for the plugin?

[elpaso]

@wonder-sk it's a python plugin resulting from the GSOC work that @akbargumbira did.
It has been briefly discussed here: https://lists.osgeo.org/pipermail/qgis-developer/2017-January/046501.html
And its features has been discussed and requested several times in the past few years.

That said, if you want to revert, I won't fight for this. Just sorry for the huge waste of time.

[elpaso]

@luipir oh, yes there was a also QEP indeed qgis/QGIS-Enhancement-Proposals#58
Honestly, I've always been convinced that it was intended to be core. Sorry if it was just me.

[wonder-sk]

I am just trying to point out that especially larger chunks of code should go through peer review process before getting merged...

[elpaso]

Ok, got the point, but considering that it's a plugin and that it is Python and that it is no new code because the plugin has been around for months I didn't think it was necessary.

I'm not going to revert, but feel free to do that and go through a new PR if you feel like to.

And rest assured that there will be no next time.

[wonder-sk]

@elpaso please don't get me wrong, I really appreciate the great work you do for the project and the resource sharing support will be a very nice addition! (just adding it without allowing a wider discussion about the implementation is a bit unfortunate)

[nyalldawson]

@elpaso thanks for adding the tests!

Another quick retrospective question: who is the maintainer for this code? Will @akbargumbira still be maintaining our are you taking over?

[nyalldawson]

@elpaso

I tried to test the plugin but get an error `cannot import name 'QtWebKitWidgets' I have the package python-qt5-webkit installed and cannot find any other related package on fedora which may provide this dependency.

But... I've thought some more on this, and I would like to revert this change while we open this change up for wider discussion. I'm just not convinced that having this plugin in master is the right move.

Here's my thoughts (please correct me if I'm wrong anywhere!)

Advantages:

• plugin is installed by default

Disadvantages:

• plugin updates are tied to qgis releases. The plugin must be removed from the plugin repo and only distributed with the qgis itself (lessons learnt from processing!). There's no longer anyway to deploy quick fixes.
• moves burden of maintenance from plugin author to core qgis development team. Plugins are a GREAT way to share the development load because individual plugin authors take full responsibility for the regular updates, additions and fixes to their work. Moving this to master discourages this (unless we give the plugin author commit rights)
• master + api is still very much in a state of flux, and adding this plugin at this time slows down the ongoing api fixing and improvement. Python code is a PITA to fix when you are modifying api - the nature of python makes it difficult to determine when methods and certain api calls are used.
• bugs and inconsistencies in behaviour between qgis and the plugin can reflect poorly on qgis itself for these "core" plugins. A clean separation between the main qgis package and 3rd party plugins helps avoid this (blame resides with the plugin, rather than qgis). (Of course, ideally the plugin would be at the same quality, stability and consistently UX to QGIS, and maybe that's already the case!)

We already have a very mature framework for distributing qgis plugins. I personally just can't see that in this case there's a strong enough argument to move this plugin from the plugin repo over to master.

I realise I may be wrong here, so I would like to open up this discussion before we take a step we cannot later back away from!

[elpaso]

who is the maintainer for this code? Will @akbargumbira still be maintaining our are you taking over?

This is my vision about this particular plugin:

• this was a QGIS.org GSOC plugin project, not akbar's, not mine
• the functionality was feature requested several times during the last few years (mainly by @pcav but not only by him)
• the resource sharing feature can also handle processing scripts and it was foreseen to replace the actual script sharing mechanism (which is core)
• the design of the plugin has been discussed (sorry if I forget anybody here) mainly with @rduivenvoorde , @anitagraser , @wonder-sk and @akbargumbira in Girona
• a QEP was prepared and discussed
• weekly reports about the implementation have been sent to the QGIS dev mailing list during the whole development process
• the plugin is about sharing collection of symbols, styles, scripts etc., something like the plugin manager, we really thought that this should have been merged as a core plugin
• the original author has not been particularly responsive lately

So, I asked myself: do we want to let it slowly die unmaintained after all the efforts that we (as a QGIS community) have put in it or should I work on that to complete the original plan (that was to have it eventually in core) and ensure the plugin a future?

I sent an email to the QGIS dev ML about merging it into core and there were no strong objections.

So, I started to port it to QGIS3 (and it took a while) and merged it.

Unless a new maintainer/volunteers and jumps in, the reason for having it in core is exactly to bring its code under the umbrella of QGIS project.
Of course I'm the developer that worked most on the plugin so I'm the natural choice for the maintainance of the plugin but I say it again this is not one my plugins, I like to think that this is an asset of the QGIS project and we should bring it in the project to ensure its future.

Full disclaimer: a rather small part of my time spent working on this plugin has been funded by a private company as a community contribution (they have no interest at all in the plugin itself but I convinced them that it was a good way to contribute back to the project by letting me work on the project during some of my working hours, needless to say that it was not a good idea).

[elpaso]

@nyalldawson

@elpaso

I tried to test the plugin but get an error `cannot import name 'QtWebKitWidgets' I have the package python-qt5-webkit installed and cannot find any other related package on fedora which may provide this dependency.

No clue here, sorry. But I'm pretty sure we can figure it out.

plugin updates are tied to qgis releases. The plugin must be removed from the plugin repo and only distributed with the qgis itself (lessons learnt from processing!). There's no longer anyway to deploy quick fix

That's not entirely correct: you can distribute a core plugin update as a regular plugin and as long as the version is newer it will shadow the core one.

But... I've thought some more on this, and I would like to revert this change while we open this change up for wider discussion. I'm just not convinced that having this plugin in master is the right move.

Here's my thoughts (please correct me if I'm wrong anywhere!)

[...]

moves burden of maintenance from plugin author to core qgis development team. Plugins are a GREAT way to share the development load because individual plugin authors take full responsibility for the regular updates, additions and fixes to their work. Moving this to master discourages this (unless we give the plugin author commit rights)

I generally agree with you, I've explained why I think that this plugin si different in the comment above.

master + api is still very much in a state of flux, and adding this plugin at this time slows down the ongoing api fixing and improvement. Python code is a PITA to fix when you are modifying api - the nature of python makes it difficult to determine when methods and certain api calls are used.

Yes, I might have chosen the wrong time, but when comes to time availability sometimes you have to take it when you have it. What time would have you suggested?

bugs and inconsistencies in behaviour between qgis and the plugin can reflect poorly on qgis itself for these "core" plugins. A clean separation between the main qgis package and 3rd party plugins helps avoid this (blame resides with the plugin, rather than qgis). (Of course, ideally the plugin would be at the same quality, stability and consistently UX to QGIS, and maybe that's already the case!)

We already have a very mature framework for distributing qgis plugins. I personally just can't see that in this case there's a strong enough argument to move this plugin from the plugin repo over to master.

Agreed, but we should make a clear choice here: why MetaSearch yes (really lovely and useful plugin!) and a Resource Sharing Plugin, born and designed as QGIS community project not?

I realise I may be wrong here, so I would like to open up this discussion before we take a step we cannot later back away from!

I agree on most of your statements, but see my other comment for the other arguments.

[elpaso]

reverted

[elpaso]

The idea was to make a new PR but it seems like github does not allow that.

[elpaso]

here is the code: https://github.com/elpaso/QGIS/tree/resource-sharing-plugin