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

rustls-ffi: drop clang-asan.patch #234

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

cpu
Copy link
Contributor

@cpu cpu commented Aug 7, 2024

Hi there, I was interested to learn that you're shipping curl with rustls-ffi by default. Very cool 🎉

While looking at the package definition I noticed you were carrying an extra patch that I think can be dropped. I'm not at all familiar with SerpentOS so hopefully I haven't made any silly errors in this contribution.

Previously the rustls-ffi build setup applied a patch to the upstream Makefile to remove unconditional use of ASAN w/ clang. It looks like it was added alongside the original addition of rustls-ffi. This branch removes the patch because it's no longer necessary for two reasons:

  1. The package was already migrated to build using cargo-c and so I believe hasn't been using the patched Makefile since then.
  2. We independently noticed & fixed this bug upstream (the fix will be included in the next release). The intent was to only enable ASAN for debug builds, not release builds (the Makefile default). Sorry about that!

Previously the `rustls-ffi` build setup applied a patch to the upstream
`Makefile` to remove unconditional use of ASAN w/ `clang`. This commit
removes the patch because it's no longer necessary for two reasons:

1. The package was already migrated to build using `cargo-c` and so
   wasn't using the `Makefile` anymore.
2. We independently noticed & fixed this bug upstream (fix to be
   included in the next release). The intent was to only enable ASAN for
   debug builds, not release builds (the `Makefile` default). Sorry
   about that!
@cpu cpu requested a review from a team as a code owner August 7, 2024 16:56
Copy link
Member

@ikeycode ikeycode left a comment

Choose a reason for hiding this comment

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

Hey @cpu thanks for the PR and for rustls-ffi! Anywhere we can displace insecure surface area in Serpent OS with libs like this, we will.

I'm not gonna get picky with the PR because honestly our docs are crap and we're still in "open the doors up" mode, so I'll do the relevant bump and build atop this using boulder (from the moss repo)

Thanks! =)

@ikeycode ikeycode merged commit 7019832 into serpent-os:main Aug 8, 2024
1 check passed
@cpu cpu deleted the cpu-rustls-ffi-no-patch branch August 8, 2024 13:22
@cpu
Copy link
Contributor Author

cpu commented Aug 8, 2024

Thanks! Don't be shy if you run into any issues w/ rustls-ffi in the future. Happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use -fsanitize=address for tests only
2 participants