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 3355: Merge into one repository where circular dependencies can not be resolved. #3355

Closed jensens closed 2 years ago

jensens commented 2 years ago

PLIP (Plone Improvement Proposal)

Dear @plone/framework-team - I am happy to submit this PLIP

Responsible Persons

Proposer: Jens Klein @jensens

Seconder: Timo Stollenwerk @tisto

Abstract

Merge non-resolvable circular depended packages into one if woven tight together.

Motivation

Plone consists out of several Python packages. Idea was to be able to test a package and its functionality in its own.

Some of our packages do not follow this path. We are having circular dependencies all over the place. Some of them can be resolved with minor to medium effort.

Other are woven together without any chance to separate them again. Those are addressed by this proposal.

Releases are much simpler and faster i there are less packages.

Future pull requests are not spanning that many repositories. At the moment often up to 5 or more repositories are to be touched for simple features or fixes. This costs developer and reviewer time. The entry barrier for new developers is high. Future refactorings are simpler.

Assumptions

Proposal & Implementation

Preparation

A plip-3355-package-merge.cfg will be created in order to review and test the implementation.

Process

Selection of packages

The primary idea is to merge some plone.app.* packages into Products.CMFPlone. For candidates see https://github.com/plone/Products.CMFPlone/issues/3304 We may identify other namespaces for mergers too, but this is out of scope of this PLIP.

Attention: It could be considered to make packages really independent. One candidate here is probably plone.app.portlets, which has only a few refactor-able dependencies in plone.app.layout and CMFPlone. This is out of the scope of this PLIP. We just do not merge those packages for now. That said, further evaluation is needed here.

Following a helper list of candidates:

Merge circulars

Fix circulars and use as addon?

Other small packages worth to think of a merger

Overall process of moving

First: Moving is done package by package and not all at once. Otherwise it would conflict to much with ongoing PRs. The time a merger-PR is open should be as short as possible.

We have a SOURCE (the package we want to get rid of) and a TARGET (which afterwards contains the code of SOURCE).

  1. SOURCE should have no active open pull requests
  2. Check SOURCE for additional information to keep, like the history (CHANGES.rst) or documentation.
  3. Create branch on SOURCE where all unneeded files are removed and all to be kept are moved to a folder keep.
  4. if TARGET have its code in /src and SOURCE not, move code in SOURCE (or vice versa).
  5. prepare a branch on TARGET where source will be merged into.
  6. merge SOURCE into target
  7. fix TARGET setup.py/setup.cfg to provide the new namespace.
  8. on target decide how to proceed with the kept files. SOURCES CHANGES.rst can be renamed to CHANGES-{SOURCE-namespace}. An introduction text should explain why it is here and that it wont be used anymore.
  9. Add a section to the README of TARGET with the contained namespaces, add a news/ entry to TARGET with the changes and point to the CHANGES.RST.
  10. create a new branch on SOURCE with a major version bump, where all code and namespaces are gone (empty package). Add to SOURCE, README and CHANGES explaining what happened.
  11. Test it

Working with GIT

Example and proof of concept merging plone.app.layout into Products.CMFPlone. Checkout both in your local workspace:

git clone git@github.com:plone/Products.CMFPlone.git
git clone git@github.com:plone/plone.app.layout.git

Prepare the to-be-merged repository al conflicting files must be deleted, renamed or moved.

cd plone.app.layout
mkdir keep
git mv CHANGES.rst keep

check if news is empty, if not copy to keep as above, otherwise remove as below

git rm -r CONTRIBUTING.rst docs .gitattributes .gitignore MANIFEST.in news/ pyproject.toml README.rst setup.py setup.cfg

And commit the changes:

git commit -m"prepare merger with Products.CMFPlone"

Back to CMFPlone, create a branch to be merged into there and push it:

cd ../Products.CMFPlone/
git cb merge-with-p.a.layout
git push --set-upstream origin merge-with-p.a.layout

Now add the plone.app.layout repository as a new remote pal and checkout the branch created before:

git remote add pal git@github.com:plone/plone.app.layout.git
git fetch pal
git co merge-to-cmfplone

Finally we can merge the branch into CMFPlone:

git co merge-with-p.a.layout
git merge merge-to-cmfplone --allow-unrelated-histories

The outcome of exact above can be seen here: https://github.com/plone/Products.CMFPlone/tree/merge-with-p.a.layout

Deliverables

Risks

Low.

Participants

tisto commented 2 years ago

@jensens +1 go go go!

jensens commented 2 years ago

@jensens +1 go go go!

@tisto May I add you as seconder?

tisto commented 2 years ago

@jensens sure!

jensens commented 2 years ago

@plone/framework-team if I get green light I prepare this for a merger of plone.app.layout with Products.CMFPlone. I am sure this will be detected at LIGO.

