Page MenuHomeFreeBSD

Mk/Uses/magick.mk: Fix DEFAULT_VERSION handling
ClosedPublic

Authored by zirias on Apr 5 2023, 9:48 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Unknown Object (File)
Sat, May 11, 5:36 AM
Subscribers

Details

Summary

graphics/ImageMagick7: Convert -nox11 to flavor
graphics/ImageMagick6: Convert -nox11 to flavor

Handle fallback for version and flavor separately, also add several
sanity checks.

This fixes creating broken *_DEPENDS for ports using it without
arguments when a -nox11 version is requested in DEFAULT_VERSIONS like
e.g. this resulting from "imagemagick=7-nox11":
libMagick++-7-nox11.so:graphics/ImageMagick7-nox11

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

zirias requested review of this revision.Apr 5 2023, 9:48 AM

Make inline docs in magick.mk a bit more consistent and explicit.

Use FLAVORS_SUB helper, remove forgotten X11 option in ImageMagick6.

I am currently running some poudriere builds to test this before I accept. In the mean time, added some comment/documentation suggestions.

Mk/Uses/magick.mk
80

Take opportunity to correct spelling.

Mk/bsd.default-versions.mk
65

Might be useful to provide an example to the user, so they know they can explicitly specify the x11 flavor (if they want).

Mk/Uses/magick.mk
80

He, certainly, will make sure to include this! I didn't even notice ... ;)

Mk/bsd.default-versions.mk
65

I wouldn't "overdo" it here, IMHO the comments are a bit excessive already (but necessary because selection of a flavor is "special" in this context ....)

Mk/bsd.default-versions.mk
65

That's fair. The explicit examples can be enumerated in the porters handbook 17.60 section.

Looks great. Works for my tests. Thanks for doing this!

Mk/Uses/magick.mk
80

please just commit this type fix separately now :)

Typo fix committed, rebased the other commits for review.

Fix package name for dependencies after flavorizing.

Bump ImageMagick ports, document in CHANGES

here are some more non-technical nit-picks, before I finally have time to really look at it :)

MOVED
17912 ↗(On Diff #120397)

^ you need to remember to update the date before pushing

Mk/Uses/magick.mk
8

^ maybe you could ammend that 'default' in all three cases is controlled by what one specifies in default_versions?

Mk/bsd.default-versions.mk
64

^it might be sensible to mention that the "default" flavor is x11.

zirias added inline comments.
MOVED
17912 ↗(On Diff #120397)

Already done multiple times ^^

Mk/Uses/magick.mk
8

Not sure which *three* cases you mean?

Mk/bsd.default-versions.mk
64

It's really a challenge getting these comments concise *and* correct ;) Will have a look ...

@zirias What about net/freeswitch? It seems it used to have a direct library dependency, but this commit removed it, without correctly replacing it. Should it have something like:

X11_USES= magick:x11,lib
X11_OFF_USES = magick:nox11,lib

Or would it be better to suggest relabeling the "X11" option for that port to instead be for ImageMagick (since it only previously influenced ImageMagick), and upstream appears to accept either version 6 or 7, and simply put a MAGICK_USES= magick directive in?

Looks to me like this port should not have an ImageMagick dependency at all, because the option that once required it was removed. So, all fine there.

Except, the X11 option, if still present, might be bogus, this should be checked!

In general, looking for X11 options that only existed for selecting a flavor of ImageMagick is a next planned step for me once this review here is done. They won't be needed any more, a user should just set DEFAULT_VERSIONS accordingly now.

@zirias Perhaps - but upstream has apparently an optional dependency on imagemagick...

Or is there an implied dependency on ImageMagick from one of the other "USES" directives?

@zirias Perhaps - but upstream has apparently an optional dependency on imagemagick...

Sure it has, but the port had the option removed that required it (for whatever reason, you'd probably have to ask its maintainer), so no need to add that dependency to the port.

Or is there an implied dependency on ImageMagick from one of the other "USES" directives?

No, dependencies should never be "implied", always explicitly listed.

CHANGES
27

@zirias There should also be a warning to users that building ImageMagick7@nox11 will use the previously set/stored options for ImageMagick7, NOT the stored options in ImageMagick7-nox11. Users migrating from the -nox11 slave ports should be alerted to the need to verify the desired options are set in the formerly master ports.

zirias added inline comments.
CHANGES
27

This would be something for UPDATING, not for CHANGES. I'm not sure it's really worth mentioning as it's kind of implicit from the entries in MOVED, but let's see what Tobias thinks!

This looks sensible to me (and I assume/know it is well tested by you). Please make sure to bump ports depending on magick, so that the new flavored dependency is certainly picked up.

Feel free to also convert the consumers of the tree that still use options (that is covered by a portmgr blanket on modernizationy-stuff)

Mk/Uses/magick.mk
8

I meant two:

  • version
  • flavor

:D

This revision is now accepted and ready to land.Apr 28 2023, 10:04 AM
This revision was automatically updated to reflect the committed changes.
zirias marked an inline comment as done.