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.

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

add libarchive 3.7.4.bcr.1 with tests and windows support #1916

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

zaucy
Copy link
Contributor

@zaucy zaucy commented Apr 30, 2024

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

  • adds tests
  • adds Windows support

@zaucy zaucy force-pushed the add-libarchive-3.7.3-and-tests branch from 5459986 to df6dc6b Compare April 30, 2024 18:18
@bazel-io
Copy link
Member

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

@zaucy
Copy link
Contributor Author

zaucy commented Apr 30, 2024

@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 zaucy changed the title add libarchive 3.7.3 with tests add libarchive 3.7.3 with tests and windows support Apr 30, 2024
@zaucy zaucy marked this pull request as ready for review April 30, 2024 18:32
@zaucy
Copy link
Contributor Author

zaucy commented Apr 30, 2024

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 zaucy force-pushed the add-libarchive-3.7.3-and-tests branch from 552e852 to 9c8711b Compare May 1, 2024 03:49
@alexeagle alexeagle added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label May 1, 2024
@DavidZbarsky-at
Copy link

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

@zaucy
Copy link
Contributor Author

zaucy commented May 2, 2024

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 zaucy mentioned this pull request May 4, 2024
@zaucy zaucy force-pushed the add-libarchive-3.7.3-and-tests branch from 593e84d to 9530ab5 Compare May 8, 2024 18:32
dzbarsky
dzbarsky previously approved these changes May 8, 2024
Copy link
Contributor

@dzbarsky dzbarsky left a comment

Choose a reason for hiding this comment

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

I think this can go in as-is if you'd like, but I'd prefer to generate the config as config.h and reduce the patching, and a 3.7.4 base would be great as well!

Thanks for helping on this one, much appreciated

modules/libarchive/3.7.3/MODULE.bazel Outdated Show resolved Hide resolved
modules/libarchive/3.7.3/presubmit.yml Outdated Show resolved Hide resolved
modules/libarchive/3.7.3/patches/build_with_bazel.patch Outdated Show resolved Hide resolved
bazel-io
bazel-io previously approved these changes May 8, 2024
Copy link
Member

@bazel-io bazel-io left a comment

Choose a reason for hiding this comment

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

Hello @bazelbuild/bcr-maintainers, all modules in this PR have been approved by their maintainers. Please take a final look to merge this PR.

Copy link
Member

@bazel-io bazel-io left a comment

Choose a reason for hiding this comment

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

Require module maintainers' approval.

@peakschris
Copy link

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

@fmeum
Copy link
Contributor

fmeum commented May 17, 2024

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

@alexeagle
Copy link
Contributor

@dzbarsky we do still need this, right?

@DavidZbarsky-at
Copy link

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 #1916 (comment)

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

@zaucy anything else I'm missing?

@zaucy
Copy link
Contributor Author

zaucy commented May 17, 2024

yes currently #1916 (comment) 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.

@peakschris
Copy link

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

@zaucy zaucy changed the title add libarchive 3.7.3 with tests and windows support add libarchive 3.7.4.bcr.1 with tests and windows support Jun 6, 2024
@bazel-io bazel-io dismissed dzbarsky’s stale review June 6, 2024 17:45

Require module maintainers' approval for newly pushed changes.

@bazel-io
Copy link
Member

bazel-io commented Jun 6, 2024

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

@bazel-io bazel-io dismissed their stale review June 6, 2024 17:45

Require module maintainers' approval for newly pushed changes.

@zaucy
Copy link
Contributor Author

zaucy commented Jun 6, 2024

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.

@DavidZbarsky-at
Copy link

@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

@zaucy zaucy requested a review from bazel-io June 7, 2024 17:02
@meteorcloudy
Copy link
Member

@fweikert What is the latest status of bazelbuild/continuous-integration#1493? Is it possible to have Linux arm64 machines on our GCP project?

@fweikert
Copy link
Member

fweikert commented Jun 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants