Whenever using make for this port, there is the warning: make[1]: "/usr/ports/www/chromium/Makefile" line 210: warning: "/usr/bin/grep mempcpy /usr/include/string.h" returned non-zero status There is no mempcpy in string.h (13.0-RELEASE-p4). This warning is unnecessary and possibly misleading. Thank you.
Created attachment 227679 [details] 0001-www-chromium-improve-test-for-mempcpy.patch (In reply to Geezer from comment #0) > There is no mempcpy in string.h (13.0-RELEASE-p4). That's the point, the port needs to know that. There is one in more recent -CURRENT versions and it can't be tested via OSRELEASE. But this test isn't optimal: It's executed every time the Makefile is evaluated *and* make prints a warning when it fails. Find attached a possible patch to solve both issues (at the cost of being hard to read, unfortunately).
Thank you.
Created attachment 227685 [details] 0001-www-chromium-deduce-mempcpy-avail-from-OSVERSION.patch As a possibly better alternative, here's a patch using OSVERSION instead of an explicit check with grep. This might be wrong for -CURRENT built during a short period of time (2 days?), but is readable and doesn't have any of the negative effects.
Yup. A lot simpler.
(In reply to Felix Palmen from comment #3 NEVER DO IT UNTIL CORRESPONDING BUMP ON BASE IS DONE, please! The problematic commit on base doesn't have corresponding bump. This kind of change SHALL make future confusion. And the fix is already proposed on Bug257352 by Tatsuki Makino. [1] (Not an attachment, though.) If the earlier patch here (I've not testet, though) works fine, it is also OK. [1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=257352#c16
(In reply to Tomoaki AOKI from comment #5) > NEVER DO IT UNTIL CORRESPONDING BUMP ON BASE IS DONE, please! > The problematic commit on base doesn't have corresponding bump. > This kind of change SHALL make future confusion. The only reason I could see *not* to resort to OSVERSION would be if the patch would actually break something when strpcpy() is present in base. Even then, I wonder whether it's worthwhile to have such an ugly hack in place just for a period of 2 days when base had a commit without an OSVERSION bump… but then, it won't be me to decide, I suggested two approaches for a reason. > And the fix is already proposed on Bug257352 by Tatsuki Makino. This fix over there has the drawback that the grep is still executed every time the Makefile is evaluated (instead of only on expansion of EXTRA_PATCHES). Unfortunately, I think there's no really readable solution to fix this, you'd have to live with what I suggested above.
Created attachment 227730 [details] 0001-www-chromium-improve-test-for-mempcpy.patch (v2) Because of the readability issue, I updated my suggestion w/o OSVERSION to include a comment explaining the "what" and "why that way".
(In reply to Tomoaki AOKI from comment #5) I'm not sure what you are missing. We have checked when this change was done in base and the first OSVERSION bump after this change was two days later and is the one found in the patch. So only if someone built an OS in the two days between the commit and the OSVERSION bump the check would be wrong and then in the harmless direction, i.e. not using mempcpy when it is available. What's wrong with doing it that way? It is sufficiently accurate, idiomatic (see §4.4 porter's handbook), fixes the warning, and improves performance. They're not going to bump OSVERSION for every single commit, so I'm not sure what you are looking for.
(In reply to Felix Palmen from comment #7) Thanks! Worked fine for me. Tried `make DISABLE_VULNERABILITIES=yes extract patch` on stable/13 and main having mempcpy() and extra-patch was gracefully ignored. Tried above, but modifying word mempcpy to nonexistent mempcpie made extra-patch applied properly. (To simulate base revs NOT having mempcpy().) *Not actually built as it requires a plenty of time.
(In reply to Robert Clausecker from comment #8) Why I'm hesitating to use regular OSVERSION way here is because no commit for version bump mentions about mempcpy() addition. This can make confusion tracking changes in the future. Single bump for multiple commits is NOT AT ALL a problem, if the commit message lists all targetted commits, regardless they're related each other or not. If I'm not missing something, no bump mentions the commits adding mempcpy(). And for this case, checking for mempcpy() instead of checking for OSVERSION has an advantage that no more update is needed when mempcpy() is added to branches that doesn't have it currently. www/chromium is a huge port and changes without actual update would be better minimized, I think. Anyway, Felix's latest patch looks fine enough for me. Thanks again, Felix!
(In reply to Tomoaki AOKI from comment #10) Well, thanks for the explanation, so I now get you just worry about sane traceability. This, of course, makes sense! I still think using grep is an ugly hack (and I hope I found the least troublesome version of it now, hehe) and relying on OSVERSION would be much better. Maybe this would warrant a PR against base to "fix" this with the next commit that bumps OSVERSION?
(In reply to Felix Palmen from comment #11) Exactly. ;-) But one thing to mention. If swiching to OSVERSION way, and the addition of mempcpy() is planned to be MFC'ed to stable/12 and stable/11, this port should be required to updated again, while (ugly) grep hack doesn't need it. I wonder which way would be better for this huge port.
(In reply to Tomoaki AOKI from comment #12) > If swiching to OSVERSION way, and the addition of mempcpy() is planned to be > MFC'ed to stable/12 and stable/11, this port should be required to updated > again, while (ugly) grep hack doesn't need it. I see the point; in fact, if this change already hit stable/13, my OSVERSION based patch is already wrong :( -- but… > I wonder which way would be better for this huge port. I still think it's the better approach cause calling external tools should be avoided. These things could pile up and kill any make performance. At the very least, make sure to do these calls only when really required to expand the variable in question (which is what my patch does). Of course, just an opinion so far. IMHO, go with the "fixed" grep for now, but revisit this once OSVERSION bumps refer the change in question.
Much as I appreciate all the help and support on this PR, in many ways the nature of the patch is irrelevant. If when mempcpy is available, then there is no warning, and when mempcpy is not available, the warning is suppressed, then logically that warning is absolutely redundant. Should the warning merely be removed? Thank you.
(In reply to Geezer from comment #14) This is not about a warning, but about inclusion/exclusion of an extra patch, depending on the availability of mempcpy(). You can't remove that, it's required for correct operation. The warning was an unfortunate by-product. I'm now just waiting for someone to commit e.g. my suggested fix (silence the warning *and* make sure the test only executes when really needed) in the (v2) patch…
(In reply to Felix Palmen from comment #13) Looks rasonable to me. Thanks! BTW, stable/13 already has mempcpy() MFC'ed, while stable/12 and 11 have not. So this would be better revisited when MFC'ed to stable/12 AND stable/11, or made clear that not at all further MFC is planned.
(In reply to Tomoaki AOKI from comment #16) 11.4 is the final release from stable/11, so I don't see any more MFCs there ;) But apart from that, agreed.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=289869de6aa59a3089a60054f10ed97d4f6b491b commit 289869de6aa59a3089a60054f10ed97d4f6b491b Author: Felix Palmen <felix@palmen-it.de> AuthorDate: 2021-09-23 19:32:24 +0000 Commit: Joseph Mingrone <jrm@FreeBSD.org> CommitDate: 2021-09-24 02:46:13 +0000 www/chromium: Tweak mempcpy check to silence a warning PR: 258271 www/chromium/Makefile | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Committed the v2 patch. Thanks everyone. Please reopen or submit a new bug if there are still problems.
Created attachment 231957 [details] Resurrect nasm fix for www/chromium 98.0.4758.102 Unfortunately, www/chromium 98.0.4758.102 dropped this [1]. So let's resurrect it! Now build is ongoing, but as it should consume a plenty of time, upload now. Sorry if any breakage happenes. [1] https://lists.freebsd.org/archives/dev-commits-ports-main/2022-February/014726.html
Created attachment 231961 [details] toggle mempcpy support based on __FreeBSD_version in the header The whole point of merging the patches between OpenBSD and FreeBSD is to avoid such patches and differences. The proper way to do this is to patch the nasm configuration header unconditionally, so that it can be used in the shared code. Please see the attached patch.
*** Bug 262067 has been marked as a duplicate of this bug. ***
(In reply to Robert Nagy from comment #21) Not tested as build with my patch is still ongoing, but your patch would not fix for stable/13. And unfortunately, commits (on main and stable/13) emitting this problem doesn't have corresponding (mentioning their commit hash and reason) version bump is NOT YET DONE, unfortunately. This is the reason I objected to use OSVERSION on Makefile at Comment 5 and expaining why on Comment 10. But if src commits below are noted on commit message for your fix, not-so-good but acceptable with sane traceability. main: git: ee37f64cf875 (ee37f64cf875255338f917a9da76c643cf59786c) stable/13: git: dba677d13b26 (dba677d13b26ad5422133b2ab76486b74d63ade4) The nearest bump on stable/13 would be 1300513. So 1300513 <= __FreeBSD_version < 1400000 needs toggling as > 1400026, too. One more note: these are not yet (no plan?) MFC'ed to stable/12. So once it's MFC'ed to stable/12, one more update is needed with this approach.
Created attachment 231983 [details] patch-third__party_nasm_BUILD.gn Okay, makes sense. Then let's move this whole thing to the gn build and that will be picked up by everyone automatically. Patch attached.
Re-open for now, this patch got lost in https://github.com/freebsd/freebsd-ports/commit/a23dfd214ae04e8b4d116ca6411570b684eb5ed6
(In reply to Robert Nagy from comment #24) Thanks. Now trying build putting your latest patch in files/ and passed the point that usually breaks (4%-5%). Now already around 29%. Just FYI for the future: Your previous patch (modified conditional for stable/13) didn't work because of locked-out problem. By grep'ping, *config/config-linux.h is included only in third_party/nasm/config/config.h *config/config.h is only included in third_party/nasm/include/compiler.h and *Base sys/param.h is included only in third_party/nasm/nasmlib/realpath.c *In realpath.c, compiler.h is included BEFORE base sys/param.h, which is guarded around HAVE_SYS_PARAM_H check. *HAVE_SYS_PARAM_H is toggled in config/config-linux.h. thus __FreeBSD_version is NOT YET DEFINED when evaluated in config/config-linux.h. Forcibly moving HAVE_SYS_PARAM_H part to the top of the file and include sys/param.h just after there with HAVE_SYS_PARAM_H guard allowed build.
(In reply to Tomoaki AOKI from comment #26) OK. The above-mentioned build with latest patch placed in www/chromium/files completed successfully on stable/13, amd64 git c39ff2415cb9. Thanks! BTW, just an idea though, removing relevant part of the patch to config-linux.h (defined HAVE_MEMPCPY before patch applied) and invert the logic make sense, especially for OpenBSD? If mem* functions are defined in sys/string.h on OpenBSD like in FreeBSD and OpenBSD currently doesn't have mempcpy() at all, future defiition corresponding this would be in sys/string.h, too, I think. If it confuses OpenBSD users, I'm OK for current patch.
(In reply to Tomoaki AOKI from comment #27) Oops! Not sys/string.h but string.h.
The patch from Robert also fixes the build on -CURRENT.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=52ea554a266bc98d64f10c5d296286c2ba8b87ea commit 52ea554a266bc98d64f10c5d296286c2ba8b87ea Author: Robert Nagy <robert@openbsd.org> AuthorDate: 2022-02-22 14:52:32 +0000 Commit: Rene Ladan <rene@FreeBSD.org> CommitDate: 2022-02-22 14:56:13 +0000 www/chromium: fix build on 14-CURRENT PR: 258271 Fixes: a23dfd21 "www/chromium: update to 98.0.4758.102" .../files/patch-third__party_nasm_BUILD.gn (new) | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
Build-testing on 13.0-RELEASE/quarterly too to be sure.
A commit in branch 2022Q1 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=e5e56b8c2efb9129760f297bbd4fa9a7b354d726 commit e5e56b8c2efb9129760f297bbd4fa9a7b354d726 Author: Robert Nagy <robert@openbsd.org> AuthorDate: 2022-02-22 14:52:32 +0000 Commit: Rene Ladan <rene@FreeBSD.org> CommitDate: 2022-02-22 15:03:50 +0000 www/chromium: fix build on 14-CURRENT PR: 258271 Fixes: a23dfd21 "www/chromium: update to 98.0.4758.102" (cherry picked from commit 52ea554a266bc98d64f10c5d296286c2ba8b87ea) .../files/patch-third__party_nasm_BUILD.gn (new) | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)