bazel-central-registry icon indicating copy to clipboard operation
bazel-central-registry copied to clipboard

add libarchive 3.7.3 with tests and windows support

Open zaucy opened this issue 9 months ago • 5 comments

Based on https://github.com/zaucy/libarchive/tree/bazel-v3.7.3

  • adds tests
  • adds Windows support

zaucy avatar Apr 30 '24 18:04 zaucy

Hello @dzbarsky, modules you maintain (libarchive) have been updated in this PR. Please review the changes.

bazel-io avatar Apr 30 '24 18:04 bazel-io

@dzbarsky I would love your opinion on these changes since you were the original maintainer. If this works out I intend to subumit these changes for 3.7.2 as well.

zaucy avatar Apr 30 '24 18:04 zaucy

some of the tests take an especially long time on the GitHub free action runners, but are quite fast locally. If they run slowly on the BCR buildkite CI then I may disable them or exclude them in the presubmit.

zaucy avatar Apr 30 '24 18:04 zaucy

Thanks for submitting! I'm about to go on vacation for a week, I will do my best to find time to look before that

DavidZbarsky-at avatar May 01 '24 23:05 DavidZbarsky-at

The presubmit is mostly green after I've excluded some tests (with added notes for any future readers.)

However, there is a patch error only on ubuntu arm64 for some reason. If anyone has any insight on that, it would be very helpful.

zaucy avatar May 02 '24 16:05 zaucy

This fix is required to allow rules_js to work on windows. Can it be moved forward?

peakschris avatar May 17 '24 10:05 peakschris

Since v3.7.3 is already in the registry, is this PR still relevant? Happy to merge once the module maintainers tell me to.

fmeum avatar May 17 '24 10:05 fmeum

@dzbarsky we do still need this, right?

alexeagle avatar May 17 '24 12:05 alexeagle

We still need this PR and/or a 3.7.4 with these changes applied on top.

From my POV, it's ready to go in, but @zaucy can make the final call on whether to change the config.h setup or keep as is.

There's also some issue with the patch not applying cleanly on one platform that we haven't gotten to the bottom of https://github.com/bazelbuild/bazel-central-registry/pull/1916#discussion_r1594523428

It would be nice to fix this before landing, but I think we need some help.

@zaucy anything else I'm missing?

DavidZbarsky-at avatar May 17 '24 15:05 DavidZbarsky-at

yes currently https://github.com/bazelbuild/bazel-central-registry/pull/1916#discussion_r1594523428 is making me hesitant to say it's done. After that's resolved I'm happy to update this PR with 3.7.4.bcr.1 added as well.

zaucy avatar May 17 '24 15:05 zaucy

Hi all, are there any updates on this? We are blocked from using rules_js on windows at present.

peakschris avatar Jun 06 '24 08:06 peakschris

Hello @dzbarsky, modules you maintain (libarchive) have been updated in this PR. Please review the changes.

bazel-io avatar Jun 06 '24 17:06 bazel-io

Unfortunately the patches aren't applying for arm64 in the presubmit and I'm not sure why. If anyone has any insight it would be much appreciated. The patch was simply generated with git diff - here's the full script: https://github.com/zaucy/libarchive/blob/bazel-v3.7.4/bazel/update_bcr.nu#L39

https://buildkite.com/bazel/bcr-presubmit/builds/6046#018feebc-e114-416d-ab7c-66064716e347

