Skip to content

Circular dependency in Makefile #10447

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

Closed
di opened this issue Dec 6, 2021 · 4 comments · Fixed by #10803
Closed

Circular dependency in Makefile #10447

di opened this issue Dec 6, 2021 · 4 comments · Fixed by #10803
Labels

Comments

@di
Copy link
Member

di commented Dec 6, 2021

From #10278 (comment):

I believe this change has introduced a circular dependency, as perceived by these messages:

$ make -v | head -1
GNU Make 3.81
$ make build
make[1]: Circular requirements/docs.txt <- .state/env/pyvenv.cfg dependency dropped.
...

For funsies, I tried the same with remake, which uses a newer version of make:

$ remake --version | head -1
GNU Make 4.3+dbg-1.5
$ remake build
remake[1]: Entering directory '/Users/miketheman/workspace/miketheman/warehouse'
remake[1]: Circular requirements/docs.txt <- .state/env/pyvenv.cfg dependency dropped.
remake[1]: Circular requirements/lint.txt <- .state/env/pyvenv.cfg dependency dropped.
remake[1]: '.state/docker-build' is up to date.
...

I don't know if this is harmful yet, but I'm having other issues getting started with development, and wondered if this is somehow related or not.

Originally posted by @miketheman in #10278 (comment)

@ewjoachim
Copy link
Contributor

ewjoachim commented Dec 6, 2021

Reposting my comment:
I think we could mostly solve the problem by making the targets more granular, but at core we need to solve something: in order to install pip-compile, we need to know which version to install it too, which means we need to run pip-compile and so on.
If we install pip-compile without the hashes, it kinda defeats the purpose of using hashes in the first place.

So one solution could be to not disclose to make that the pip-compile target needs the pip-compile requirement file, considering that it doesn't often change, and the venv gets recreated once in a while anyway.

So we would have:

  • A target for venv creation but not installation of dependencies
  • a target for pip-tools that would depend on that one, which would actually use the requirements/pip-tools.txt file but wouldn't say it
  • requirements/*.txt would depend on that one
  • the full environement target, the one everyone uses, would use the relevant .txt files and declare it.

I believe it would work.

@di
Copy link
Member Author

di commented Dec 6, 2021

Or, we could take this as an opportunity to move more towards #4948, since it doesn't make a ton of sense right now for us to compile dependencies on one platform (the host OS) to be used on a different platform (inside the container images), which removes the need to build dependencies in the virtual environment.

@abitrolly
Copy link
Contributor

Why not to vendorize pip-compile?

@abitrolly
Copy link
Contributor

For example with pex.

pip install pex
pex pip-tools --console-script pip-compile --output-file pip-compile.pex
-rwxr-xr-x 1 root root 4750609 Dec  7 10:53 pip-compile.pex

abitrolly added a commit to abitrolly/warehouse that referenced this issue Dec 11, 2021
This removes circular dependency in Makefile added in pypi#10278

Unblocks pypi#10052
Fixes pypi#10446
Fixes pypi#10447
abitrolly added a commit to abitrolly/warehouse that referenced this issue Dec 11, 2021
This removes circular dependency in Makefile added in pypi#10278

Unblocks pypi#10052
Fixes pypi#10446
Fixes pypi#10447
abitrolly added a commit to abitrolly/warehouse that referenced this issue Dec 11, 2021
This removes circular dependency in Makefile added in pypi#10278

Unblocks pypi#10052
Fixes pypi#10446
Fixes pypi#10447
@di di added the bug 🐛 label Feb 18, 2022
@di di closed this as completed in #10803 Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants