Page MenuHomeFreeBSD

New port: devel/grantlee5 and Uses/grantlee.mk
ClosedPublic

Authored by tcberner on Aug 7 2016, 10:04 PM.

Details

Summary

The upcoming KDE Frameworks ports need grantlee5

  • Add devel/grantlee5 and move installed headers into a subdirectory include/grantlee5
  • Move header files from devel/grantlee into a subdirectory

include/grantlee4

   to make sure ports do not pick up the wrong headers
*  Add Uses/grantlee.mk to handle the PLIST_SUB and LIB_DEEPENDS needed 
   by ports using grantlee (before we set the PLIST_SUB manual in very
   of the depending ports, which now should not be needed anymore).  
*  The ports depending on devel/grantlee have been version bumped
   and modified from  
      LIB_DEPENDS=libgrantlee_gui.so:devel/grantlee
   to
      USES=grantlee:4
Test Plan

Tested in poudriere.
[And has been used in the experimental KDE repo for quite a while].

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tcberner updated this revision to Diff 19108.Aug 7 2016, 10:04 PM
tcberner retitled this revision from to New port: devel/grantlee5 and Uses/grantlee.mk.
tcberner updated this object.
tcberner edited the test plan for this revision. (Show Details)
tcberner added reviewers: rakuco, mat.
mat added inline comments.Aug 7 2016, 11:07 PM
Mk/Uses/grantlee.mk
1–15 ↗(On Diff #19108)

Try to use the same format as other USES, first the feature/usage/args, then a description of the args, then what it is for and all.

6 ↗(On Diff #19108)

Use english :-)

17 ↗(On Diff #19108)

MAINTAINER should be email addresses.

47 ↗(On Diff #19108)

This feels like being missnomed, it should only contain the library name *.so, or be called *_LIB_DEPEND or something.

50 ↗(On Diff #19108)

My first reaction was to say "use a more specifc term and not SHLIB_VER" but then, I read the grantlee 4 port.
After reading it, I would say change the build argument to a selfbuild or something, so that people do not get tempted to use it to only add a BUILD_DEPEND.

mat edited edge metadata.Aug 7 2016, 11:08 PM

Also, make sure to svn cp grantlee grantlee5 before committing.

rakuco edited edge metadata.Aug 8 2016, 11:17 AM

I think some things should be done differently to make this patch smaller and easier to understand:

  • USES=grantlee:build is confusing, as mat already pointed out. I'd go further than what him and suggest it's dropped altogether: in general, Uses/foo.mk is used by ports depending on foo, not foo itself. I don't think it's bad to move grantlee:build's logic to the grantlee ports themselves.
  • As usual, I'm not a big fan of those sed calls in devel/grantlee and devel/grantlee5. Proper patches with more context such as Fedora's are easier to understand.

I was also going to suggest not changing devel/grantlee's installation paths, but I guess this doesn't work if some port passes -I/usr/local/include to the compiler too early.

devel/grantlee5/pkg-descr
4 ↗(On Diff #19108)

Gitorious no longer exists; I've updated devel/grantlee's WWW in rP419815, you should do the same to grantlee5.

tcberner updated this revision to Diff 19181.Aug 10 2016, 4:58 PM
tcberner edited edge metadata.
  • Rewrite the comments in grantlee.mk
  • rename 'build' to 'selfbuild'
  • rename *_LIB to *_LIB_DEPEND
  • update WWW
Owners edited edge metadata.Aug 10 2016, 4:58 PM
tcberner marked 5 inline comments as done.Aug 10 2016, 5:00 PM

mark stuff done

@rakuco I kept selfbuild as it allows us to only define the versions inside grantlee.mk and suck them into the Makefiles of devel/grantlee and devel/grantlee5.

mat accepted this revision.Aug 11 2016, 3:30 PM
mat edited edge metadata.
This revision is now accepted and ready to land.Aug 11 2016, 3:30 PM

I still think grantlee.mk can be improved (please don't take it as bikeshedding):

  • The documentation at the top should mention that additional variables are available in PLIST_SUB. Similarly, if GRANTLEE_VERSION is a public variable it should be documented.
  • You can get rid of GRANTLEE_LIB_DEPEND.
  • If you rename the SHLIB_VER plist substitution to something less generic, I don't see why not make both that variable and GRANTLEE_VER available to all ports (plus AFAIK "shlib version" refers to the first number in the version field and what the binaries actually depend on in the ELF header, and it would be "0" in this case).

In the end, I'm thinking of something like this:

# ...

GRANTLE_VERSION= ...

.if empty(grantlee_ARGS:Mselfbuild)
LIB_DEPENDS+= ${GRANTLEE${_grantlee_version}_LIB_DEPEND}
.endif

PLIST_SUB+=  GRANTLEE_VERSION_SHORT=${GRANTLEE_VERSION:R} \
             GRANTLEE_VERSION_FULL=${GRANTLEE_VERSION}

(the PLIST_SUB names above are suggestions and may be too long)

tcberner updated this revision to Diff 19265.Aug 14 2016, 7:34 PM
tcberner edited edge metadata.
tcberner marked an inline comment as done.

Rename the plist subs to the suggested names, and increase the documentation
in grantlee.mk. Further do the suggested simplification of grantlee.mk.

This revision now requires review to proceed.Aug 14 2016, 7:34 PM

One thing that has just occurred to me is that the PORTREVISION bumps in the non-grantlee ports are not necessary: the dependencies remain the same (especially if you use libgrantlee_gui.so in GRANTLEE4_LIB_DEPEND) and the plist also remains identical, everything you're changing do not reflect in changes in the generated packages.

tcberner updated this revision to Diff 19266.Aug 14 2016, 9:13 PM
tcberner edited edge metadata.

Get rid of PORTREVISION bumps, and set GRANTLEE4_LIB_DEPEND to libgrantlee_gui.so

rakuco accepted this revision.Aug 14 2016, 9:21 PM
rakuco edited edge metadata.

lgtm at last :-)

This revision is now accepted and ready to land.Aug 14 2016, 9:21 PM
mat added a comment.Aug 15 2016, 11:31 AM

Oh, now that I think about it, could you run the grantlee.mk file through the indenter I wrote a while back ?

Tools/scripts/indent_make_if.pl Mk/Uses/grantlee.mk

I will run all the Mk files through it at one point, and it would be nice if new files were already indented the same way.

mat added a comment.Aug 15 2016, 11:32 AM

Also, don't forget to edit devel/Makefile to hook the new port to the build.

tcberner updated this revision to Diff 19289.Aug 15 2016, 3:05 PM
tcberner edited edge metadata.

Run the ident-script on grantlee.mk and add grantlee5 to devel/Makefile

This revision now requires review to proceed.Aug 15 2016, 3:05 PM
tcberner updated this revision to Diff 19290.Aug 15 2016, 3:20 PM
tcberner edited edge metadata.

Fixup svn properties.

rakuco accepted this revision.Aug 15 2016, 3:21 PM
rakuco edited edge metadata.

lgtm, thank you

This revision is now accepted and ready to land.Aug 15 2016, 3:21 PM
This revision was automatically updated to reflect the committed changes.