Applying build_with_bazel.patch:
--
  | The next patch would create the file .bazelrc,
  | which already exists!  Applying it anyway.
  | patching file .bazelrc
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file .bazelrc.rej
  | The next patch would create the file BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file BUILD.bazel.rej
  | The next patch would create the file MODULE.bazel,
  | which already exists!  Applying it anyway.
  | patching file MODULE.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file MODULE.bazel.rej
  | The next patch would create the file WORKSPACE.bzlmod,
  | which already exists!  Applying it anyway.
  | patching file WORKSPACE.bzlmod
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file WORKSPACE.bzlmod.rej
  | The next patch would create the file bazel/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file bazel/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file bazel/BUILD.bazel.rej
  | The next patch would create the file bazel/config.bzl,
  | which already exists!  Applying it anyway.
  | patching file bazel/config.bzl
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file bazel/config.bzl.rej
  | The next patch would create the file bazel/libarchive_test.bzl,
  | which already exists!  Applying it anyway.
  | patching file bazel/libarchive_test.bzl
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file bazel/libarchive_test.bzl.rej
  | The next patch would create the file cat/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file cat/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file cat/BUILD.bazel.rej
  | The next patch would create the file cpio/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file cpio/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file cpio/BUILD.bazel.rej
  | The next patch would create the file libarchive/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file libarchive/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file libarchive/BUILD.bazel.rej
  | The next patch would create the file libarchive/test/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file libarchive/test/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file libarchive/test/BUILD.bazel.rej
  | The next patch would create the file libarchive_bazel_generic_config.h,
  | which already exists!  Applying it anyway.
  | patching file libarchive_bazel_generic_config.h
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file libarchive_bazel_generic_config.h.rej
  | The next patch would create the file libarchive_bazel_windows_config.h,
  | which already exists!  Applying it anyway.
  | patching file libarchive_bazel_windows_config.h
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file libarchive_bazel_windows_config.h.rej
  | The next patch would create the file libarchive_fe/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file libarchive_fe/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file libarchive_fe/BUILD.bazel.rej
  | The next patch would create the file tar/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file tar/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file tar/BUILD.bazel.rej
  | patching file tar/bsdtar.c
  | The next patch would create the file test_utils/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file test_utils/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file test_utils/BUILD.bazel.rej
  | patching file test_utils/test_common.h
  | The next patch would create the file unzip/BUILD.bazel,
  | which already exists!  Applying it anyway.
  | patching file unzip/BUILD.bazel
  | Hunk #1 FAILED at 1.
  | 1 out of 1 hunk FAILED -- saving rejects to file unzip/BUILD.bazel.rej
  | Traceback (most recent call last):
  | File "bcr_presubmit.py", line 513, in <module>
  | sys.exit(main())
  | File "bcr_presubmit.py", line 504, in main
  | repo_location, config_file = prepare_test_module_repo(args.module_name, args.module_version)
  | File "bcr_presubmit.py", line 245, in prepare_test_module_repo
  | apply_patch(source_root, source["patch_strip"], patch_file)
  | File "bcr_presubmit.py", line 202, in apply_patch
  | subprocess.run(
  | File "/usr/lib/python3.8/subprocess.py", line 516, in run
  | raise CalledProcessError(retcode, process.args,
  | subprocess.CalledProcessError: Command '['patch', '-f', '-p1', '-i', PosixPath('/workdir/modules/libarchive/3.7.4.bcr.1/patches/build_with_bazel.patch')]' returned non-zero exit status 1.

zaucy avatar Jun 06 '24 18:06 zaucy

@meteorcloudy can we get some sort of linux arm platform that is supported? A lot of us are deploying software to Graviton these days for cost efficiency reasons and it's unfortunate to not have it tested upstream; especially for C code it can have a lot of architecture-specific bits

DavidZbarsky-at avatar Jun 07 '24 16:06 DavidZbarsky-at

@fweikert What is the latest status of https://github.com/bazelbuild/continuous-integration/pull/1493? Is it possible to have Linux arm64 machines on our GCP project?

meteorcloudy avatar Jun 07 '24 17:06 meteorcloudy

Yeah, it's possible, I just didn't have time to fix all the weird breakages.

fweikert avatar Jun 07 '24 18:06 fweikert

@dzbarsky @meteorcloudy could this go in soon? Without it we are blocked in using rules_js on windows. Sorry for the nag and thanks in advance!

peakschris avatar Jun 14 '24 23:06 peakschris

@DavidZbarsky-at the prebuilt release for this failed:

 In file included from external/libarchive~/libarchive/archive_hmac.c:32:
external/libarchive~/libarchive/archive_hmac_private.h:51:10: fatal error: 'CommonCrypto/CommonHMAC.h' file not found
   51 | #include <CommonCrypto/CommonHMAC.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

https://github.com/aspect-build/bsdtar-prebuilt/actions/runs/9529065254

Is it non-hermetic and some crypto system library has to be installed on the runner?

alexeagle avatar Jun 15 '24 15:06 alexeagle

@DavidZbarsky-at the prebuilt release for this failed:

 In file included from external/libarchive~/libarchive/archive_hmac.c:32:
external/libarchive~/libarchive/archive_hmac_private.h:51:10: fatal error: 'CommonCrypto/CommonHMAC.h' file not found
   51 | #include <CommonCrypto/CommonHMAC.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

https://github.com/aspect-build/bsdtar-prebuilt/actions/runs/9529065254

Is it non-hermetic and some crypto system library has to be installed on the runner?

The CommonCrypto include was patched out in my original version and I guess the patch was not carried forward. I'm traveling for the next few weeks, @zaucy do you have a chance to fix it up?

DavidZbarsky-at avatar Jun 15 '24 15:06 DavidZbarsky-at

I'll take a look.

zaucy avatar Jun 15 '24 15:06 zaucy

Ah I see. I didn't notice the source patch. This is the one you're talking about right?

--- a/libarchive/archive_hmac_private.h
+++ b/libarchive/archive_hmac_private.h
@@ -40,12 +40,13 @@
  */
 int __libarchive_hmac_build_hack(void);
 
-#ifdef __APPLE__
-# include <AvailabilityMacros.h>
-# if MAC_OS_X_VERSION_MAX_ALLOWED >= 1060
-#  define ARCHIVE_HMAC_USE_Apple_CommonCrypto
-# endif
-#endif
+// Don't compile against CommonCrypto.
+// #ifdef __APPLE__
+// # include <AvailabilityMacros.h>
+// # if MAC_OS_X_VERSION_MAX_ALLOWED >= 1060
+// #  define ARCHIVE_HMAC_USE_Apple_CommonCrypto
+// # endif
+// #endif

I understand wanting to make bsdtar hermetic because its being used as a tool, but shouldn't we be avoiding modifying sources when submitting modules to the BCR? This feels like a patch that should be either done in a patch in the repo that wants it to be hermetic (e.g. https://github.com/aspect-build/bsdtar-prebuilt) and/or submitted as a feature request upstream to libarchive.

Regardless, I'll be fixing it since it was in the patch prior to this one.

zaucy avatar Jun 15 '24 15:06 zaucy

Ah I see. I didn't notice the source patch. This is the one you're talking about right?

--- a/libarchive/archive_hmac_private.h
+++ b/libarchive/archive_hmac_private.h
@@ -40,12 +40,13 @@
  */
 int __libarchive_hmac_build_hack(void);
 
-#ifdef __APPLE__
-# include <AvailabilityMacros.h>
-# if MAC_OS_X_VERSION_MAX_ALLOWED >= 1060
-#  define ARCHIVE_HMAC_USE_Apple_CommonCrypto
-# endif
-#endif
+// Don't compile against CommonCrypto.
+// #ifdef __APPLE__
+// # include <AvailabilityMacros.h>
+// # if MAC_OS_X_VERSION_MAX_ALLOWED >= 1060
+// #  define ARCHIVE_HMAC_USE_Apple_CommonCrypto
+// # endif
+// #endif

I understand wanting to make bsdtar hermetic because its being used as a tool, but shouldn't we be avoiding modifying sources when submitting modules to the BCR? This feels like a patch that should be either done in a patch in the repo that wants it to be hermetic (e.g. https://github.com/aspect-build/bsdtar-prebuilt) and/or submitted as a feature request upstream to libarchive.

Regardless, I'll be fixing it since it was in the patch prior to this one.

Yep, that's the one. I don't have a super strong philosophical opinion on where the patch should live but compiling against and OSX SDK is a common pain point so I suspect others may want this patch as well.

I suppose if we're very motivated we could patch it to wrap this in another #define and allow configuring it with a build flag? Or add the feature upstream as you suggest

dzbarsky avatar Jun 15 '24 16:06 dzbarsky

Some kind of #define in the upstream that we could configure would probably be the best call imo!

anyways here's the fix: https://github.com/bazelbuild/bazel-central-registry/pull/2271

zaucy avatar Jun 15 '24 16:06 zaucy

hey @zaucy - any idea why we get

Action failed to execute: java.io.IOException: ERROR: src/main/native/windows/process.cc(202): CreateProcessW("C:\users\runneradmin\_bazel_runneradmin\snmkx6sz\execroot\_main\external\aspect_bazel_lib~~toolchains~bsd_tar_windows_amd64\tar.exe" --create --file bazel-out/x64_windows-fastbuild/bin/tar_empty.tar @bazel-out/x64_windows-fastbuild/bin/_tar_empty.mtree.txt): This version of %1 is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher.

when running this binary on windows? https://github.com/bazel-contrib/bazel-lib/actions/runs/10913564506/job/30290275800

alexeagle avatar Sep 18 '24 02:09 alexeagle

Oh it's an ELF-format binary.

alexeagle avatar Sep 18 '24 13:09 alexeagle