search

bazelbuild / rules_pkg

Bazel rules for creating packages of many types (zip, tar, deb, rpm, ...)
Apache License 2.0
216 stars 174 forks source link

Merge verify_archive_test_lib with verify_archive_test_main.py.tpl #705

Closed sschwebach closed 1 year ago

sschwebach commented 1 year ago

Fixes #704

One of the deps within the verify_archive_test macro depends on a label, which would be incorrectly evaluated in a caller's BUILD file to be a non-external package.

ORIGINAL CHANGE: This change wraps the label in a Label() call to ensure it's referenced properly in the rules_pkg context.

I added a new external repository to make a test since it didn't quite seem appropriate to put it in the mapping repository, though I can certainly do so if desired.

UPDATED CHANGE: Instead of relying on an additional public py_library target the test class used by the test is simply part of the test template used to create the final executable. While this is somewhat inefficient it removes the need to ensure imports are maintained when this target is used in another workspace.

aiuto commented 1 year ago

I'll right. I'm stumped. I tried with both Label(//pkg:verify_archive_test_lib) and as @rules_pkg//pkg:varify_archive_test_lib. The integration tests fail.
I think your change is right, but I don't understand why the test fails.

aiuto commented 1 year ago

I am now thinking this is needlessly efficient. We could just fold the test lib into the .tpl file and eliminate the library. Then the problem you had would go away.

sschwebach commented 1 year ago

I'll right. I'm stumped. I tried with both Label(//pkg:verify_archive_test_lib) and as @rules_pkg//pkg:varify_archive_test_lib. The integration tests fail. I think your change is right, but I don't understand why the test fails.

I'm equally stumped on this one. From what I can tell there shouldn't be any difference using this from one external workspace compared to another. I'll see if I can find what might be causing the issue before looking at merging the library into the template.

aiuto commented 1 year ago

I'm even more convinced that merging into the template is the best solution. It means that if you import rules_pkg into your own third_party, you don't have to rebase the import of the library. In fact, my comment in the template "Hey reviewer!!! What if we just added the source to the test lib here, so we would not have to make the library for that public?" hints that I considered that in the first implementation.

sschwebach commented 1 year ago

I've pushed a change to merge the two together, along with an additional change to remove the json and runfiles imports that don't seem to be used. I also edited the title/description since it's a bit different than the original change.