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 5295
Build 5475: arc lint + arc unit

Event Timeline

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.

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.

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?

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 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

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 :)

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 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.
tcberner edited edge metadata.

Correct the IGNORE message, when an invalid KDE_DIST is passed

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

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 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
Mk/Uses/kde.mk
86

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 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

mark stuff done

tcberner edited edge metadata.

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

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 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

mark stuff done

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 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

The Differential Revision line was not the last line.