Page MenuHomeFreeBSD

Rethink %SUBDIR% for Xfce's MASTER_SITES
ClosedPublic

Authored by madpilot on Nov 1 2016, 7:13 PM.

Details

Reviewers
ehaupt
mat
olivierd
Group Reviewers
O5: Ports Framework(Owns No Changed Paths)
portmgr
Commits
rP504557: - Simplify XFCE MASTER_SITES usage
Summary

This patch simplify MASTER_SITES in each Xfce ports. Users can only override the default component (xfce), possible values are:

  • apps
  • art
  • bindings
  • thunar-plugins
  • panel-plugins
  • archive

'xfce' contains core components. Test is needed, for ports where ${PORTNAME} differ from official name.

Diff Detail

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

Event Timeline

olivierd updated this revision to Diff 21902.Nov 1 2016, 7:13 PM
olivierd retitled this revision from to Rethink %SUBDIR% for Xfce's MASTER_SITES.
olivierd updated this object.
olivierd edited the test plan for this revision. (Show Details)
olivierd added a reviewer: mat.
olivierd added subscribers: xfce, ehaupt.
ehaupt accepted this revision.Nov 2 2016, 8:36 AM
ehaupt added a reviewer: ehaupt.

Looks good to me.

This revision is now accepted and ready to land.Nov 2 2016, 8:36 AM
mat added inline comments.Nov 2 2016, 11:58 AM
Mk/bsd.sites.mk
1178–1183

I don't understand the difference in the if and else here.

if DISTNAME without DISTVERSION is PORTNAME, then use a lowercased version of PORTNAME, otherwise, use DISTNAME without DISTVERSION.
I had a quick check, I could not find any port in this diff where both are not the same thing.

Lowercase is for x11-fm/thunar, package name (and tarball) is spelled Thunar, but it is located in src/xfce/thunar/ directory.

Check is required, because I noticed, when ${DISTNAME} is defined in Makefile, path is well formed, we can fetch tarball.

For example x11/libexo

make fetch
===>  License GPLv2 LGPL21 accepted by the user
===>  Found saved configuration for libexo-0.11.2
===>   libexo-0.11.2 depends on file: /usr/local/sbin/pkg - found
=> exo-0.11.2.tar.bz2 doesn't seem to exist in /usr/ports/distfiles/xfce4.
=> Attempting to fetch http://archive.xfce.org/src/xfce/exo/0.11/exo-0.11.2.tar.bz2
exo-0.11.2.tar.bz2                            100% of 1282 kB 1591 kBps 00m01s
===> Fetching all distfiles required by libexo-0.11.2 for building

But, when ${DISTNAME} is not defined in Makefile, it does not work (e.g. x11/xfce4-terminal):

make fetch
===>  License GPLv2 accepted by the user
===>   xfce4-terminal-0.8.1 depends on file: /usr/local/sbin/pkg - found
=> xfce4-terminal-0.8.1.tar.bz2 doesn't seem to exist in /usr/ports/distfiles/xfce4.
=> Attempting to fetch http://archive.xfce.org/src/apps/xfce4-terminal-0.8.1/0.8/xfce4-terminal-0.8.1.tar.bz2
fetch: http://archive.xfce.org/src/apps/xfce4-terminal-0.8.1/0.8/xfce4-terminal-0.8.1.tar.bz2: Not Found

Path should be src/apps/xfce4-terminal/0.8/ not src/apps/xfce4-terminal-0.8.1/0.8/

mat edited edge metadata.Nov 3 2016, 3:32 PM

Lowercase is for x11-fm/thunar, package name (and tarball) is spelled Thunar, but it is located in src/xfce/thunar/ directory.

If there are no cases of the directory being uppercased, then it could always be based on DISTNAME with :tl, so it is a non issue.

Check is required, because I noticed, when ${DISTNAME} is defined in Makefile, path is well formed, we can fetch tarball.
For example x11/libexo

