Bug 258271 - www/chromium: Makefile warning for grep mempcpy /usr/include/string.h. This warning is redundant and persistent.
Summary: www/chromium: Makefile warning for grep mempcpy /usr/include/string.h. This w...
Status: New
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: amd64 Any
: --- Affects Some People
Assignee: freebsd-chromium (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-05 03:42 UTC by Geezer
Modified: 2021-09-13 14:54 UTC (History)
4 users (show)

See Also:
bugzilla: maintainer-feedback? (chromium)


Attachments
0001-www-chromium-improve-test-for-mempcpy.patch (1.28 KB, patch)
2021-09-05 11:22 UTC, Felix Palmen
no flags Details | Diff
0001-www-chromium-deduce-mempcpy-avail-from-OSVERSION.patch (934 bytes, patch)
2021-09-05 14:26 UTC, Felix Palmen
no flags Details | Diff
0001-www-chromium-improve-test-for-mempcpy.patch (v2) (1.45 KB, patch)
2021-09-07 06:39 UTC, Felix Palmen
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geezer 2021-09-05 03:42:31 UTC
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.
Comment 1 Felix Palmen 2021-09-05 11:22:21 UTC
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).
Comment 2 Geezer 2021-09-05 14:11:20 UTC
Thank you.
Comment 3 Felix Palmen 2021-09-05 14:26:28 UTC
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.
Comment 4 Geezer 2021-09-05 15:00:39 UTC
Yup. A lot simpler.
Comment 5 Tomoaki AOKI 2021-09-06 14:13:15 UTC
(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
Comment 6 Felix Palmen 2021-09-06 17:35:57 UTC
(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.
Comment 7 Felix Palmen 2021-09-07 06:39:06 UTC
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".
Comment 8 Robert Clausecker 2021-09-07 13:39:33 UTC
(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.
Comment 9 Tomoaki AOKI 2021-09-08 12:41:07 UTC
(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.
Comment 10 Tomoaki AOKI 2021-09-08 12:58:45 UTC
(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!
Comment 11 Felix Palmen 2021-09-09 11:04:07 UTC
(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?
Comment 12 Tomoaki AOKI 2021-09-11 06:36:22 UTC
(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.
Comment 13 Felix Palmen 2021-09-13 12:57:50 UTC
(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.
Comment 14 Geezer 2021-09-13 14:01:11 UTC
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.
Comment 15 Felix Palmen 2021-09-13 14:08:24 UTC
(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…
Comment 16 Tomoaki AOKI 2021-09-13 14:50:37 UTC
(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.
Comment 17 Felix Palmen 2021-09-13 14:54:43 UTC
(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.