Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pre-installation of universal2 environment for OSX tests #1234

Merged
merged 4 commits into from
Nov 3, 2024

Conversation

perrinjerome
Copy link
Contributor

We need to pre-install the same versions that will be installed by buildout later. For most distributions there is a constraint in constaints.txt that we keep up to date with buildout.cfg, but not for MarkupSafe

@perrinjerome
Copy link
Contributor Author

The tests are failing nowadays because we pre-install latest MarkupSafe , which today is 3.0.2, but buildout requires 3.0.1 so it needs to install a new version and can not use the pre-installed version.

This might not be the best way of addressing this, I don't fully understand how the versions in constraints.txt, tox.ini and buildout.cfg are kept in sync, but this seem to be an explanation and a possible solution for the test failures on OSX

@dataflake
Copy link
Member

Version pins are maintained in versions-prod.cfg and versions.cfg. The constraints and requirements files are auto-generated by running the buildout or by executing the util.py script in the root of the repository.

@perrinjerome
Copy link
Contributor Author

Thanks, then this looks like a bug in that script, it reads only versions-prod.cfg. versions-prod.cfg contains the versions of packages required to run zope in production and versions.cfg contains the versions of packages required to run zope in production, run tests suite and build documentation with sphinx.

Do you think the script should be amended so that it generates constraints.txt from versions.cfg ( and keep generating requirements.txt from version-prod.cfg ) ?

I can try to do this

@perrinjerome perrinjerome force-pushed the perrinjerome/macosxfails branch from 1b232b8 to e11cb58 Compare October 30, 2024 04:28
@perrinjerome
Copy link
Contributor Author

I did this and replaced this pull request with the new approach, now util.py includes all versions in constraints.txt (and requirements-all.txt is not changed).

requirements file still contains only prod versions (from
versions-prod.cfg) but constraints is extended to also contain versions
from versions.cfg, so that during tests we can install extras with pip,
like we do for universal2 on OSX.

This fixes a problem of test failing on OSX because the version pin for
MarkupSafe was missing and when installing it with pip, the latest
version was installed, which might not be the version defined in
versions.cfg, causing buildout to install again MarkupSafe at the version
defined in versions.cfg and failing.
@perrinjerome perrinjerome force-pushed the perrinjerome/macosxfails branch from e11cb58 to 5f47da9 Compare October 30, 2024 04:35
@dataflake
Copy link
Member

No please do not touch that script. It is fine as is. I was mistaken in my description. It uses versions-prod.cfg. The goal is to just have the minimum requirements for running Zope and not huge files that pull in the world for stuff that is not needed in a production setting.

@perrinjerome
Copy link
Contributor Author

OK, but in case we misunderstood each other: this change does not change the "requirements" file, only the "constraints" file, so when we install with pip install --requirement requirements-full.txt it still installs just the minimum requirements exactly same as today, what's different is only the constraints file, so that when we install with pip install --constraint constraints.txt zope.interface ... MarkupSafe like it's done in tox.ini these packages are installed with the same version constraints as when installing with buildout.

@dataflake
Copy link
Member

My problem right now is that I do not have access to a computer this week and I can only look at things on my phone. It's hard for me to cross-check anything or review anything other than small changes. I'll need to defer this PR until I am back home, or @icemac can chime in.

@perrinjerome
Copy link
Contributor Author

Sure, no problem, let's wait.

If this approach is problematic, another idea I just have is that we could keep util.py as it is, generating requirements and constraints files from versions-prod.cfg and use the same approach to generate a constraints-test.cfg from versions.cfg and then change tox.ini to use -c {toxinidir}/constraints-test.txt instead of -c {toxinidir}/constraints.txt.

@perrinjerome
Copy link
Contributor Author

use the same approach to generate a constraints-test.cfg from versions.cfg and then change tox.ini to use -c {toxinidir}/constraints-test.txt instead of -c {toxinidir}/constraints.txt.

this would be #1237

@dataflake
Copy link
Member

I personally would much prefer this approach over #1237.

As I said, the original goal was to have a smaller minimum requirements list for production that does not pull in the world during install. So that's why requirements-full.txt only uses versions-prod.cfg as basis. (Side note: IMHO these files should be renamed to requirements.txt and something like versions-required.cfg to reflect their purpose better).

I think it's perfectly fine to let constraints.txt grow instead of creating yet another file in the repository root. So please go forward with this PR and kill #1237.

@perrinjerome
Copy link
Contributor Author

Thanks ! I also believe it's not really problematic to include the constraints for test dependencies like this. I'm merging now, it's for bad timing with the release, I guess it would have made it easier if you had green CI.

@perrinjerome perrinjerome merged commit d54a989 into master Nov 3, 2024
28 checks passed
@perrinjerome perrinjerome deleted the perrinjerome/macosxfails branch November 3, 2024 14:03
dataflake added a commit that referenced this pull request Nov 3, 2024
@dataflake
Copy link
Member

The extended constraints file is published at https://zopefoundation.github.io/Zope/releases/master/constraints.txt and will automatically flow into the next releases.

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

Successfully merging this pull request may close these issues.

2 participants