make fetch
===>  License GPLv2 LGPL21 accepted by the user
===>  Found saved configuration for libexo-0.11.2
===>   libexo-0.11.2 depends on file: /usr/local/sbin/pkg - found
=> exo-0.11.2.tar.bz2 doesn't seem to exist in /usr/ports/distfiles/xfce4.
=> Attempting to fetch http://archive.xfce.org/src/xfce/exo/0.11/exo-0.11.2.tar.bz2
exo-0.11.2.tar.bz2                            100% of 1282 kB 1591 kBps 00m01s
===> Fetching all distfiles required by libexo-0.11.2 for building

But, when ${DISTNAME} is not defined in Makefile, it does not work (e.g. x11/xfce4-terminal):

make fetch
===>  License GPLv2 accepted by the user
===>   xfce4-terminal-0.8.1 depends on file: /usr/local/sbin/pkg - found
=> xfce4-terminal-0.8.1.tar.bz2 doesn't seem to exist in /usr/ports/distfiles/xfce4.
=> Attempting to fetch http://archive.xfce.org/src/apps/xfce4-terminal-0.8.1/0.8/xfce4-terminal-0.8.1.tar.bz2
fetch: http://archive.xfce.org/src/apps/xfce4-terminal-0.8.1/0.8/xfce4-terminal-0.8.1.tar.bz2: Not Found

Path should be src/apps/xfce4-terminal/0.8/ not src/apps/xfce4-terminal-0.8.1/0.8/

Right. There is some lazy evaluation problem. This seems to work for me:

