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.

Sign up for GitHub

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

Jump to bottom

Add ability to control path inside PEX file #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add ability to control path inside PEX file #42

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions pex/pex_rules.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def _pex_library_impl(ctx):
)


def _gen_manifest(py, runfiles):
def _gen_manifest(py, runfiles, strip_pex_path_prefixes):
"""Generate a manifest for pex_wrapper.

Returns:
Expand All @@ -149,6 +149,12 @@ def _gen_manifest(py, runfiles):
dpath = f.short_path
if dpath.startswith("../"):
dpath = dpath[3:]

for prefix in strip_pex_path_prefixes:
if dpath.startswith(prefix):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have the same bug here that I had: a path can match two prefixes.

Consider strip_prefixes set to: ['aaa', 'bbb']

If the files are the following:

  • aaa/bbb/foo (expect to strip aaa and produce bbb/foo)
  • bbb/aaa/bar (expect to strip bbb and produce aaa/bar)

However, depending on the order the prefixes are applied, you will probably get one of foo or bar as an output, since it strips both.

To fix it, in my version I strip the LONGEST prefix match, which is a bit annoying but makes in unambiguous, and works when there are "overlapping" strip prefixes (e.g. strip root1 and root1/foo/bar/subroot)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsure if it's less unambiguous then stripping by the first matched prefix which is what this patch does.

Copy link
Contributor

@evanj evanj Jan 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah crap I missed the break on the next line. Silly me, sorry.

Which one will be first though? It will depend on the order deps are specified in BUILD files. I guess that could actually be useful in some cases.

At any rate: Not matching more than one prefix is really the important part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not deps but first in the strip_pex_path_prefixes list, the stripping afterall apply to all files in runfiles which can be either deps, srcs or even data attribute, if I recall well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I see: This adds the attribute to pex_binary. I see! I added it to pex_library since we wanted to have some pex_library make the python package whatever, no matter where it lives in the Bazel workspace.

Anyway: Great minds think alike! I agree that something like this should end up as part of these rules.

Copy link

Choose a reason for hiding this comment

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

What would be the benefit of adding this to pex_library versus pex_binary?

Copy link
Owner

Choose a reason for hiding this comment

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

It might be useful to have it apply to data from pex_library rules; otherwise you would have to manually propagate that information along to the final pex_binary in some situations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... Now that you mention it: I'm pretty sure my version of this does not actually consider the data attribute, and only applies it to the srcs attribute. I think the internal code where I'm using this doesn't use any data files.

Copy link

Choose a reason for hiding this comment

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

So I've been playing with this and I definitely think that applying it to the pex_library (both srcs and data) makes the most sense as it removes the need for duplicating code. In my case I have two binaries that make use of the same library and I'd have to strip the same prefix in both binaries. In Buck this is called base_module maybe we can adopt the same name (see: https://buckbuild.com/rule/python_library.html#base_module).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A counter-argument will be that you wouldn’t include test data in the
pex_library? B/c if I understood correctly having this attribute in
pex_library only requires that you will need to define anything that
eligible to striping in the pex_library rule, correct?

On the other hand, having it in pex_binary and pex_test have direct effect
on the resulting Pex since this later are the rules that create them, and
yes, I agree that interface wise it's not DRY (and personally I don't like it too :)) however IMHO it gives users more control.

dpath = dpath[len(prefix):]
break

pex_files.append(
struct(
src = f.path,
Expand All @@ -165,6 +171,7 @@ def _gen_manifest(py, runfiles):

def _pex_binary_impl(ctx):
transitive_files = set(ctx.files.srcs)
strip_pex_path_prefixes = ctx.attr.strip_pex_path_prefixes

if ctx.attr.entrypoint and ctx.file.main:
fail("Please specify either entrypoint or main, not both.")
Expand All @@ -177,7 +184,13 @@ def _pex_binary_impl(ctx):
main_file = pex_file_types.filter(ctx.files.srcs)[0]
if main_file:
# Translate main_file's short path into a python module name
main_pkg = main_file.short_path.replace('/', '.')[:-3]
main_file_path = main_file.short_path
for prefix in strip_pex_path_prefixes:
if main_file_path.startswith(prefix):
main_file_path = main_file_path[len(prefix):]
break

main_pkg = main_file_path.replace('/', '.')[:-3]
transitive_files += [main_file]

deploy_pex = ctx.new_file(
Expand All @@ -196,7 +209,7 @@ def _pex_binary_impl(ctx):
manifest_file = ctx.new_file(
ctx.configuration.bin_dir, deploy_pex, '_manifest')

manifest = _gen_manifest(py, runfiles)
manifest = _gen_manifest(py, runfiles, strip_pex_path_prefixes)

ctx.file_action(
output = manifest_file,
Expand Down Expand Up @@ -328,7 +341,6 @@ pex_attrs = {
allow_files = repo_file_types),
"data": attr.label_list(allow_files = True,
cfg = "data"),

# Used by pex_binary and pex_*test, not pex_library:
"_pexbuilder": attr.label(
default = Label("//pex:pex_wrapper"),
Expand Down Expand Up @@ -357,6 +369,7 @@ pex_bin_attrs = _dmerge(pex_attrs, {
default = True,
mandatory = False,
),
"strip_pex_path_prefixes": attr.string_list(),
})

pex_library = rule(
Expand Down