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

Add explicit deps from manage.py to settings.py in each app. #31

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Mar 22, 2023

With this change, the example commands in the README work properly
(e.g., pants run helloworld/service/frontend/manage.py -- runserver)

Also explicitly note the BUILDROOT, now that ./pants is no more.

With this change, the example commands in the README work properly
(e.g., pants --concurrent run helloworld/service/frontend/manage.py -- runserver)

Also explicitly note the buildroot, now that `./pants` is no more.
@benjyw benjyw requested review from jsirois and stuhood March 22, 2023 21:43
Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

Although this LGTM on its own, it still seems like maybe a bug that running a main.py directly that is owned by a pex_binary doesn't go through that pex_binary.

@benjyw
Copy link
Contributor Author

benjyw commented Mar 23, 2023

Even if it did go through the pex_binary, that doesn't solve the fundamental issue, it only happens to work in this case because there is a handwritten dep from the pex_binary on all of :lib, which happens to include settings.py.

So I think the issues are orthogonal - manage.py depends on settings.py regardless of any pex_binary, and that dep can't be inferred, so should be written manually.

@benjyw benjyw merged commit 67681a5 into pantsbuild:main Mar 23, 2023
@benjyw benjyw deleted the add_deps_on_settings branch March 23, 2023 03:11
@jsirois
Copy link
Contributor

jsirois commented Mar 23, 2023

Yeah, agreed. The not using an "owning" pex_binary is an orthogonal bug. but it does seem a bug on its own - you set pex_binary knobs (other than dependencies say), and they get ignored. Here it caused effectively a Pants upgrade break, which revealed a better way to structure the dependencies in python_source instead of in pex_binary.

@benjyw
Copy link
Contributor Author

benjyw commented Mar 23, 2023

Well, but you can still run the pex_binary to get that. But if you run the source then A) that is uniform behavior with running sources that don't have a pex_binary, and B) it is a lot faster.

@jsirois
Copy link
Contributor

jsirois commented Mar 23, 2023

But if the pex_binary settings are required for the thing to run at all, say venv_site_packages_copies=True which is required for some known real cases in the world, you get a failure for no reason. It's a strange arrangement that you set up a pex_binary to work, and then running it only works by using the target address. Using the file address just has to be known to fail.

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