tisto commented 2 years ago

@plone/framework-team @MrTango @ale-rt can we get some more votes on this?

mauritsvanrees commented 2 years ago

Seems a fine plan. Git history is kept, so git blame works. I would be very happy with less packages!

Might need 'git merge --allow-unrelated-histories'. Edit: Ah, you already have it. And 'git filter-repo', which I recently used, to rewrite history. See https://stackoverflow.com/a/10548919

Maybe add everything in src. filter-repo helps there.

We can debate the list. For example, I would rather have p.a.contentrules as dependency of the Plone package, so not merged here.

jensens commented 2 years ago

I would love to have real dependencies back where possible, p.a.contentrules is a good example. We may identify more (in both directions?)

ale-rt commented 2 years ago

I feel and share your pain.

But I think that a brutal merge of whole packages in to Products.CMFPlone is not good for the future, especially while they are so "Plone classic oriented" plone.app.layout. And I would not exactly call this operation "resolution of circular dependencies".

I would solve that by actually better structuring our code.

For example check:

This is clearly some Plone classic related stuff that should be better placed in another package (plone.app.content itself? a new plone.classic_or_whatever package?).

I think the same can be said of lots of other packages, e.g. plone.app.layout. I do not see the point in moving this code in to Products.CMFPlone today and remove or re-move it tomorrow.

So I am +100 on resolving circular dependencies, -1 in moving all this code in to 1 repo.

jensens commented 2 years ago

Yesterday I started to untangle plone.app.portlets from plone.app.layout in order to use plone.app.portlets in some future Plone as an addon. Yesterdays work is a first small step. Next larger step is to untangle plone.app.portlets from Products.CMFPlone. We here speak about all the configuration and tests. This is possible but some effort!

OK, lets have a look at most plone.app.* packages.

I identified these major problems to solve :

  1. An low hanging fruit would be to get rid of Products.CMFPlone.utils.* imports. We would need to move those needed in other packages in a new *sigh* plone.base (have fun looking for a better name!) package.
  2. Products.CMFPlone.interfaces.* is imported at a bunch of places and creates circular dependencies. Also a candidate for a plone.base package.
  3. Products.CMFPlone.permissions is also used all over the place. Again a candidate for plone.base.
  4. PloneMessageFactory import -> just define again in a package. But why not put it in plone.base if we are going to have it...

Core packages in plone.app.* with problematic indirections to CMFPlone:

Packages which are OK

Packages which are OK as consumers of CMFPlone:

mauritsvanrees commented 2 years ago

@ale-rt Are you thinking of a possible future where we have a stripped-down core Plone release without the classic stuff? So say without most templates and browser views and control panels? For example:

Something like that?

mauritsvanrees commented 2 years ago

@jensens Do you see anything really go wrong due to circular imports? Theoretically it is certainly better to avoid it. In practice we have had circular imports for years, and they work.

And does merging plone.app.layout into the Products.CMFPlone repo actually help if these two parts of the code still import from each other? You still have a circular import, it is just strictly speaking in the same package.

ale-rt commented 2 years ago

@ale-rt Are you thinking of a possible future where we have a stripped-down core Plone release without the classic stuff? So say without most templates and browser views and control panels? For example:

  • plone.volto would depend on plone.restapi and plone.core.
  • plone.classic would depend on plone.core and plone.app.portlets, plone.app.theming, etc.

Something like that?

I think it really make sense to write a PLIP that propose that.

From my talks with quite some people from both the volto and classic teams, that would make sense.

In that way we would have a solid and reliable core on top of which people can build their frontend stuff. The FWT will take care that this core will be maintained in such a way that Volto, Plone classic or whatever the future will bring us can use it without harm.

For historical reason this is not the case: we have lots of classic UI stuff buried deep in our core. I see no urge in fixing that, but I would like to avoid adding more of this in Products.CMFPlone.

I think I have the consensus of the @plone/framework-team here (correct me if I am wrong).

tisto commented 2 years ago

We indeed discussed this in the FWT and I fully agree that in the long run, we should have a Plone core with two consumers, Plone Classic and Volto (via REST API).

Though, we have to be careful to not move everything around at the same time. I see @jensens PLIP as a first step in this direction without breaking anything for now. Maybe we should only move packages that are needed in a future "Plone core" and leave the ones out that serve Plone Classic, so that we don't have to move them first-in-then-out.

I could imagine to go in small steps with this PLIP. First merge the low-hanging-fruits, do an alpha release and see how that goes. Then approach the next batch, discuss the findings again, then move on.

