Page MenuHomeFreeBSD

Define KDE_DIST instead of MASTER_SITES and DIST_SUBDIR in every KDE port
ClosedPublic

Authored by tcberner on Aug 25 2016, 7:06 PM.

Details

Summary

Add KDE_DIST=foo similar to the Qt ports QT_DIST.

At the moment this supports:

  • kde4
  • kactivities
  • applications:legacy
  • calligra, calligra:l10n

In the future, with KDE Framworks, Plasma and (up to date) Applications ports
this will be extended to include

  • frameworks, framworks:portingaid
  • plasma
  • applications

I chose to add it directly below the USE_KDE=foo entry of the ports.

I also opted to move the distfiles of the kde4 stuff into

  • ${DISTDIR}/KDE/kde4/<version>

from ${DISTDIR}/KDE/<version>, as I intend to do

  • ${DISTDIR}/KDE/frameworks/<version>
  • ${DISTIDR}/KDE/plasma/<version>
  • ${DISTDIR}/KDE/applications/<version>

for the future options -- so this change is mainly done for my OCD.

Diff Detail

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

Event Timeline

tcberner updated this revision to Diff 19685.Aug 25 2016, 7:06 PM
tcberner retitled this revision from to Define KDE_DIST instead of MASTER_SITES and DIST_SUBDIR in every KDE port.
tcberner updated this object.
tcberner edited the test plan for this revision. (Show Details)
tcberner added reviewers: rakuco, mat.
mat edited edge metadata.Aug 26 2016, 12:53 PM

New variables should be used at a minimum, for example, if:

USES=kde:4
KDE_DIST=kde4

could be folded into:

USES=kde:4,dist

it would be great.

mat added a comment.Aug 26 2016, 12:56 PM

Also, all the moves in DISTFILES don't server real purpose, it will invalidate all the distfiles downloaded by people, and, most importantly, the FreeBSD distcache.

How about using USES=kde:4,dist=foo and USES=kde:5,dist=bar this way, it would also be easier to add further arguments to kde.mk should the need ever arise -- instead of USES=kde:4,foo and USES=kde:5,bar?

mat added a comment.Sep 14 2016, 4:10 PM

How about using USES=kde:4,dist=foo and USES=kde:5,dist=bar this way, it would also be easier to add further arguments to kde.mk should the need ever arise -- instead of USES=kde:4,foo and USES=kde:5,bar?

Good idea :-)

tcberner updated this revision to Diff 20350.Sep 14 2016, 6:47 PM
tcberner edited edge metadata.
  • Instead of defining KDE_DIST=foo, pass foo to kde.mk via kde:4,dist=foo
  • Do not move distfiles, even if it breaks your heart :P
Owners edited edge metadata.Sep 14 2016, 6:47 PM
mat added a comment.Sep 14 2016, 7:10 PM

Thinking back about it, I wonder if using an = sign is a good idea, it may confuse things. (not saying it is bad, just thinking out loud)

Also, I think using a : in the args is a bad idea, the : should only be used once, separating the uses from its args. You could easily replace it with an underscore as you are not trying to write sentences but simple words.

Also wondering, if dist=application:legacy is the only valid version of dist=application:*, why not drop the legacy part, or merge lines 90/91 into ​. if ${_KDE_DIST:Mapplications\:legacy} so that using application:foo is caught as being bad. (See, using a colon is bad, you have to escape it when doing :M)

Yes, it looks a bit kludgey...
As mentioned in the beginning, once we have KF5 imported there will be dist=applications for the up-to-date version of KDE applications, the :legacy versions can then hopefully be dropped after some time...

Instead of using applications:legacy how about the good old applications.legacy?

Maybe we could also do the magic without using dist or KDE_DIST.
kde.mk could maybe do it itself, by comparing ${PORTVERSION} with the ones it knows about in case MASTER_SITES and DIST_SUBDIR is not set in the ports Makefile?
That would certainly make the Makefiles cleaner again to read :)

mat added a comment.Sep 14 2016, 8:49 PM

Yes, it looks a bit kludgey...
As mentioned in the beginning, once we have KF5 imported there will be dist=applications for the up-to-date version of KDE applications, the :legacy versions can then hopefully be dropped after some time...
Instead of using applications:legacy how about the good old applications.legacy?

Sure, that works too, and avoids the : :-)

Maybe we could also do the magic without using dist or KDE_DIST.
kde.mk could maybe do it itself, by comparing ${PORTVERSION} with the ones it knows about in case MASTER_SITES and DIST_SUBDIR is not set in the ports Makefile?
That would certainly make the Makefiles cleaner again to read :)

Ah, well, if you can make things automatic, it is even better :-)

Trying it out, I don't think the magic will work very nicely :/ -- just having the version to decide what the port is works fine, unless there are exceptions...

I think sticking with passing an argument to kde.mk is probably the cleanest way. I'll go and think a little about how to name it, so it's not as
kludgey -- maybe just passing it directly like you initially suggested is the best way after all :)

tcberner updated this revision to Diff 20386.Sep 16 2016, 5:56 PM
tcberner edited edge metadata.

