Bug 275487
| Summary: | x11-themes/xfce-icons-elementary (still) not prefix-safe | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Mikhail T. <freebsd-2024> | ||||||||
| Component: | Individual Port(s) | Assignee: | freebsd-xfce (Nobody) <xfce> | ||||||||
| Status: | Closed FIXED | ||||||||||
| Severity: | Affects Only Me | CC: | madpilot | ||||||||
| Priority: | --- | Flags: | madpilot:
maintainer-feedback+
|
||||||||
| Version: | Latest | ||||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
Hi, Thanks for the patch! You make good points, Just to be pedantic I'll elaborate a little: I suspect the improper management of prefix could be a vestige of the past when (I have not checked it is a mere hypothesis) elementary theme build system did not allow it as an argument. Also while I agree that it is anyway better to respect CFLAGS, there is not strict rule on that about programs that are not actually installed on the system. Upstream could in fact have special needs for cflags of internal tools that could conflict with ports system CFLAGS. This is not the case though, so your patch looks fine in this respect too. BTW the += is not needed at that point of the Makefile, we have still to include any other makefile, so there is no existing MAKE_ARGS variable to append to. I'm going to test this patch and commit it soon. Created attachment 246730 [details]
patch v2
Luckily poudriere took very little for a first test and I noticed one thing I did notice in my first commit.
The PREFIX variable is already included by default in MAKE_ENV, so I don't see the need to add it to MAKE_ARGS too.
Seems to work fine like that, so I'm proposing a simpler patch.
Could you also test this?
If the PREFIX value passed via MAKE_ENV is not enough we need to investigate why, because it indicates something wrong in the upstream Makefile and the correct fix is not passing the value twice, but fixing the Makefile.
I also made the REINPLACE command silent, since this is the common practice in the ports tree, and also the way suggested in all the porter's handbook examples.
> The PREFIX variable is already included by default in MAKE_ENV
Maybe, that's only true during some stages :( Without the PREFIX added explicitly, all of the files (except license) are installed under work/stage/usr/local instead of work/stage/opt here, and I get an error during "make package":
mi@aldan:ports/x11-themes/xfce-icons-elementary (1150) find work/stage/opt -type f
work/stage/opt/share/licenses/xfce-icons-elementary-0.18/catalog.mk
work/stage/opt/share/licenses/xfce-icons-elementary-0.18/LICENSE
work/stage/opt/share/licenses/xfce-icons-elementary-0.18/GPLv2
mi@aldan:ports/x11-themes/xfce-icons-elementary (1151) find work/stage/usr/local -type f | wc -l
8803
In fact, that's exactly why I bothered to submit this PR in the first place -- had it worked properly (during the rebuild of xfce), I wouldn't even have noticed the CFLAGS nit...
(In reply to Mikhail T. from comment #3) Thanks for the quick test. Looks like the upstream Makefile has some issue though. I want to investigate a little more. I'll followup as soon as I have some information. (In reply to Guido Falsi from comment #2) > The PREFIX variable is already included by default in MAKE_ENV Being part of the /environment/ is unsufficient to overwrite the assignment inside Makefile. My patch makes part of the MAKE_ARGS, which is stronger. Alternatively, one can add `-e PREFIX` to the arguments -- to indicate, that in the case of this particular variable (PREFIX), the value found in the environment (if any) shall trump the setting inside Makefile. Either way a change to MAKE_ARGS is necessary, and, I think, mine is more straightforward. Created attachment 246732 [details]
patch v3
This new patch should work.
Upstream code uses a very simple configure script to unconditionally ad PREFIX in the Makefile. That script is ignoring environment, and only accepts a "--prefix=" command line argument.
Adding such an argument should fix things.
It could also be fixed by changing "PREFIX=" to "PREFIX?=" in their Makefile.in. But I feel adding a CONFIGURE_ARGS line is less invasive than adding a patch.
Please report back this one really works for you. In the while I'll do some more testing here.
Yes, the explicit setting of CONFIGURE_ARGS works just as well as that of MAKE_ARGS. Why is one better than the other, I do not know, but I don't have to know -- I'm not the port's maintainer :) > But I feel adding a CONFIGURE_ARGS line is less invasive than adding a patch. Full agreement here. > I also made the REINPLACE command silent, since this is the > common practice in the ports tree It being the common practice is hardly an argument :) I prefer such things visible to make it easier to review build-logs. But, again, this is not my port... (In reply to Mikhail T. from comment #7) > Why is one better than the other (?) first of all we are running configure anyway so let's give it the correct argument. Also, this is clearly how upstream intended things to be done. Also passing the same information twice goes against multiple generic coding rules, for example DRY (Don't Repeat Yourself). IMHO upstream build system could be improved, so well, anything would look a little hacky here. The port was clearly wrong before, now that you made me look at it (for a good reason) I try to produce the best fix possible. Regarding REINPLACE: While I could agree that being common practice is not a definitive argument (but conventions are important), I consider the fact that the porter's handbook ALWAYS has it silent in the examples is a very strong suggestion. The reinplace command is usually quite verbose and tends to clog the output. The handbook does not give a definitive rule except saying that installation commands should never be silenced. Anyway I'm going to commit this latest patch shortly, now that we have it working. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=40eb0c4f9753d898422c4aa8d7882710be966f13 commit 40eb0c4f9753d898422c4aa8d7882710be966f13 Author: Guido Falsi <madpilot@FreeBSD.org> AuthorDate: 2023-12-02 23:09:03 +0000 Commit: Guido Falsi <madpilot@FreeBSD.org> CommitDate: 2023-12-02 23:09:03 +0000 x11-themes/xfce-icons-elementary: Respect PREFIX, CFLAGS - Make the port PREFIX safe by correctly passing the prefix argument to its configure script - Make the port respect CFLAGS when building an internal (not installed with the pkg) binary tool - Make REINPLACE_CMD silent PR: 275487 x11-themes/xfce-icons-elementary/Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) Patch committed. Thanks! |
Created attachment 246728 [details] Explicitly set PREFIX, use proper CFLAGS Although the port's post-patch target attempts to fix PREFIX, it does that incorrectly. The attached diff offers an alternative fix to PREFIX, and also ensures, that the svgtopng compiled (but not installed) by the port is built using the local ${CFLAGS}.