search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
246 stars 186 forks source link

PLIP: Get rid of portal_quickinstaller #1340

Closed mauritsvanrees closed 8 years ago

mauritsvanrees commented 8 years ago

Proposer : Maurits van Rees

Seconder : @hvelarde

Abstract

No longer support installing via Extensions/install.py.

Motivation

There are two ways of installing products: GenericSetup and CMFQuickInstaller. Checking if a package is currently installed, means having to check portal_setup and portal_quickinstaller, or having code that makes sure those two are in sync. The syncing is mostly in place, with some recent changes, but supporting both remains tricky. Cleanup would make this code much easier to understand.

Assumptions

Some core packages need to actually get a GenericSetup profile instead of their current install.py (for example Products.Marshall). Some need to get an uninstall profile.

Proposal & Implementation

Maurits van Rees

Note that CMFQuickInstaller is Plone specific, in spite of the CMF in the name. So compatibility with CMF is not an issue.

See http://sourceforge.net/p/plone/mailman/message/34390699/ for small initial discussion in August 2015.

mauritsvanrees commented 8 years ago

Working on this at the Alpine City Strategic Sprint 2016 in Innsbruck, Austria. WIP on CMFPlone branch get-rid-of-qi: https://github.com/plone/Products.CMFPlone/tree/get-rid-of-qi The prefs_install_products_form on that branch no longer uses portal_quickinstaller.

hvelarde commented 8 years ago

+1 I think is clear, implemented with enough time in advance, documentation, deprecation warnings and so on; makes a lot of sense.

@keul thoughts? in order to run Python code, are import steps in setuphandlers.py easy and/or powerful enough in your opinion?

keul commented 8 years ago

No, I'm -1 about this.

Setuphandlers are powerful, I always use them, but aren't they related to an installation already running? On the other hand install.py is something can be run at the very beginning, maybe checking Python version (OK, not so useful anymore nowadays) or trying to perform some import assumption.

However I like the fact that magic uninstall operation done by quickinstaller will gone.

mauritsvanrees commented 8 years ago

@keul: You mean you sometimes need to run some code before applying the first import step? I have not needed that so far, but I can imagine it. Fair point. That could easily be handled by this GenericSetup pull request I created earlier: https://github.com/zopefoundation/Products.GenericSetup/pull/24 I created that for the post_handler, but the pre_handler would help you.

keul commented 8 years ago

@mauritsvanrees yep! This can help (hoping to see this merged)

hvelarde commented 8 years ago

@mauritsvanrees what about reinstalls?

mauritsvanrees commented 8 years ago

Reinstalls are not supported. Basically that is already true in the current add-ons control panel.

But with the new setup a reinstall would really just be an uninstall and install, which is easy to do in an add-on if you need it. The old quick installer had a specific reinstall because this did not remove any portal objects that the install had added.

mauritsvanrees commented 8 years ago

Added plip config in coredev: https://github.com/plone/buildout.coredev/blob/5.0/plips/plip1340-get-rid-of-qi.cfg

gforcada commented 8 years ago

And a jenkins job for it: http://jenkins.plone.org/view/PLIPs/job/plip-1340-QuickInstaller/

mauritsvanrees commented 8 years ago

Ah, sweet, thanks.

ebrehault commented 8 years ago

@mauritsvanrees, your PLIP has been approved by the Framework team, and @frapell is the one in charge of review your PR once it will be ready.

mauritsvanrees commented 8 years ago

Note to self. Todo: hidden products are no longer hidden:

frapell commented 8 years ago

@mauritsvanrees Do we have something analogue to the "isProductInstalled" we have in qi ? this is very helpful, and I remember having issues to know if a specific profile for a package was ran or not (for packages that include several profiles)

mauritsvanrees commented 8 years ago

With the new setup, we still have an is_product_installed method (or isProductInstalled as deprecated alias). It gets the default profile of the product. It then checks the only canonical place left: portal_setup. If the default profile has a last version other than unknown then we say it is installed.

This last check is done with a single line that does a call to portal_setup, but for ease of use I have added this as method is_profile_installed on the installer view. You can pass this a full profile id and get back True or False.

mauritsvanrees commented 8 years ago

Random idea out of scope for this plip.

Situation:

If the author of product A is really sure, he may add a metadata.xml in his uninstall profile that depends on the uninstall profile of package B.

Practically speaking: ATContentTypes depends on a profile from Archetypes, but when uninstalling ATContentTypes we are probably not going to run an uninstall profile for Archetypes. Well, in this case we might actually do it: if someone uninstalls ATCT there is no good reason to keep Archetypes installed.

An idea may be to add a cleanup control panel. It may be a part of the add-ons control panel. Or it may be more general, where it can also be a third party add-on (maybe part of ftw.upgrade). For our use case here: add a form that lists all products that are:

Just a few half baked ideas, but if someone wants to take this on, it may be good.

hvelarde commented 8 years ago

I have another idea/request: fire a couple of new events on add-on install and uninstall; if someone give us a hand, we can dedicate some resources to this.

mauritsvanrees commented 8 years ago

This is largely finished. I will need to change plone.app.testing to use the new get_installer instead of QI. And probably I will discover a few more things that need to be changed.

Meanwhile, I would like some feedback on how much deprecation is needed.

