Page MenuHomeFreeBSD

Mk/bsd.options.desc.mk: Add version control systems OPTION descriptions
Needs ReviewPublic

Authored by loader on Oct 7 2018, 3:43 PM.

Details

Reviewers
koobs
kevans
Group Reviewers
portmgr
Summary

Proposed commit log message:

Mk/bsd.options.desc.mk: Add version control OPTIONS descriptions

- Add BAZAAR description
- Add CVS description
- Add DARCS description
- Add GIT description
- Add MERCURIAL description
- Add PERFORCE description
- Rename SVN to SUBVERSION


Reviewed_by: koobs, adamw
Approved by: koobs (ports, mentor)
Differential_Revision: D17459

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 31920
Build 29471: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
koobs requested changes to this revision.Nov 23 2018, 3:44 AM

Please also include the updates to any ports that use these OPTIONS in their Makefile's already (thereby leveraging the new shared descriptions)

Apologies for the delayed review

This revision now requires changes to proceed.Nov 23 2018, 3:44 AM

Changes to Mk/bsd.options.desc.mk don't explicitly require (block on) portmgr approval

# - This file is maintained by ports@FreeBSD.org so that entries can be added
#   to it easily.  Any sweeping changes should be approved by portmgr@.

Options_Desc_MAINTAINER=        ports@FreeBSD.org

Please also include the updates to any ports that use these OPTIONS in their Makefile's already (thereby leveraging the new shared descriptions)

Do I also need to add RSYNC_DESC, DARCS_DESC and BAZAAR_DESC into bsd.options.desc.mk?

https://svnweb.freebsd.org/ports/head/devel/ocaml-opam/Makefile?revision=483093&view=markup#l30

30      RSYNC_DESC=             Remote repository synchronized with Rsync
31      GIT_DESC=               Remote repository synchronized with git
32      HG_DESC=                Remote repository synchronized with mercurial
33      DARCS_DESC=             Remote repository synchronized with darcs

https://svnweb.freebsd.org/ports/head/devel/cvs2svn/Makefile?revision=479407&view=markup#l30

30      SUBVERSION_DESC=        Build with subversion support
31      GIT_DESC=               Build with git support
32      BAZAAR_DESC=            Build with Bazaar support

If we are going to change all these descriptions, I should open a Bugzilla report to get an approval from the maintainer for the ports like devel/ocaml-opam, right?

You can add whatever *default* description, but those descriptions are really bad most of the time, they only help if you already know what they do because they just say "FOO_DESC= foo support". If you don't know what foo is, or whatever it is used for, you can't know what it is going to change for the port. Most port should define their own _DESC to something really descriptive, that explains what it does.

Please also include the updates to any ports that use these OPTIONS in their Makefile's already (thereby leveraging the new shared descriptions)

Do I also need to add RSYNC_DESC, DARCS_DESC and BAZAAR_DESC into bsd.options.desc.mk?

https://svnweb.freebsd.org/ports/head/devel/ocaml-opam/Makefile?revision=483093&view=markup#l30

30      RSYNC_DESC=             Remote repository synchronized with Rsync
31      GIT_DESC=               Remote repository synchronized with git
32      HG_DESC=                Remote repository synchronized with mercurial
33      DARCS_DESC=             Remote repository synchronized with darcs

https://svnweb.freebsd.org/ports/head/devel/cvs2svn/Makefile?revision=479407&view=markup#l30

30      SUBVERSION_DESC=        Build with subversion support
31      GIT_DESC=               Build with git support
32      BAZAAR_DESC=            Build with Bazaar support

If we are going to change all these descriptions, I should open a Bugzilla report to get an approval from the maintainer for the ports like devel/ocaml-opam, right?

Yes, I suppose it makes (logical) sense to include any/all other "revision control system" options that exist in ports that, as @mat said, use very generic default descriptions, of the form "{Build with|Include|Enable} $repository support".

The ocaml-opam options are bit too specific (in my opinion) to include here.

