Page MenuHomeFreeBSD

bsd.port.mk: Switch to USES=gl
ClosedPublic

Authored by zeising on Aug 17 2018, 3:59 PM.

Details

Summary

Switch to USES=gl instead of USE_GL=

This is designed such that you need USES=gl, and then specify components as needed with USE_GL= as before. For backwards compatibility, USE_GL= automatically sets USES=gl.

See also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=230692

Test Plan

exp-run

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

linimon retitled this revision from Switch to USES=gl to bsd.port.mk: Switch to USES=gl.Aug 17 2018, 7:49 PM
matthew added inline comments.
Mk/Uses/gl.mk
6 ↗(On Diff #46847)

Spelling: 'component'

Mk/Uses/gl.mk
6 ↗(On Diff #46847)

further more this comment contradicts the statement later on the USES=gl takes no arguments.

Mk/Uses/gl.mk
6 ↗(On Diff #46847)

Could you maybe expand component and argument a bit, as just afterwards yo say the script does not take any.

28 ↗(On Diff #46847)

Remove quotes around yes.

28–30 ↗(On Diff #46847)

USE_GL=yes should generate a DEV_WARNING too, as this is now only an optional argument to the USES.

Mk/bsd.port.mk
366–369 ↗(On Diff #46847)

This whole bit should be moved to Mk/Uses/gl.mk.

1427–1429 ↗(On Diff #46847)

You should add a check testing that USES does not already contains gl.

Also, add a DEV_WARNING saying that using USE_GL without USES=gl is wrong.

I've updated the patch to take into account the comments in the review.

The comment at the top of Uses/gl.mk was wrong, hence the confusion. It was a leftover from a previous attempt at implementing this.
The USE_GL= must remain, it's how to select which components to use. It's modeled on USES=gnome.

Mk/bsd.port.mk
1422–1424 ↗(On Diff #47005)

This should only be a compat shim, so it should only add gl if it is not already in USES. It should also add a DEV_WARNING saying that it is invalid to use USE_GL without USES=gl.

Mk/bsd.port.mk
1422–1424 ↗(On Diff #47005)

Ok, however, USE_GL= is not a compat shim, it's still needed to specify which GL components to bring in. But perhaps I'm misunderstanding what you mean?

Mk/bsd.port.mk
1422–1424 ↗(On Diff #47005)

Thank you for your input, I worded it wrong.

It should only add if USES does not include gl. Then it needs to have a warning like this:

DEV_WARNINGS+=   "Using USE_GL alone is deprecated, please add USES=gl."

Updated patch with a DEV_WARN if USE_GL is set without USES=gl.

zeising added inline comments.
Mk/bsd.port.mk
1422–1424 ↗(On Diff #47005)

Thank you very much for the clarification.

antoine requested changes to this revision.Aug 26 2018, 9:56 PM
antoine added a subscriber: antoine.
antoine added inline comments.
Mk/bsd.port.mk
1424 ↗(On Diff #47303)

This breaks previous behavior (yes used to imply glu, not gl)

This revision now requires changes to proceed.Aug 26 2018, 9:56 PM
mat requested changes to this revision.Aug 27 2018, 10:06 AM
mat added inline comments.
Mk/bsd.port.mk
1423 ↗(On Diff #47303)

The variable is called DEV_WARNING.

Updated with latest round of comments.

Thank you very much for the review!

Mk/Uses/gl.mk
10–11 ↗(On Diff #47454)

s/gl/glu/

34 ↗(On Diff #47454)

This should be glu too.

Mk/Uses/gl.mk
10–11 ↗(On Diff #47454)

Doh. Will fix.

34 ↗(On Diff #47454)

No, this is the case when USES=gl but USE_GL is not defined at all, then we should use gl. The case when USE_GL is yes is handled below, and defaults to glu, just like before.

Mk/Uses/gl.mk
34 ↗(On Diff #47454)

Having

  1. USE_GL=yes -> USE_GL=glu
  2. USE_GL is not defined -> USE_GL=gl

is really really a bad idea. yes and not defined should mean the same thing.

Mk/Uses/gl.mk
34 ↗(On Diff #47454)

I do not agree.
No port today has USES=gl and USE_GL not defined, so there is no backwards compatibility to take into account. I understand USE_GL=yes means glu, that's because it's been that way for a very long time. USES=gl is new, and can imply new things. It would surprise me as a developer that USES=gl actually means i get glu, not gl unless I specifically asked for glu with USE_GL.

I think USE_GL not defined should be forbidden.

Also, the DEV_WARNING should suggest to replace USE_GL=yes with USE_GL=glu

Updated diff with changes based on the latest round of comments.

I elected to make it an error not to specifu USE_GL at all, when USES=gl is set.

Changes to games/q3cellshading to make it compile again.

Mk/Uses/gl.mk
34 ↗(On Diff #47561)

Should say something about having to set USE_GL.

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

Why move this?

Mk/Uses/gl.mk
34 ↗(On Diff #47561)

Noted, will fix.

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

To fix the build. USES need to be defined before bsd.port.pre.mk is included, and the port has USE_GL at line 42, which implies USES. See build failure log here:
http://package23.nyi.freebsd.org/data/111i386-default-PR230692PR230909/2018-08-29_18h58m56s/logs/errors/q3cellshading-1.0_3.log and comment from Antoine here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=230692#c6

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

Yeah, so, you changed the include to be bsd.port.options.mk, I get that, but why move this block, it was fine up there.

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

Because it had to be after the bsd.port.pre.mk include, otherwise it wouldn't build..

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

Just changing the include from bsd.port.pre.mk gives me this:

make: "/usr/ports/games/q3cellshading/Makefile" line 91: Malformed conditional (${COMPILER_TYPE} == clang)
make: "/usr/ports/Mk/bsd.port.mk" line 1834: Malformed conditional (${WITH_PKG} == devel)
make: "/usr/ports/Mk/bsd.port.mk" line 2140: Malformed conditional (${PREFIX} == /usr)
make: "/usr/ports/Mk/bsd.port.mk" line 2658: Malformed conditional (${PREFIX} == /usr)
make: "/usr/ports/Mk/bsd.port.mk" line 2671: Malformed conditional (${PREFIX} == /usr)
make: "/usr/ports/Mk/bsd.port.mk" line 4553: Malformed conditional ((${PREFIX} != ${LOCALBASE} && ${PREFIX} != ${LINUXBASE} && ${PREFIX} != "/usr" && ${PREFIX} != "/" && !defined(NO_PREFIX_RMDIR)))
make: Fatal errors encountered -- cannot continue
make: stopped in /usr/ports/games/q3cellshading

So bsd.port.pre.mk needs to be included before the block, but after the uses stuff, which is in the options block.

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

from bsd.port.pre.mk to bsd.port.options.mk

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

This looks more like a symtom of not changing bsd.port.post.mk to bsd.port.mk at the end for the Makefile. So my point stands.

bsd.port.pre.mk / bsd.port.post.mk must be kept for the ${COMPILER_TYPE} check

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

Even changing the bsd.port.pre.mk to bsd.port.options.mk and bsd.port.post.mk to bsd.port.mk I still get the following error:

make: "/usr/ports/games/q3cellshading/Makefile" line 90: Malformed conditional (${COMPILER_TYPE} == clang)
make: Fatal errors encountered -- cannot continue

bsd.port.pre.mk / bsd.port.post.mk must be kept for the ${COMPILER_TYPE} check

I think my suggested change, with moving the entire block is the only way to solve this.

games/q3cellshading/Makefile
67–82 ↗(On Diff #47561)

Adding bsd.port.pre.mk here is fine, but you do not need to move the rest.

Updated based on latest set of comments.

Restored the ordering in games/q3cellshading/Makefile and just fixed includes
Reworded IGNORE message with USE_GL isn't defined but USES=gl is.

Update comment in Mk/Uses/gl.mk to reflect reality.

What's missing to get this in?

This revision was not accepted when it landed; it landed in state Needs Review.Sep 10 2018, 7:56 PM
This revision was automatically updated to reflect the committed changes.