-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Use the binary version of psycopg for development #2212
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
base: main
Are you sure you want to change the base?
Conversation
680ef1f
to
8b9975f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Django project to use the binary distribution of psycopg instead of the C extension version, switching from psycopg[c]
to psycopg[binary]
to simplify the deployment process.
- Changes the psycopg dependency from the C extension variant to the binary distribution
- Removes PostgreSQL development dependencies from the Docker build process
- Simplifies the Docker image by eliminating compilation requirements
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
requirements/common.txt | Updates psycopg dependency from [c] to [binary] variant |
Dockerfile | Removes PostgreSQL client tools and development libraries no longer needed for binary distribution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gcc \ | ||
libc6-dev \ | ||
libpq-dev \ | ||
zlib1g-dev \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If finally we're going to use psycopg[binary] check if this depenencies are not needed to the compilation of other dependencies (eg: pillow).
If not, and we don't need any more of these system dependencies to compile python packages, I'll invite you to get rid of the whole section of installing debian packages (also compilers), and removing an leaving only the pip install line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gcc and g++ are required for libsass. I looked for an alternative that doesn't require compiling but couldn't find one yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is libsass requiring both gcc and g++ ?
Since you studied it, can we open a PR for the libsass repo to ask a wheel for recent python versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can find an alternate package to replace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check this again.
https://github.com/sass/libsass-python says
Provides prebuilt wheel binaries for Linux, Windows, and Mac.
but our build fails with
77.32 × Building wheel for libsass (pyproject.toml) did not run successfully.
...
77.32 error: command 'gcc' failed: No such file or directory
when those dependencies are not installed.
This feels more a like bug, rather than a missing feature in the libsass package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking.
It seems there isn't a valid python alternative to libsass-python that I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any action needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
As mentioned by @pauloxnet, psql
was only recently removed and re-added, and should be restored before this is merged.
Also, it appears we can solve the compilation issue for local dev and still use the [c]
variant in the production image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fd601f2
to
752036c
Compare
No, I don't intend to keep the version outdated. We can update this PR and close the Dependabot PR, or we can merge this PR, and Dependabot will create another PR to update it. My point was that we don't need both PRs to be merged. |
I updated the dependency version, separated the definitions for dev and prod, reverted the removal of runtime dependencies, and updated the PR title. The CI failure doesn't seem related to the changes in the PR and I could use help to handle it: https://github.com/django/djangoproject.com/actions/runs/17961730284#summary-51086239582 |
752036c
to
e56641b
Compare
I reverted the offending commit for now. Hopefully the build will pass if you rebase again. |
e56641b
to
b76e118
Compare
Thanks you. CI is passing now. I moved the PR out of draft status. |
Replaces
psycopg[c]
withpsycopg[binary]
for development.Relates to #2215