jensens commented 2 years ago
  1. Meanwhile - after my research above - I am not that keen to follow this PLIP in the first place. Maybe this is then the logical next step and then move stuff around into a combined new plone.core (including CMFPlone). I am more with @ale-rt here to first get our homework done and fix our dependencies.
  2. @mauritsvanrees It hurts my software-architect-heart to have imports in both directions between packages. It is bleeding. It does work as we have been very careful to not create real circular imports (otherwise Python breaks). Sometimes therefore imports are done in a function/method.
  3. I would really like to create a plone.base package with Interfaces, permissions, basic utils This would help to untangle a lot. Maybe this should go into an own PLIP for 6.0 (we are still alpha and it would not be a breaking change given depredations are set everywhere)?

I tend to close this PLIP and start over with a bigger plan, maybe a Roadmap towards 7.0 (with plone.core).

mauritsvanrees commented 2 years ago
  1. I would really like to create a plone.base package with Interfaces, permissions, basic utils This would help to untangle a lot. Maybe this should go into an own PLIP for 6.0 (we are still alpha and it would not be a breaking change given depredations are set everywhere)?

For me, this would be fine.

plone.base should have only very few dependencies. zope.interface, zope.schema, maybe Products.CMFCore. I think you have about 90 percent of the utils/permissions/interfaces covered then, and then we will see what to do with the remaining code that would need extra dependencies at the moment. Maybe INavigationRoot needs to be in plone.base as well. That would go a long way towards untangling CMFPlone and p.a.layout. Also: the old locations should do backwards compatibility imports, so code (and data) does not break.

ale-rt commented 2 years ago

I like the way things developed :) I just wonder if adding a plone.base in the play is doable in a reasonable time, also considering the fact that we have to fix all the imports or we will have tons of deprecation warnings to fix.

tisto commented 2 years ago

What if we start to prepare things in Plone 6 (come up with an architecture and plan, add deprecation messages, document what we want to do, move things that we can move without breaking anything) and then do the actual thing in Plone 7?

I think we have to take lots of things into account and take the big picture into account:

Long before Plone 6 was started I made a proposal in the FWT that Plone 6 should be Plone 5.2 + Volto + DX site root, to make the transition for people super easy and then drop Plone 5.2/Python2 support soon. Doing the release management for Plone 6 will be a challenge in any case. We have to choose our battles here IMHO. The FWT members agreed with this idea back then but it never had any real consequences when voting for PLIPs or reviewing PLIPs.

In general, we have two options for Plone 6+:

a) make the transition from Plone 5.2 to Plone 6 easy, have a Plone 6 final release soon, drop Plone 5.2 soon, move on soonish to Plone 7/8/9. The FWT has to become more restrictive with PLIPs then. Though, part of this PLIP could easily go into Plone 7 because it can happen sooner than later (no need to support Plone 5.2 since forever).

b) make Plone 6 a "big breaking changes release" with even more breaking changes and be prepared to support Plone 5.2 for a long period of time. This PLIP could go into Plone 6 then, though, we still would have to deprecate imports in Plone 5.2. Though, this would require a motion by the @plone/release-team to commit to a longer support period of Plone 5. My personal option is that we put enough pressure on the release team already. It will also make it less attractive for Volto folks to move to Plone 6, which is not what we want I guess.

To sum things up. I am all for this PLIP in general and I can't wait to get started with things. Though, we have to be careful with breaking backward compatibility, because this has lots of consequences we need to be aware of. We need to keep the big picture in mind, the marketing story we tell, and the amount of work for the release team that comes with that story.

gforcada commented 2 years ago

My 5 cents, for what's worth:

introducing for Plone 6 an almost empty plone.core package gives us the opportunity to keep moving interfaces, utilities, etc etc during the plone 6 time frame, with strict deprecation warnings and backward compatible imports to be removed at the time of plone 7.

If the idea is to eventually have this dual volto + plone.restapi + plone.rest + plone.core and plone.core + barceloneta LTS then it might be a good idea to also create, at the time of plone 6 release, another almost empty package plone.layout (?) to keep moving all the UI code that is not meant to be on plone.core and its dependencies.

jensens commented 2 years ago

I am -1 for more breaking changes in 6.0 and a release soon. That said, having deprecation warnings would be fine. And so a move of functions and interface-classes to a plone.base package would not break anything. A plone.core is something for Plone 7

esteele commented 2 years ago

Agreed. I’ve been asking for this for years, so I’m really excited to see it get some traction, but its too much to add into Plone 6 at this point.

rodfersou commented 2 years ago

What if we start to prepare things in Plone 6 (come up with an architecture and plan, add deprecation messages, document what we want to do, move things that we can move without breaking anything) and then do the actual thing in Plone 7?

sounds like a solid plan to me 👍

rodfersou commented 2 years ago

I tend to close this PLIP and start over with a bigger plan, maybe a Roadmap towards 7.0 (with plone.core).

makes sense to break this work in multiple smaller PLIP's

jensens commented 2 years ago

Close in favor of #3395