_PATH=        ${DISTNAME:S/-${DISTVERSIONFULL}//:tl}/${DISTVERSION:C/^([0-9]+\.[0-9]+).*/\1/}

(Also, can _PATH be renamed to, say, _XFCE_PATH or something, to make it less generic.)

In D8416#175494, @mat wrote:

Lowercase is for x11-fm/thunar, package name (and tarball) is spelled Thunar, but it is located in src/xfce/thunar/ directory.

If there are no cases of the directory being uppercased, then it could always be based on DISTNAME with :tl, so it is a non issue.

Check is required, because I noticed, when ${DISTNAME} is defined in Makefile, path is well formed, we can fetch tarball.
For example x11/libexo

make fetch
===>  License GPLv2 LGPL21 accepted by the user
===>  Found saved configuration for libexo-0.11.2
===>   libexo-0.11.2 depends on file: /usr/local/sbin/pkg - found
=> exo-0.11.2.tar.bz2 doesn't seem to exist in /usr/ports/distfiles/xfce4.
=> Attempting to fetch http://archive.xfce.org/src/xfce/exo/0.11/exo-0.11.2.tar.bz2
exo-0.11.2.tar.bz2                            100% of 1282 kB 1591 kBps 00m01s
===> Fetching all distfiles required by libexo-0.11.2 for building

But, when ${DISTNAME} is not defined in Makefile, it does not work (e.g. x11/xfce4-terminal):

make fetch
===>  License GPLv2 accepted by the user
===>   xfce4-terminal-0.8.1 depends on file: /usr/local/sbin/pkg - found
=> xfce4-terminal-0.8.1.tar.bz2 doesn't seem to exist in /usr/ports/distfiles/xfce4.
=> Attempting to fetch http://archive.xfce.org/src/apps/xfce4-terminal-0.8.1/0.8/xfce4-terminal-0.8.1.tar.bz2
fetch: http://archive.xfce.org/src/apps/xfce4-terminal-0.8.1/0.8/xfce4-terminal-0.8.1.tar.bz2: Not Found

Path should be src/apps/xfce4-terminal/0.8/ not src/apps/xfce4-terminal-0.8.1/0.8/

Right. There is some lazy evaluation problem. This seems to work for me:

_PATH=        ${DISTNAME:S/-${DISTVERSIONFULL}//:tl}/${DISTVERSION:C/^([0-9]+\.[0-9]+).*/\1/}

(Also, can _PATH be renamed to, say, _XFCE_PATH or something, to make it less generic.)

I tried your suggestion, but it fails with some ports.
For failing ports, if I replace ${DISTVERSIONFULL} by ${DISTVERSION} everything is fine, except for ports which were functional at first.

  • x11/xfce4-terminal (not working with ${DISTVERSION}, success with ${DISTVERSIONFULL})
  • x11/libexo (not working with ${DISTVERSIONFULL}, success with ${DISTVERSION})
  • misc/xfce4-wm-themes (not working with ${DISTVERSIONFULL}, success with ${DISTVERSION})
  • x11-fm/thunar (not working with ${DISTVERSION}, success with ${DISTVERSIONFULL})
mat added a comment.Nov 4 2016, 9:51 AM
  • x11/xfce4-terminal (not working with ${DISTVERSION}, success with ${DISTVERSIONFULL})
  • x11/libexo (not working with ${DISTVERSIONFULL}, success with ${DISTVERSION})
  • misc/xfce4-wm-themes (not working with ${DISTVERSIONFULL}, success with ${DISTVERSION})
  • x11-fm/thunar (not working with ${DISTVERSION}, success with ${DISTVERSIONFULL})

Ok, those can also be fixed.

The problem is the way DISTNAME is created.

By default, it is ${PORTNAME}-${DISTVERSIONFULL}. If you ask to match -${DISTVERSIONFULL} to do something, it does not have to be expanded, it can simply use the -${DISTVERSIONFULL} and be done with it.

If you set DISTNAME to foo-${PORTVERSION} and ask to match -${DISTVERSIONFULL} it needs to expand both sides. At that point, I figure DISTVERSIONFULL is not yet usable, and it so, it does not do what you want. On the other hand, if you change to foo-$(DISTVERSIONFULL} (which is the same, from your point of view) it does not need to expand it to match -${DISTVERSIONFULL} and it works.

So, for those two the following patch solves the problem:

--- misc/xfce4-wm-themes/Makefile
+++ misc/xfce4-wm-themes/Makefile
@@ -9 +9 @@ MASTER_SITES=       XFCE/art
-DISTNAME=      xfwm4-themes-${PORTVERSION}
+DISTNAME=      xfwm4-themes-${DISTVERSIONFULL}
--- x11/libexo/Makefile
+++ x11/libexo/Makefile
@@ -8 +8 @@ MASTER_SITES=       XFCE
-DISTNAME=      exo-${PORTVERSION}
+DISTNAME=      exo-${DISTVERSIONFULL}

Adding a comment in bsd.sites.mk in the XFCE block saying that DISTNAME must use DISTVERSIONFULL would be enough to steer people in the right way.

(Everything you definitively did not want to know about how make(1) variables work under the hood)

madpilot updated this revision to Diff 58362.Jun 7 2019, 2:41 PM
madpilot edited the summary of this revision. (Show Details)
madpilot added a subscriber: madpilot.

I merged in the suggestions from mat into the patch from olivierd.

It works fine for me. my testing has been making my machine perform "make checksum" on all affected ports after cleaning the distcache.

I ssee this simplification as useful and I'd like to get it in the tree soon, before XFCE 4.14 is released, so the two big commits don't overlap.

If any other changes are required or problems identified I'll try to addresss those.

This revision now requires review to proceed.Jun 7 2019, 2:41 PM

BTW if an exp run is required let me know and I'll ask for one.

madpilot added inline comments.Jun 7 2019, 2:46 PM
Mk/bsd.sites.mk
1178–1183

I've addressed this using the code you suggested in D8416#175493

I merged in the suggestions from mat into the patch from olivierd.
It works fine for me. my testing has been making my machine perform "make checksum" on all affected ports after cleaning the distcache.
I ssee this simplification as useful and I'd like to get it in the tree soon, before XFCE 4.14 is released, so the two big commits don't overlap.
If any other changes are required or problems identified I'll try to addresss those.

I use this simplification for a while (on my own ports). This should be committed quickly.

madpilot updated this revision to Diff 58738.Jun 17 2019, 4:03 PM

Update patch to latest tree.

madpilot commandeered this revision.Jun 17 2019, 4:13 PM
madpilot added a reviewer: olivierd.

Commandeering this revision, since I've taken over with new patches.

madpilot marked an inline comment as done.Jun 19 2019, 12:46 PM
mat accepted this revision as: portmgr.Jun 19 2019, 1:24 PM

Assuming you did check that everything still works :-)

In D8416#447242, @mat wrote:

Assuming you did check that everything still works :-)

Thanks a lot. I did test but I'm going to do a further test right before committing.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 19 2019, 4:14 PM
This revision was automatically updated to reflect the committed changes.