Check jinja version for consent resource

[richvdh]

Fixes #3296

[codecov-io]

Codecov Report

Merging #4327 into develop will decrease coverage by <.01%.
The diff coverage is 51.61%.

@@             Coverage Diff             @@
##           develop    #4327      +/-   ##
===========================================
- Coverage    73.72%   73.72%   -0.01%     
===========================================
  Files          300      300              
  Lines        29751    29770      +19     
  Branches      4878     4884       +6     
===========================================
+ Hits         21935    21947      +12     
- Misses        6388     6393       +5     
- Partials      1428     1430       +2

Impacted Files	Coverage Δ
synapse/config/server.py	62.3% <46.15%> (-1.8%)	⬇️
synapse/python_dependencies.py	45.83% <55.55%> (-1.79%)	⬇️
synapse/handlers/user_directory.py	71.38% <0%> (+0.3%)	⬆️
synapse/app/homeserver.py	57.33% <0%> (+0.33%)	⬆️
synapse/state/v1.py	92.24% <0%> (+1.55%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37f8bdc...f4ad0ba. Read the comment docs.

[erikjohnston]

I'm failing to follow the changes that you're making here? It seems to be doing more than just adding checking jinja versions?

[richvdh]

sorry. The basic problem is that we now require different versions of Jinja2 for consent (2.9) vs email notifs (2.8). I'm slightly hesitant just to bump both to 2.9, mostly because stretch only has 2.8 (although certainly this would be a lot simpler if we didn't care about that).

If we just add "resources.consent": ["Jinja2>=2.9"] to CONDITIONAL_REQUIREMENTS without updating the email notifs entry, we have more problems:

First, check_requirements will complain if Jinja2 2.8 is installed, even if consent is disabled. This is solved by:

1. making check_requirements only enforce core deps by default. I figured the optional deps check wasn't much use here as it stood.
2. Moving the error message code into DependencyException.message (from synapse.app.__init__) so that it can be reused
3. In synapse.config.server.ServerConfig.read_config, adding a call to _check_resource_requirements, which in turn calls check_requirements('resources.consent') if the consent resource is enabled.
4. While we're in there, adding a check that only known resources are being configured.
5. Ideally, we'd litter the code with more calls to check_requirements when the relevant options are enabled, but that's left as an exercise for the future.

Second, list_requirements will list Jinja2 twice, which upsets pip when you do an xargs pip install, especially when the py27-old tox target turns the >= into ==. The solution here is to rewrite list_requirements to merge together requirements for the same project.

[richvdh]

going to rework this to avoid the palaver over supporting different deps for different features

[richvdh]

ok, hopefully this is a bit simpler now.

[erikjohnston]

I think this looks sane!

[erikjohnston]

I'd be tempted to pull this up as a top level template string and then use the multiline quotes, but that's a matter of taste

[richvdh]

yeah, that would have been a good idea. I just c&ped it tbh.

[erikjohnston]

Fair