Created attachment 236975 [details] security/john: fix build on arm64 This fixes an incorrect PPC patch, making it work by removing a #ifdef linux instead of adding an extra include. The build on AArch64 now succeeds. @pkubaj, please check if the build on PPC still succeeds before this port is committed. Tested with Poudriere on armv7 arm64 amd64 i386 FreeBSD 13.1. @danfe Please don't erase the author field when committing this patch, as you accidentally did in 5c648bf. If you use git-am to apply it, meta data will automatically be preserved.
Tested, builds fine.
(In reply to Robert Clausecker from comment #0) > Please don't erase the author field when committing this patch, as you > accidentally did in 5c648bf. There was no accident, I typically dislike Git's separation of author and comitter and use --author only when applying the patch verbatim, including commit log (which seldom happens). I by far prefer traditional "Submitted by:" attribution, and always preserved the credit when it's due.
(In reply to Alexey Dokuchaev from comment #2) As per Committer's Guide §9.14: > Submitted by: This has been deprecated with git; submitted patches should have the author set by using git commit --author with a full name and valid email. Please try to follow this guide. It makes it much easier to find who authored which patches if metadata is entered into the right places. I am definitely not a fan of having my patch authorship erased either.
Any progress with this patch?
(In reply to Robert Clausecker from comment #4) > Any progress with this patch? I still need to understand how this fix works, why messing with PowerPC's #include <altivec.h> affects AArch64 build, what does upstream think about it, etc.
(In reply to Alexey Dokuchaev from comment #5) Ah, I see. I can explain this for you. The source code of john actually builds just fine on AArch64 (aka arm64) if no patches are applied. However, for PowerPC support, the <altivec.h> include file is required. This include file is present but gated behind #ifdef __linux__ in the original source for some reason. Meaning that on FreeBSD, it is not being included and compilation fails. In commit 1e66703, pkubaj@ had discovered this build failure on PowerPC targets, but patched it incorrectly. Instead of removing the #ifdef __linux__ gating around the <altivec.h> include file, he added another <altivec.h> include into a different place. This fixed the build on PowerPC, but now made the AArch64 build fail as it takes the same path where pkubaj@ added the phony include. This is because AArch64 too has 32 SIMD registers, so #define regs 32 is correct for AArch64 and no extra case is needed. My patch fixes pkubaj@'s patch by removing the phony extra include directive and instead removing the #ifdef __linux__ gating from the correct directive.
(In reply to Robert Clausecker from comment #6) > The source code of john actually builds just fine on AArch64 (aka arm64) > if no patches are applied. Yeah, I know. I've heard from another user that AArch64 build is broken and suggested he removes the offending patch by pkubaj@. > However, for PowerPC support, the <altivec.h> include file is required. You don't say? ;-) > gated behind #ifdef __linux__ in the original source for some reason. Yeah, this is the weird part. I guess it explains why there're no issues about broken AArch64 build filed on their GitHub as it works for GNU/Linux folks which unfortunately outnumber us. Interestingly, similar #include <arm_neon.h> in that file is not guarded. I've left the comment to the commit which introduced it, Alexander is usually quite responsive.
So... is there anything that still speaks against commiting the patch?
(In reply to Robert Clausecker from comment #8) > is there anything that still speaks against committing the patch? Just wanted to give people a chance to comment on that newly opened issue. Also, the patch won't be necessary as simple sed(1) call would suffice in this case.
(In reply to Alexey Dokuchaev from comment #9) > Just wanted to give people a chance to comment on that newly opened issue. Also, the patch won't be necessary as simple sed(1) call would suffice in this case. Please note that as per policy, sed(1) (aka REINPLACE_CMD) may only be used for dynamic replacements. Static replacements must use patch files. See porter's handbook §4.4.3: > Only use sed(1) to replace variable content. You must use patch files instead of sed(1) to replace static content.
(In reply to Robert Clausecker from comment #10) > Please note that as per policy, sed(1) (aka REINPLACE_CMD) may only be used > for dynamic replacements. This is no more than a general guideline. I believe I can judge well enough when a patch or sed(1) call is a better option in each particular case.
(In reply to Alexey Dokuchaev from comment #11) > This is no more than a general guideline. Keep in mind that “general guidelines” are usually not written with “Only use ...” and “You must use ...” But as with ignoring the rules about authorship attribution, it seems like you just do whatever you want anyway. In any way, I'm still not sure what you are waiting for. The port currently has a defective patch applied. We are all in agreement about this (and apparently have been from the very beginning). Yet you are doing nothing about it and the port stays broken. If it is so important for you to keep the port in a state as upstream intended it, why haven't you at least removed the offending, clearly incorrect patch?
(In reply to Robert Clausecker from comment #12) > But as with ignoring the rules about authorship attribution As I've said, I always preserve the due credit. FWIW I was firmly against the portmgr@ decision to remove the "Created by" tags from the ports' Makefiles. > why haven't you at least removed the offending, clearly incorrect patch? Because it would apparently break the PowerPC builds? It should be removed together with #ifdef __linux__ -> #ifndef __APPLE__ change in one commit which I'll do shortly.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=809fe307f02ed333ae4e3ce3325df958e19c3ab1 commit 809fe307f02ed333ae4e3ce3325df958e19c3ab1 Author: Alexey Dokuchaev <danfe@FreeBSD.org> AuthorDate: 2022-10-19 07:48:36 +0000 Commit: Alexey Dokuchaev <danfe@FreeBSD.org> CommitDate: 2022-10-19 07:48:36 +0000 security/john: try to fix the port's build on AArch64 In commit 1e6670382fbb, the observed failure on PowerPC had been mended in a way that broke AArch64 build. Robert's investigation had shown that <altivec.h> should've been included elsewhere, and it actually is, but guarded by `#ifdef __linux__' for some reason. Alexander explained that at the time the AltiVec support was just for two platforms: macOS (first) and Linux (added later). If the guard of `#include <altivec.h>' was needed, then we could probably replace the `#ifdef __linux__' with `#ifndef __APPLE__' for the same effect on old macOS versions which do not need the #include. PR: 266732 Submitted by: Robert Clausecker Discussed with: Solar Designer security/john/Makefile | 2 ++ security/john/files/patch-src_sboxes-s.c (gone) | 10 ---------- 2 files changed, 2 insertions(+), 10 deletions(-)