Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Run our oldest supported configuration in CI #3952

Merged
merged 13 commits into from
Sep 27, 2018
Merged

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Sep 25, 2018

No description provided.

…dentity is 16.0+ in all supported distros. pysaml3.0.0 doesn't expose it in the __version__, but we can trust the package managers here.
@hawkowl hawkowl requested a review from a team September 25, 2018 16:24
@@ -64,6 +64,25 @@ setenv =
{[base]setenv}
SYNAPSE_POSTGRES = 1

[testenv:py27-old]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this need listing under envlist ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The envlist is just what's run on tox with no args, so I guess not?

tox.ini Outdated
/usr/bin/find "{toxinidir}" -name '*.pyc' -delete
/bin/sh -c 'python -m synapse.python_dependencies | sed -e "s/>=/==/g" | sed -e "s/psycopg2==2.6/psycopg2==2.7/" | sed -e "s/pyopenssl==16.0.0/pyopenssl==17.0.0/" | xargs pip install'
pip install -e .
coverage run {env:COVERAGE_OPTS:} --source="{toxinidir}/synapse" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we really care about the coverage here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not really.

tox.ini Outdated
lxml
commands =
/usr/bin/find "{toxinidir}" -name '*.pyc' -delete
/bin/sh -c 'python -m synapse.python_dependencies | sed -e "s/>=/==/g" | sed -e "s/psycopg2==2.6/psycopg2==2.7/" | sed -e "s/pyopenssl==16.0.0/pyopenssl==17.0.0/" | xargs pip install'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add some comments explaining the exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

tox.ini Outdated
lxml
commands =
/usr/bin/find "{toxinidir}" -name '*.pyc' -delete
/bin/sh -c 'python -m synapse.python_dependencies | sed -e "s/>=/==/g" | sed -e "s/psycopg2==2.6/psycopg2==2.7/" | sed -e "s/pyopenssl==16.0.0/pyopenssl==17.0.0/" | xargs pip install'
Copy link
Member

@richvdh richvdh Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest one sed with multiple -es rather than multiple seds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -64,6 +64,25 @@ setenv =
{[base]setenv}
SYNAPSE_POSTGRES = 1

[testenv:py27-old]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a comment explaining what this env does (and why)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@hawkowl hawkowl requested a review from a team September 26, 2018 11:35
tox.ini Outdated
commands =
/usr/bin/find "{toxinidir}" -name '*.pyc' -delete
# Make all greater-thans equals, as well as update psycopg2 and pyopenssl
# versions to older versions which still compile on current versions of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/older/newer/?

Could you give more specific examples? which OSes do these libs not compile on? (Travis runs on a pretty ancient os (ubuntu 14.04 ?), so I'd be surprised if there were things which we needed to support for xenial which don't compile on there)

@hawkowl hawkowl requested a review from a team September 27, 2018 10:02
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm otherwise

tox.ini Outdated
commands =
/usr/bin/find "{toxinidir}" -name '*.pyc' -delete
# Make all greater-thans equals so we test the oldest version of our direct
# dependencies, but make the psycopg2 version one which can compile against
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we're going to stare at this in a month's time and try to remember the history here. Could you put

"although we support psycopg2 2.6 (because that's what's in xenial), the Travis build images use postgres 10, which is incompatible with 2.6, so we make 2.7 our minimum. Similarly, blah blah pyopenssl".

tox.ini Outdated
# dependencies, but make the psycopg2 version one which can compile against
# PostgreSQL 10 and the pyopenssl one which can work against an OpenSSL 1.1
# compiled cryptography.
/bin/sh -c 'python -m synapse.python_dependencies | sed -e "s/>=/==/g" -e "s/psycopg2==2.6/psycopg2==2.7/" -e "s/pyopenssl==16.0.0/pyopenssl==17.0.0/" | xargs pip install'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[given we're not running the tests against postgres here, why do we care about psycopg2 at all?]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could just remove it entirely, yeah.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants