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

The setup-py goal generates arguably incorrect requirements for in-repo code #1126

Closed
jsirois opened this issue Feb 18, 2015 · 5 comments
Closed
Assignees
Labels

Comments

@jsirois
Copy link
Contributor

jsirois commented Feb 18, 2015

The example case here is pantsbuild.pants.contrib.scrooge aka //contrib/scrooge/src/python/pants/contrib/scrooge:plugin: https://github.com/pantsbuild/pants/blob/master/contrib/scrooge/src/python/pants/contrib/scrooge/BUILD

This has transitive deps on code owned by pantsbuild.pants aka //src/python/pants:pants-packaged: https://github.com/pantsbuild/pants/blob/master/src/python/pants/BUILD#L11

Instead of generating an sdist for pantsbuild.pants.contrib.scrooge with a setup.py install_requires including pantsbuild.pants, it just inlines the transitive source code it uses from pantsbuild.pants.

The upshot is that one cannot simply pip install pantsbuild.pants.contrib.scrooge and get a fully functioning pants install with the scrooge plugins included, one has to instead pip install pantsbuild.pants pantsbuild.pants.contrib.scrooge.

@jsirois jsirois added the bug label Feb 18, 2015
@jsirois
Copy link
Contributor Author

jsirois commented Feb 18, 2015

@jsirois
Copy link
Contributor Author

jsirois commented Mar 24, 2015

@stuhood - the fact that the pantsbuild.pants.contrib.scrooge sdist has pantsbuild.pants sources it uses embedded is known. It may or may not be related to your issue as modelled in #1317

@jsirois
Copy link
Contributor Author

jsirois commented Mar 24, 2015

I'm actually pretty convinced this is tied to the #1317 issue on OS X and it is import order dependent. To actually fix this import order dependence (if a file from a pantsbuild.pants package is imported from pantsbuild.pants.controb.scrooge and sibling files in the package only exist in pantsbuild.pants because scrooge does not use them - boom) this issue will need to be fixed.

If this issue will take more time to resolve than is desirable for you (I expect this will be the case), then an option I support would be to make an auxiliary target - say in contrib/scrooge/BUILD - that depends on both src/python/pants:pants_transitional_publishable_binary and contrib/scrooge/src/python/pants/contrib/scrooge/tasks and src/python/pants/goal:task_registrar and has a setup_py. Burning and using that 1 sdist would work around.

@jsirois
Copy link
Contributor Author

jsirois commented Mar 24, 2015

OK - I think the fix is 2-part:

  • change plugin targets to depend on pants-packaged
  • fix the minified_dependencies algorithm in setup_py.py

That fix is encompassed by the last 3 commits in #1317 which goes green on local linux, travis linux and travis OS X as well as having no more overlap in sdist file ownership as confirmed by the pex listing done in CIs.

I'm going to sleep on this and then if all still seems sane with this fix in the am, add test coverage and break out new issues to cover - maybe - making this more natural. The new "change plugin targets to depend on pants-packaged" rule would be nice to avoid since its antithetical to the usual practice of declaring exactly what you use.

jsirois added a commit that referenced this issue Mar 25, 2015
The dependency cycles present in the regular fine-grained pants targets
have been fixed so the old parallel mega-targets can go.

Also include a dry-run release in the pants self distribution test.
This DRYs up knowledge of the release target(s) and covers them all
instead of just pantsbuild.pants.

This is one of 3 commits that works towards solving
#1126

Testing Done:
Ran the release.sh locally with flag permutations -n, -t, -pn and -pt.
CI now runs the -pn variation as well.

CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/55836268

Bugs closed: 1126, 1323

Reviewed at https://rbcommons.com/s/twitter/r/1983/
@jsirois jsirois self-assigned this Mar 25, 2015
@jsirois
Copy link
Contributor Author

jsirois commented Apr 1, 2015

All fixes in.

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

No branches or pull requests

1 participant