Add attribute strip_prefix to pkg_zip

[davschne]

Resolves #170

I've tried to replicate the behavior of the eponymous attribute in pkg_tar, though I find this behavior counterintuitive in some cases. Specifically, see the test packages:

• //tests:test-tar-strip_prefix-empty
• //tests:test-tar-strip_prefix-none

My intuition is that the packages built from these targets should contain:

/
/etc/
/etc/nsswitch.conf

In other words, the default behavior (when strip_prefix isn't provided or is empty) should be to create paths in the archive relative to the package of the target.

Instead they contain only:

/nsswitch.conf

In other words, subdirectories are ignored.

So to clarify, this PR resolves #170 only when the value of strip_prefix isn't falsy in Starlark.

[aiuto]

It's going to be a few days before I can look at this - I'm behind on lots of things because of the power outages in the NYC area last week. My first thought is - can we avoid this and build on the pkg_filegroup work that @nacl is exploring?

…

On Mon, Aug 10, 2020 at 7:12 PM David Schneider ***@***.***> wrote: @davschne <https://github.com/davschne> requested your review on: #221 <#221> Add attribute strip_prefix to pkg_zip as a code owner. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHAUSXCVZIZQPQYCMN3SAB5ETANCNFSM4P2N6GQQ> .

[davschne]

No worries, @aiuto. I'm not yet familiar with the pkg_filegroup proposal, but I'll look into it and provide feedback. You can hold off reviewing this PR until then.

Subsequent to posting this PR, I noticed this one that adds several attributes to pkg_zip, including strip_prefix. If we decide to go ahead with the change, I'll see if I can consolidate my work with that work.

[davschne]

@aiuto, while I'm not sure I fully understand #212 or the motivations for it, my understanding is that it would introduce breaking changes to the APIs of pkg_zip, pkg_tar, and pkg_deb - that it would represent a new pattern for how to form archives. Is that correct? It may indeed be a good direction to go in the long term, say for the next major version of this library. My team needs increased parity between pkg_zip and pkg_tar in the near-term, though, and it would be nice if we didn't have to adapt all our existing packaging code to a new API in order to achieve it. I'd argue for merging this PR, or better yet #127, in the near future, since they're aimed at filling in some gaps in the current API, and not block their progress while a new API is being considered.

[aiuto]

Yes. The new idea is a breaking change. But your point about next major version is well taken.
We can keep this for now, but I am not going to invest my own cycles in remapping and prefix striping on a per rule basis.
We can easily branch this to support 1.x and 2.x development. In the 2.x tree we'll have a more principled (and understandable) way of composing packages with parity across all rules.

[davschne]

Sounds good, @aiuto. Thanks for the merge. I believe #127 will fill in some additional gaps in pkg_zip’s current API with respect to the other packaging macros. I doubt you’d need to invest your own time in ironing out the discrepancies - others are eager to work on it! :)

[cplee]

looks like this landed on master instead of main. Also, will you be planning a release of this anytime soon or if not, how would I use an unreleased version of these rules?

[nacl]

looks like this landed on master instead of main.

Bah. Thanks for the catch.

I opened a PR (#229) to merge master into main to rectify this, but I'm not sure of the best way to get that past the CLA bot. @aiuto?

Also, will you be planning a release of this anytime soon or if not, how would I use an unreleased version of these rules?

I can't really speak to releases, but if you want to use the rules at HEAD, you'd need to do something like what's described here: bazelbuild/bazel#10062 (comment)

[aiuto]

I think the CLA bot failure is because you are forwarding (for lack of the right term) a commit from davschne ( 15fb7a0 ) and that is confusing the bot w.r.t. authorship. @davschne Would you like to try the equivalent? Or do you mind me just copying out the change and re-applying to main?

…

On Fri, Sep 4, 2020 at 4:12 PM Andrew Psaltis ***@***.***> wrote: looks like this landed on master instead of main. Bah. Thanks for the catch. I opened a PR (#229 <#229>) to merge master into main to rectify this, but I'm not sure of the best way to get that past the CLA bot. @aiuto <https://github.com/aiuto>? Also, will you be planning a release of this anytime soon or if not, how would I use an unreleased version of these rules? I can't really speak to releases, but if you want to use the rules at HEAD, you'd need to do something like what's described here: bazelbuild/bazel#10062 (comment) <bazelbuild/bazel#10062 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHCUBHOZA4LE3T3YOKDSEFCZBANCNFSM4P2N6GQQ> .

[davschne]

Sure, I'll do it.