Page MenuHomeFreeBSD
Differential D24240

Improve and extend sed_checked.sh
Needs ReviewPublic
Actions

Authored by mandree on Mar 31 2020, 3:53 PM.
Tags
None
Referenced Files
F106074303: D24240.diff
Tue, Dec 24, 11:21 PM
Unknown Object (File)
Tue, Dec 17, 9:22 AM
Unknown Object (File)
Sun, Dec 8, 6:47 AM
Unknown Object (File)
Nov 10 2024, 2:07 PM
Unknown Object (File)
Oct 26 2024, 10:22 AM
Unknown Object (File)
Oct 26 2024, 10:15 AM
Unknown Object (File)
Oct 5 2024, 10:34 AM
Unknown Object (File)
Oct 5 2024, 6:43 AM
View All 99 Files
Subscribers
emaste
imp
manu
mat

Details

Reviewers
mandree
Group Reviewers
portmgr
O5: Ports Framework(Owns No Changed Paths)
Commits
rP552563: Tools/scripts/sed_checked.sh: make more useful
Summary

The current sed_checked.sh validates REINPLACE_CMD expressions in port Makefiles.
It has a few shortcomings that this diff strives to address:

  • the test is blind to whether a REINPLACE_CMD line is completely useless, or partially ineffective - report that because for some machine edits, a broader glob has been common practice
  • REINPLACE_CMD may suppress creation of backup files (-i ''), which invalidates this check, so warn about that
  • report found data immediately, and again through REWARNFILE.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mandree created this revision.Mar 31 2020, 3:53 PM
mandree created this object with edit policy "committers (Project)".
Herald added a subscriber: mat. · View Herald TranscriptMar 31 2020, 3:53 PM
mandree updated this revision to Diff 71702.May 12 2020, 8:32 PM

same code, rediffed with "full" context

mandree added a comment.Jun 4 2020, 9:23 AM

ping

mandree added a subscriber: manu.Jun 16 2020, 2:02 PM

@portmgr @manu @mat @swills

Guys, this has been sitting for five weeks since the full context was provided, and almost three months since initial review request.
Can we get this moving? Can I get approval for commit? I've been using this for 10+ weeks.

mandree added a comment.Jul 16 2020, 3:03 PM

No more complaints received in a month, can I commit?

mandree accepted this revision.Sep 23 2020, 4:56 PM

ping

This revision is now accepted and ready to land.Sep 23 2020, 4:56 PM
mandree removed a reviewer: swills.Sep 23 2020, 4:56 PM
mat added a comment.Sep 30 2020, 8:40 AM
In D24240#568481, @mandree wrote:

No more complaints received in a month, can I commit?

We do not have more complains, but we still do not agree with the change, so the answer is still no.

mandree added a comment.Sep 30 2020, 2:12 PM

OK, then this will escalate to core@ for abuse because there has been no feedback as to why it's unacceptable in spite of solving practical problems.

imp added a subscriber: imp.Sep 30 2020, 3:13 PM
In D24240#592496, @mat wrote:
In D24240#568481, @mandree wrote:

No more complaints received in a month, can I commit?

We do not have more complains, but we still do not agree with the change, so the answer is still no.

Can you articulate a reason? A blanket "no" without an actionable explanation is frowned upon. The code looks fine. The concept seems on its face OK. What's the holdup?

emaste added a subscriber: emaste.Sep 30 2020, 7:51 PM
Closed by commit rP552563: Tools/scripts/sed_checked.sh: make more useful (authored by mandree). · Explain WhyOct 17 2020, 12:37 PM
This revision was automatically updated to reflect the committed changes.
mandree added a commit: rP552563: Tools/scripts/sed_checked.sh: make more useful.
Owners added a reviewer: O5: Ports Framework.Oct 17 2020, 12:37 PM
Herald added 1 blocking reviewer(s): portmgr. · View Herald TranscriptOct 17 2020, 12:37 PM
mandree reopened this revision.Jan 4 2021, 6:15 PM

since swills@ has reverted this as of r552736, reopen.

mandree mentioned this in D27954: BUGFIX: Only call sed_checked.sh if REINPLACE_ARGS hasn't been altered.Jan 4 2021, 6:17 PM
mandree added a comment.Feb 14 2021, 10:53 AM

In the light of findings mentioned in D27954 we might want to add removal of .bak files, but other than that my proposal already looks in-depth at the REINPLACE_ARGS, to complain if something uses a nonstandard -i argument (backup suffix).

Revision Contents
Changeset List

PathSize
Tools/
scripts/
91 lines

Diff 70063

View Options

Tools/scripts/sed_checked.sh

Loading...