Please include the individual port changes in this diff too (the ports that will no longer have OPTIONS_DESC set after adding them to bsd.options.desc.mk

loader edited the summary of this revision. (Show Details)

Add {BZR,CVS,RSYNC}_DESC and a few ports changes

Here's a list of the ports that have the same description in bsd.options.desc.mk (changes are included in this diff):

GIT_DESC:
ports-mgmt/portest|ultima@FreeBSD.org|Git support
ports-mgmt/portshaker|romain@FreeBSD.org|git support
www/py-rhodecode|wg@FreeBSD.org|GIT support

HG_DESC:
ports-mgmt/portshaker|romain@FreeBSD.org|mercurial support

RSYNC_DESC:
ports-mgmt/portshaker|romain@FreeBSD.org|rsync support
shells/scponly|garga@FreeBSD.org|rsync support

These ports remain unchanged:

BAZAAR_DESC:
devel/cvs2svn|ohauer@FreeBSD.org|Build with Bazaar support
BZR_DESC:
deskutils/zim|rm@FreeBSD.org|Version control for notebooks using bzr

CVS_DESC:
devel/git|garga@FreeBSD.org|Enable CVS support
devel/git-gui|garga@FreeBSD.org|Enable CVS support
devel/git-lite|garga@FreeBSD.org|Enable CVS support
devel/git-subversion|garga@FreeBSD.org|Enable CVS support

DARCS_DESC:
devel/ocaml-opam|hannes@mehnert.org|Remote repository synchronized with darcs

GIT_DESC:
deskutils/zim|rm@FreeBSD.org|Version control for notebooks using git
devel/cvs2svn|ohauer@FreeBSD.org|Build with git support
devel/ocaml-opam|hannes@mehnert.org|Remote repository synchronized with git
devel/thunar-vcs-plugin|sergey.dyatko@gmail.com|Git support
devel/tig|tobik@FreeBSD.org|Install devel/git as runtime dependency
sysutils/password-store|rene@FreeBSD.org|Enable git storage
www/moinmoincli|0mp@FreeBSD.org|Install git to use git-diff(1) instead of diff(1) for colored output
www/py-kallithea|freebsd@skinc.ru|Git repositories support

HG_DESC:
devel/ocaml-opam|hannes@mehnert.org|Remote repository synchronized with mercurial

MERCURIAL_DESC:
deskutils/zim|rm@FreeBSD.org|Version control for notebooks using hg

RSYNC_DESC:
devel/bzrtools|fullermd@over-yonder.net|rsync for rspush
devel/ocaml-opam|hannes@mehnert.org|Remote repository synchronized with Rsync
devel/p5-SVN-Notify-Mirror|bofh@FreeBSD.org|Enable rsync for remote working copy update
net/prosearch|ports@FreeBSD.org|crawler with RSYNC
sysutils/zxfer|ports@scaleengine.com|Enable RSYNC Support

Should I add these ports maintainers to reviewers?
garga,romain,ultima,wg

koobs requested changes to this revision.Jul 22 2019, 3:37 AM

For all options added in Mk/bsd.options.desc.mk I think we should have 'version control` in the description, like

MERCURIAL_DESC= Mercurial version control support

After changing the names in Mk/bsd.options.desc.mk to their full names, update ports that use those names too

Mk/bsd.options.desc.mk
41

Rename to full name (BAZAAR)

158

Rename to MERCURIAL

This revision now requires changes to proceed.Jul 22 2019, 3:37 AM

Mk/bsd.options.desc.mk MAINTAINER is:

Options_Desc_MAINTAINER= ports@FreeBSD.org

No need for protmgr blocking review

koobs retitled this revision from Mk/bsd.options.desc.mk: Add GIT and MERCURIAL descriptions to Mk/bsd.options.desc.mk: Add version control systems OPTION descriptions.Jul 22 2019, 3:39 AM
koobs removed a reviewer: portmgr.

Perhaps these could say Git repository support or, even better, Git code repository support.

I'm sick of LibXYZ support descriptions.

For all options added in Mk/bsd.options.desc.mk I think we should have 'version control` in the description, like

MERCURIAL_DESC= Mercurial version control support

After changing the names in Mk/bsd.options.desc.mk to their full names, update ports that use those names too

Should I also change the SVN_DESC?= Subversion support to SUBVERSION_DESC?= Subversion version control support in Mk/bsdoptions.desc.mk?

For all options added in Mk/bsd.options.desc.mk I think we should have 'version control` in the description, like

MERCURIAL_DESC= Mercurial version control support

After changing the names in Mk/bsd.options.desc.mk to their full names, update ports that use those names too

Should I also change the SVN_DESC?= Subversion support to SUBVERSION_DESC?= Subversion version control support in Mk/bsdoptions.desc.mk?

Yep, all options should be full names, with '<name> version control support' as the description

We should also add to this review any changes to existing ports to have them use the new names consistently. This will be your "sweeping commit" mentee item

Perhaps these could say Git repository support or, even better, Git code repository support.

I'm sick of LibXYZ support descriptions.

My previous comments/request for changes included this

Change version control OPTIONs to their full name and update ports with these OPTIONs

I'm not sure if it's okay to update the OPTION descriptioins in some ports or just keep them as is,
for example, GIT_DESC in www/moinmoincl/Makefile:

GIT_DESC=        Install git to use git-diff(1) instead of diff(1) for colored output

How do I get approval from all these port maintainers? Should I add all of them to the reviewer list?

I'm not sure if it's okay to update the OPTION descriptioins in some ports or just keep them as is,
for example, GIT_DESC in www/moinmoincl/Makefile:

GIT_DESC=        Install git to use git-diff(1) instead of diff(1) for colored output

We dont need to change any <OPTIONS_DESC> used in any ports who's descriptions dont match the default description, that is <name> version control support. That is to say, we leave port defined descriptions in place is they already better/more correctly describe the option

If you've removed any <OPTIONS>_DESC from ports that don't match the default description, re-add them (remove them from this diff)

How do I get approval from all these port maintainers? Should I add all of them to the reviewer list?

For any ports where we *remove* the <OPTIONS>_DESC because it matches the default description, approval is not required, as the change is a `framework / sweeping change' that doesn't require approval.

Said another way, it doesnt require approval because were just switching from *ports* defined descriptions, to *equivalent* (or better described) *shared* descriptions.

koobs requested changes to this revision.Jul 26 2019, 5:10 AM
koobs added inline comments.
Mk/bsd.options.desc.mk
433

Is this not intended to be 'Rsync version control support` ?

devel/anjuta/Makefile
82

For this (anjuta) port: Is this option for explicitly and only for 'version control' support, or something else/more?

If so, this change is fine.

If not, change the option name (SVN -> SUBVERSION) but leave the defined (overriding) description

devel/bzrtools/Makefile
19 ↗(On Diff #60046)

For this (bzrtools) port: Is this option for *explicitly and only* for 'version control' support, or something else/more?

If so, this change is fine.

If not, or its valuable/meaningful to keep the 'for rspush' description in place, change the option name (SVN -> SUBVERSION) but leave the defined (overriding) description

devel/ocaml-opam/Makefile
30–33

For this (ocaml-opam) port: Is this option for explicitly and only for 'version control' support, or something else/more?

If so, this change is fine.

If not, or if the existing port-defined description is *more* correct/meaningful, change the option names but leave the defined (overriding) descriptions

devel/p5-SVN-Notify-Mirror/Makefile
29 ↗(On Diff #60046)

For this port: Is this option for explicitly and only for 'version control' support, or something else/more?

If so, this change is fine.

If not, or the port-defined description is *more* meaningful/correct, leave the defined (overriding) description

devel/tig/Makefile
31 ↗(On Diff #60046)

For this port: Is this option for explicitly and only for 'version control' support, or something else/more?

If so, this change is fine.

If not, leave the port-defined (overriding) description

net-mgmt/rancid3/Makefile
47–49

For this port: Is this option for explicitly and only for 'version control' support, or something else/more? The current description reads like it *swaps* version control support from CVS to SVN (does this mean CVS is no longer supported?)

If so, this change is fine.

If not, change the option name (SVN -> SUBVERSION) but leave the defined (overriding) description

net/prosearch/Makefile
31 ↗(On Diff #60046)

This ports description doesn't like it only means 'version control support'. Is this option for explicitly and only for 'version control' support, or something else/more?

If so, this change is fine.

If not, or if the port-defined description is *more* meaningful/correct, leave the defined (overriding) description

security/hydra/Makefile
6

Why 0 not 1 ?

sysutils/password-store/Makefile
29 ↗(On Diff #60046)

For this port: Is this option for explicitly and only for 'version control' support, or something else/more? This description reads like it ("storage") is not explicitly (or only) about version control.

If so, this change is fine.

If not, or the port-defined description is *more* meaningful/correct, leave the defined (overriding) description

sysutils/zxfer/Makefile
21 ↗(On Diff #60046)

For this port: Is this option for explicitly and only for 'version control' support, or something else/more? It doesn't read like its a 'version control support' function, just a transfer/sync function.

If so, this change is fine.

If not, or the port-defined description is *more* correct/accurate, leave the defined (overriding) description

textproc/docproj/Makefile
41

Does "from ports" here mean that if the option is not defined, it uses svnlite from base?

If so, the shared 'Subversion version control support' is probably not appropriate, and the port-defined description should remain.

If not, and this options explicitly and only means 'version control support', this change is fine.

www/moinmoincli/Makefile
24 ↗(On Diff #60046)

This option doesn't read like 'git version control support', but more like 'Use git-diff instead of bundled diff`

If the existing port-defined description is *more* accurate/meaningful, leave it

This revision now requires changes to proceed.Jul 26 2019, 5:10 AM
Mk/bsd.options.desc.mk
433

Rsync doesn't do version control AFAIK.

This should be something like Remote syncing support, but it doesn't seem related to the rest of this patch.

Remove RSYNC_DESC and keep some of the port defined descritions.

loader added inline comments.
Mk/bsd.options.desc.mk
433

RSYNC_DESC was added because of the port devel/ocaml-opam
https://reviews.freebsd.org/D17459#389750
I have deleted RSYNC_DESC from this review.

security/hydra/Makefile
6

Sorry, typo.

@loader Let's get this sorted/complete so we can concentrate on graduating you (See D25395)

See also: https://wiki.freebsd.org/KubilayKocak/MentoringChecklist

loader marked an inline comment as done.

Update patch on a recent ports tree (r539330)

Mk/bsd.options.desc.mk
158

Just a head's up, MERCURIAL is no longer sorted here after the rename.

sort MERCURIAL_DESC and SUBVERSION_DESC in bsd.options.desc.mk

loader added inline comments.
Mk/bsd.options.desc.mk
158

Thanks @adamw.

@kevans is assisting me with my mentorship duties, to otherwise unblock this review/change, because I am super occupied for at least a week

@kevans please add yourself to co-mentors for @loader

bsd.options.desc.mk changes are not blocked by anyone, the file is explicitly available for anyone to commit to. update reviewers accordingly