And an other try :)

  • Get rid again of the dist=, and simply pass the distname as argument.
  • Do not overcomplicate it, and instead of splitting the distarg, just use it as is, i.e. no longer calligra and calligra:l10n but simply calligra_l10n for the latter.
Owners edited edge metadata.Sep 16 2016, 5:56 PM
tcberner updated this revision to Diff 20388.Sep 16 2016, 6:22 PM
tcberner edited edge metadata.

Correct the IGNORE message, when an invalid KDE_DIST is passed

Owners edited edge metadata.Sep 16 2016, 6:22 PM

Other stupid idea: Add additional virtual CATEGORIES, to the existing kde: kde-frameworks, kde-plasma, kde-applications.
And only apply this to the new ports for applications-16.*, kf5-* and plasma5*-.

CATEGORIES= foo kde

becomse

CATEGORIES= foo kde-frameworks

mat added a comment.Sep 21 2016, 1:19 PM

Other stupid idea: Add additional virtual CATEGORIES, to the existing kde: kde-frameworks, kde-plasma, kde-applications.
And only apply this to the new ports for applications-16.*, kf5-* and plasma5*-.

CATEGORIES= foo kde

becomse

CATEGORIES= foo kde-frameworks

That sounds a good idea, maybe foo kde kde-frameworks

Cool :) I'll update the diff tomorrow to use this.

tcberner updated this revision to Diff 20625.Sep 22 2016, 9:11 PM
tcberner edited edge metadata.

Add new virtual category kde-kde4 .

If MASTER_SITES or DIST_SUBDIR is not set, kde.mk will fill now in the blanks
depending on the kde-foo CATEGORY present. At the moment this is only kde-kde4.

With the import of the new KDE ports the following additional categories would be added:

  • kde-frameworks: ~ 70 ports
  • kde-plasma: ~ 40 ports
  • kde-applications: ~200 ports
Owners edited edge metadata.Sep 22 2016, 9:11 PM
mat added inline comments.Sep 23 2016, 10:38 AM
Mk/Uses/kde.mk
87

I don't think :O:u are needed here, the variable is literally defined on the previous line and can be kept clean. If it needs to be sorted, maybe just add a comment about it, as the unique thing, well, eyes should be enough to figure that out before committing a change 😄

Mk/bsd.port.mk
2442

I wonder if ${_KDE_CATEGORIES_SUPPORTED} could not be added here instead.

I'll give the bsd.port.mk change a go :) -- but there seems to be an issue with metaports, as they now try to fetch a distfile. However, I cannot check for empty(DISTFILES) in kde.mk it seems.

tcberner updated this revision to Diff 20660.Sep 23 2016, 9:24 PM
tcberner edited edge metadata.
  • Slightly simplify the code in kde.mk: the outer if to check for empty MASTER_SITES or DIST_SUBDIR is not really necessary, and we can just check for the category.
  • Use ${_KDE_CATEGORIES_SUPPORTED} in bsd.port.mk
Owners edited edge metadata.Sep 23 2016, 9:24 PM
tcberner marked 2 inline comments as done.Sep 23 2016, 9:25 PM

mark stuff done

tcberner updated this revision to Diff 20661.Sep 23 2016, 9:27 PM
tcberner edited edge metadata.

Actually remove :O:u .from the for-loop :D -- does not need to be sorted at all.

Owners edited edge metadata.Sep 23 2016, 9:27 PM
rakuco edited edge metadata.Oct 10 2016, 8:45 AM

The CATEGORIES approach looks unusual, but seems to work well. If mat is OK with it, so am I. I guess there's no need to change any of the distinfo files, which would reduce the size of this diff quite a bit.

Mk/Uses/kde.mk
102

Is this comment still relevant? I don't see anything Calligra-specific below.

editors/calligra-l10n/files/bsd.l10n.mk
9

This looks like a leftover from a previous iteration of this patch.

tcberner updated this revision to Diff 21265.Oct 11 2016, 6:41 PM
tcberner edited edge metadata.

Adress issues mentioned by Raphael:

  • Remove the distinfo changes
  • Remove old entry in calligra's bsd.l10n.mk
  • Remove calligra comment from kde.mk
Owners edited edge metadata.Oct 11 2016, 6:41 PM
tcberner marked 2 inline comments as done.Oct 11 2016, 6:42 PM

mark stuff done

rakuco accepted this revision.Oct 11 2016, 6:48 PM
rakuco edited edge metadata.

There are a few things you should do before landing:

  • Update the description of this review request, as things have changed significantly compared to the original idea.
  • Ask for an exp-run to see if nothing broke; if possible, the exp-run shouldn't use FreeBSD's distcache so we can verify no port was broken by this change.
mat accepted this revision.Oct 12 2016, 10:37 AM
mat edited edge metadata.

Looks in great shape, can't wait for the exp-run :-)

This revision is now accepted and ready to land.Oct 12 2016, 10:37 AM
tcberner closed this revision.Oct 18 2016, 9:25 PM

Committed: https://svnweb.freebsd.org/changeset/ports/424182
[somehow it didn't close it automatically].

mat added a comment.Oct 18 2016, 9:38 PM

The Differential Revision line was not the last line.