Skip to content

add support for no-index, requirements files, and repo paths#40

Merged
benley merged 7 commits intobenley:masterfrom
georgeliaw:requirements-file-support
Aug 8, 2017
Merged

add support for no-index, requirements files, and repo paths#40
benley merged 7 commits intobenley:masterfrom
georgeliaw:requirements-file-support

Conversation

@georgeliaw
Copy link
Contributor

@georgeliaw georgeliaw commented Jul 16, 2017

Needed to update the pex_binary rules to keep the clutter down in the BUILD files and make them easier to maintain. Figured I should open a PR in case you would like to pull this into master.

@georgeliaw georgeliaw force-pushed the requirements-file-support branch 2 times, most recently from c04abc4 to d9a65f0 Compare July 16, 2017 11:41
@georgeliaw georgeliaw force-pushed the requirements-file-support branch from d9a65f0 to 36d7308 Compare July 16, 2017 11:58
Copy link
Owner

@benley benley left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! This looks good, I just have a few questions.

for dep in ctx.attr.deps:
if hasattr(dep.py, "repos"):
repos += dep.py.repos
for file in egg_file_types.filter(ctx.files.repos):
Copy link
Owner

Choose a reason for hiding this comment

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

Would it work to allow source distributions in repos along with eggs and wheels? That could be very convenient at times.

Copy link
Owner

Choose a reason for hiding this comment

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

Also technically one can specify https URLs pointing at a package repository with --repos. It's not a great practice with regard to Bazel, but I know people will want to do that... Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benley added support for source distributions as well as cache disabling.

Have some mixed feelings about supporting URLs for the same reason that it's not exactly great practice. I'll think on the issue a bit this week.

# Use a /tmp path, but keep the actual venv inside the bazel outdir.
# Avoids having to worry about cleanup, even if sandboxing is off.
TMPF=$$(mktemp)
TMPF=$$(mktemp 2>/dev/null || mktemp -t tmp)
Copy link
Owner

Choose a reason for hiding this comment

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

Wow, I wonder how this ever worked on MacOS - did mktemp's behaviour change in a recent macos release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benley yeah this was a weird one, it appears the behavior was fixed to match gnu in OS X 10.11, but older versions of OS X require the -t flag. Only encountered it due to a coworker running into this issue.

https://unix.stackexchange.com/questions/30091/fix-or-alternative-for-mktemp-in-os-x

@georgeliaw
Copy link
Contributor Author

georgeliaw commented Jul 21, 2017

@benley gave the URL --repo some thought. I think there's more complexity to that task than would fit into the scope of this PR. For the time being, users should be able to pull packages in via http_file in their workspace (possibly wrap it up into a filegroup) and load it into either the eggs or repos params. Have not tried that out myself, but this way they would be able to benefit from checksum enforcement.

Anything nicer would probably require writing new workspace rules for like pypi_server to set the url and pypi_package to set package name/checksum. Not sure that work is worth doing at the moment if we can use the workaround above.

For the time being, we can consider the PR ready to merge unless there are any changes you would like me to make.

EDIT: I see we both seem to agree on new workspace rules being the nicer option to implement from looking at #20. We can probably look at that at a later point in time.

@georgeliaw
Copy link
Contributor Author

@benley I just recalled that requirements files themselves can be used to point to other repo urls, so technically this is already supported. Again, obviously not the greatest practice when it comes to Bazel, but the use-case is available.
https://pip.pypa.io/en/stable/reference/pip_install/#requirements-file-format

Pip since version 8.0 also supports hash checking, which we could try enforcing to improve the build determinism:
https://pip.pypa.io/en/stable/reference/pip_install/#hash-checking-mode

This technically means we wouldn't have to go down the workspace rule route.

What are your thoughts on this?

@benley
Copy link
Owner

benley commented Aug 8, 2017

Oh cool, that is a good point. I completely forgot about this PR, sorry about that!

@benley benley merged commit 90bdd67 into benley:master Aug 8, 2017
@georgeliaw georgeliaw deleted the requirements-file-support branch August 8, 2017 23:01
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