rules_pkg icon indicating copy to clipboard operation
rules_pkg copied to clipboard

pkg_files and pkg_tar interface are different

Open AustinSchuh opened this issue 2 years ago • 6 comments

We've been using pkg_tar for things we should have been using pkg_files for. In the process of converting, I've been noticing that the 2 have the same concepts, but a different syntax. It would be great to be able to change the rule name and for everything (that is applicable) to just work. Things like package_dir -> prefix, remap_paths -> renames.

AustinSchuh avatar Dec 02 '22 02:12 AustinSchuh

I keep running into differences. include_runfiles only exists on pkg_tar

AustinSchuh avatar Dec 02 '22 07:12 AustinSchuh

@aiuto , I'm happy to work on at least part of this one if there's consensus that this is reasonable to do. The include_runfiles is making it hard to move a pkg_tar depending on a pkg_tar over to using pkg_files.

AustinSchuh avatar Dec 02 '22 07:12 AustinSchuh

Unfortunately, this is rather complicated, and probably isn't feasible without some complex migration tooling (@aiuto may have a differing opinion):

  • package_dir is like prefix, but it also applies to the outputs of packaging providers. package_dir as a concept should probably be eliminated at some point IMO, and users should instead use pkg_filegroup.
  • remap_paths allows you to do arbitrary path transformations. renames is just file renames and not much more. I never felt inclined to do this for pkg_files, given how complex and error-prone it is. It also never made it to pkg_zip.

IIRC include_runfiles is something intended to support some legacy behavior in pkg_tar. The correct handling of runfiles for pkg_files is a rather complicated topic that needs to be designed. That being said, if functionality like the one for pkg_tar were to land in pkg_files, I probably would be OK with it, at least in the immediate term.

You might be able to use buildozer in some cases, but not all. A proper migration path, especially at scale, may require more complicated tooling, perhaps using something like https://github.com/Instagram/LibCST. I wrote something like this to adjust strip_prefix in https://github.com/bazelbuild/rules_pkg/pull/492.

nacl avatar Dec 02 '22 14:12 nacl

We just migrated some internal usages of the @bazel_tools version of rules_pkg and noticed that the behavior of include_runfiles was subject to a breaking change with https://github.com/bazelbuild/rules_pkg/commit/7a991dea418ab17c7e86f0a7b5e7d4a87ef4304b. In its current form, it's not usable with any kind of external dependencies anymore and has been that way since rules_pkg 0.2.5.

The implementation in pkg_files does seem to support a version of include_runfile that looks much more (but not entirely) correct, it just isn't surfaced as an attribute of the pkg_zip rule and not used at all by pkg_tar.

@nacl @aiuto Would you accept patches to either revert include_runfiles to its precise legacy behavior or (preferably) make it work just as it does in the regular Bazel output tree? Happy to describe the design, but I don't think there is any kind of choice: There should be an executable.runfiles directory with the expected structure next to any executable in the DefaultInfo of things we add when include_runfiles is True.

fmeum avatar Feb 02 '23 11:02 fmeum

Sorry I missed this entirely. Yes. I'm happy to accept PRs against this. I have some half finished things in the works. I will try to put them up as WIP PRs you could take a look at.

aiuto avatar Feb 15 '23 17:02 aiuto

I assume this PR is related? https://github.com/bazelbuild/rules_pkg/pull/724

mnil avatar Aug 11 '23 08:08 mnil