For example, the old QI has isProductInstalled. In the new one you should use is_product_installed. But I have added isProductInstalled as allowed name for backwards compatibility. It does roughly the same as before, but there are differences. And all keyword arguments are ignored. Is this useful? Or confusing? If someone does getToolByName(context, 'portal_quickinstaller') then the old name is still there and does the exist same as before. That may be the best backwards compatibility. See this plone.app.upgrade commit for some examples of what needs to be changed in code currently using QI: https://github.com/plone/plone.app.upgrade/commit/93b13114fb6cedf081fccc4e46ab92cd050f3f55 Without the extra backwards compatibility in get_installer we would need a bit of extra code there, using either the old or the new name, but it is not hard. We already need to do slightly different things.

My recommendation: let's remove isProductInstalled and friends from the new installer. It's cleaner.

See https://github.com/plone/Products.CMFPlone/blob/get-rid-of-qi/Products/CMFPlone/GET_RID_OF_QI.rst which shows this and other changes. This file is a rough sketch for a documentation update.

It also has an idea for Plone 5.0 that I would like feedback on: add the new get_installer function and let it return the old portal_quickinstaller with getToolByName.

I came up with this idea, but my recommendation now is: let's not add this function in Plone 5.0.

@hvelarde I thought you listed yourself as seconder, but now I don't see this anymore. Maybe I remember wrongly. But do you have feedback on my questions? Feedback by others is welcome too of course. And a seconder. :-) BTW, I am offline for the next week.

hvelarde commented 8 years ago

@mauritsvanrees sorry about that; let me know how can I help you.

jensens commented 8 years ago

This PLIP is officially under review by the FWT now.

frapell commented 8 years ago

I added my review in https://github.com/plone/buildout.coredev/blob/5.1/plips/reviews/plip1340-review-frapell.rst

I left two suggestions about consistency in the code and a very minor addition to documentation.

Other than that, +1 for merging, it looks fantastic

mauritsvanrees commented 8 years ago

@frapell Thanks for the review. BTW, the review has the title of a different plip. :-)

I have updated the CMFPlone code to use the get_installer function where needed. And I have added a sub section on deprecating upgradeProduct. I have rebased all branches to pull in the latest changes from master and have started a new run of the plip job: http://jenkins.plone.org/view/PLIPs/job/plip-1340-QuickInstaller/5/

mauritsvanrees commented 8 years ago

BTW, on the CMFQuickInstaller branch I have a commit where I comment out two event subscribers. The events notified the old QI that a profile has been installed, so that the QI can register some information about, and register the product as installed.

My intention is to actually not merge that commit. The commit is only there to show that these events are no longer needed in Plone. But to me it seems nicer to keep the events: add-on authors can then still rely on them to work the same way as in Plone 5.0.

frapell commented 8 years ago

@mauritsvanrees Thanks! it is fixed now, I used the other one as template :P

jensens commented 8 years ago

After FWT meeting we decided this one is ready for merge. Thanks @frapell for the review and many more thanks @mauritsvanrees for the idea and implementations.

I'am going to merge now, just need to figure out what :D

hvelarde commented 8 years ago

may I ask where is the documentation before you merge and close this?

jensens commented 8 years ago

Glad you are asking. I found a few occurrences at https://github.com/plone/documentation/blob/5.0/develop/addons/components/genericsetup.rst which need some love. I'll open an issue at the documentation tracker after merge.

mauritsvanrees commented 8 years ago

Good news! Thanks for the reviews.

The documentation is currently a text file in the CMFPLone repo. I intend to use that as basis for an update to the docs. You can open an issue and assign it to me. Note that during the creation of this PLIP I already updated the genericsetup.rst doc to describe the current situation, and mostly the various xml files.

Jens, on what to merge:

jensens commented 8 years ago

Note to self:

plip config at https://github.com/plone/buildout.coredev/blob/5.0/plips/plip1340-get-rid-of-qi.cfg

So we have those merges:

Also all GenericSetup changes were merged already.

Do not forget to create an issue in the docs repository.

jensens commented 8 years ago

@mauritsvanrees if you like to merge this yourself, I'am fine. You know better than I....

mauritsvanrees commented 8 years ago

Yeah, I might as well merge it myself. Will do.

mauritsvanrees commented 8 years ago

As basis for the docs, I have copied the GET_RID_OF_QI.rst from the CMFPlone plip branch to this new docs branch: https://github.com/plone/documentation/tree/plip-1340-deprecate-qi

mauritsvanrees commented 8 years ago

The PLIP job is happy, but I want to do a final check before merging.

Two pull requests should be fine for Plone 5.0:

I test them in combination here: http://jenkins.plone.org/job/pull-request-5.0/1434/

For Plone 5.1 we have the same, plus two extra, for which the packages in 5.0 already use other branches:

I test them in combination here: http://jenkins.plone.org/job/pull-request-5.1/630/

mauritsvanrees commented 8 years ago

The 5.1 job succeeded, the 5.0 one had a few failures that should be easy to fix. Doing that now.

mauritsvanrees commented 8 years ago

The 5.0 job passes too.

But I should test https://github.com/plone/plone.app.upgrade/pull/86 on Plone 4.3 as well: http://jenkins.plone.org/job/pull-request-4.3/406/

mauritsvanrees commented 8 years ago

4.3 passes too. Merging all now.

mauritsvanrees commented 8 years ago

And it's a wrap!

Handling the documentation here: https://github.com/plone/documentation/pull/696