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

Dependencies: support installation on macOS #2653

Closed
wants to merge 1 commit into from

Conversation

alopix
Copy link
Contributor

@alopix alopix commented Feb 4, 2023

In order to allow VS Code to show autocompletion and not complain about dependency imports, I tried installing requirements on macOS. This did not work due to a pg dependency.
This PR should fix the installation being supported on macOS as well using conditional dependencies in the requirements.txt.

@hughrun
Copy link
Contributor

hughrun commented Feb 8, 2023

@alopix I don't think I've noticed VS Code complaining - happy to take a look at this but can you provide more context? How are you seeing this bug? Are you running in Docker or directly?

@alopix
Copy link
Contributor Author

alopix commented Feb 8, 2023

@hughrun VS Code will not be able to provide auto completion and know nothing about the packages since they are only available via the Docker context.
image

If you try installing the dependencies directly on macOS to get proper autocompletion etc. you need this requirements.txt workaround on macOS, otherwise the install won’t succeed:

      
      pg_config is required to build psycopg2 from source.  Please add the directory
      containing pg_config to the $PATH or specify the full executable path with the
      option:
      
          python setup.py build_ext --pg-config /path/to/pg_config build ...
      
      or with the pg_config option in 'setup.cfg'.
      
      If you prefer to avoid building psycopg2 from source, please install the PyPI
      'psycopg2-binary' package instead.
      
      For further information please check the 'doc/src/install.rst' file (also at
      <https://www.psycopg.org/docs/install.html>).
      
      [end of output]

@chdorner
Copy link
Member

chdorner commented Feb 13, 2023

I'd be slightly hesitant to simply switch to the binary distribution when run on macOS because the psycopg2 docs advises to use the source one rather than the prebuilt binary ones for production environments. Mainly due to it coming with its own distribution of libpq and libssl, the latter might even cause segfaults with Python's builtin ssl module. This change would effectively mean that a production environment running on macOS would use the binary distribution.

I can see two separate solutions:

  1. Given that we still have a production branch, we might want to switch to psycopg2-binary on the main branch for all platforms, but then ensure that in the production branch we swap this out for the source distribution psycopg2
    • We already remove some development dependencies in the production branch, not sure if this change could cause more merge conflicts when @mouse-reeve merges main into production though. And these kinds of manual merge resolutions can be a source of bugs.
  2. I haven't tested it locally, but surely the psycopg2 source distribution can be built on macOS if the required libpq and libssl requirements are installed correctly. We could expand the developer documentation with examples of how to best go about this on recent versions of macOS.
    • on second thought, I am actually running this native on my Mac without any installation issues having installed PostgreSQL via homebrew. I see libpq installed via homebrew, and I'm guessing it either uses the system's libssl or the one from OpenSSL via homebrew.

@hughrun
Copy link
Contributor

hughrun commented Feb 17, 2023

I also have some concerns about unexpected consequences from messing with the requirements.txt.

I think a better way to solve this for developers (given the reasoning is specifically around VS Code intellisense) is to use Visual Studio Code Dev Containers. I haven't previously been using this, but I've just set it up and it seems to work pretty well. Basically you install the plugin and then it mounts your local dev environment as a volume so that changes are made both inside and outside the running container. Since all the requirements are installed in the container, your VS Code can then go ahead and use intellisense for auto-completion and so on.

It requires a fairly simple .devcontainer/devcontainer.json file:

{
	"name": "Bookwyrm Development",
	"dockerComposeFile": ["../docker-compose.yml"],
	"service": "web",
	"workspaceFolder": "/app",
	 "forwardPorts": [1333, 8888]
}

I'd be happy to add a PR for that so it just comes with the Bookwyrm repo, and update the Bookwyrm dev docs to explain how to do this.

@jaschaurbach
Copy link
Member

I am developing on macOS and I just installed it into my venv by hand.

There is no reason to put the binary into the requirements as they are not required. I have another macOS running where I did not need it as I was running PG not from brew but installed it from source.

This is BTW not only an issue with VS Code, PyCharm CE has the same issue as it defaults to not look inside the docker container.

I am pretty sure that folks who are devs are able to solve this issue without having an extra doc or unnecessary requirements installed.

BookWyrm is not the first project with psycopg2 and macOS-issues and will not be the last one.

@jaschaurbach
Copy link
Member

I suggest to follow the idea of @hughrun and close this PR without merging.

@alopix
Copy link
Contributor Author

alopix commented Mar 21, 2023

Closing this PR. I agree with at least the controversy around which dependency to add—source or binary.

I think "folks who are devs are able to solve this issue without having an extra doc or unnecessary requirements installed" is not the right approach to open up development to everyone and make it as accessible as possible. I think there should at least documentation to show how to easily solve the initial problem described.

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.

None yet

4 participants