| tcberner | |
| mat |
| Contributor Reviewers (ports) | |
| portmgr | |
| O5: Ports Framework |
This change makes ports for Haskell applications include all dependencies in
DISTFILES. This brings dependencies on hs-* ports to 0.
Benefits of this:
Ports touched by this commit can be inferred from lang/ghc/bsd.hackage.mk changes.
The diff passes poudriere bulk on FreeBSD 12 amd64.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| Mk/Uses/cabal.mk | ||
|---|---|---|
| 33 ↗ | (On Diff #55665) | Is there any cabal port that actually sets DISTFILES ? |
| 39–60 ↗ | (On Diff #55665) | There are helpers that are for qmake, cmake, meson, they are all in bsd.options.mk. I would prefer that all helpers be defined in one place, instead of having some magically happen when you add a feature. |
It turned out that
. if ${PORT_OPTIONS:M${opt}}
CABAL_FLAGS+= ${${opt}_CABAL_FLAGS}
. else
CABAL_FLAGS+= -${${opt}_CABAL_FLAGS}
. endifconstruction is wrong. When the option is unset, the .else branch doesn't get executed.
What is the proper way to handle option setting?
Ignore my previous comment, it was a mistake on my side. Got lost in nested .ifs.
| Mk/Uses/cabal.mk | ||
|---|---|---|
| 35 ↗ | (On Diff #55764) | This is not right, see Mk/bsd.port.mk lines 5186+ to see what to add to that variable. |
| 106 ↗ | (On Diff #55764) | Remove trailing / |
| 109–117 ↗ | (On Diff #55764) | Please remove this, we already have a standard way for ports to "do" things when they need, do not invent a new one |
| 120 ↗ | (On Diff #55764) | Please do not piggy-back on an existing target, create your own. |
| Mk/Uses/cabal.mk | ||
|---|---|---|
| 35 ↗ | (On Diff #55764) | I saw that, but don't get what's wrong with this line. Should I prepend some integer before the target name? |
> poudriere bulk -j130amd64 -pghc -tC */hs* [...] [00:00:08] Warning: www/hs-yesod dependency on textproc/hs-yaml has wrong PKGNAME of 'hs-yaml' but should be 'yaml' [00:00:08] Warning: www/hs-yesod-static dependency on www/hs-wai-app-static has wrong PKGNAME of 'hs-wai-app-static' but should be 'wai-app-static' [...]
| devel/hs-happy/Makefile | ||
|---|---|---|
| 17 ↗ | (On Diff #56159) | Wrong place in the Makefile. Should happen before OPTIONS_DEFINE. See Chapter 15. Order of Variables in Port Makefiles. |
| devel/hs-hspec/Makefile | ||
| 16 ↗ | (On Diff #56159) | Shouldn't this just be removed? |
| devel/hs-hspec/Makefile | ||
|---|---|---|
| 16 ↗ | (On Diff #56159) | Sure, I will remove it when landing this. |
| Mk/Uses/cabal.mk | ||
|---|---|---|
| 10–11 ↗ | (On Diff #56342) | You should check if cabal_ARGS is empty and set IGNORE= when it is like other USES to prevent someone from adding bogus USES=cabal:foobar things. |
| 99–101 ↗ | (On Diff #56342) | It might be good to pass through MAKE_ENV too. This is important for stuff like BINARY_ALIAS etc. to work correctly. Ports might want to overwrite do-build, so maybe guard it with .if !target(do-build). Same for do-install. |
| Mk/bsd.options.mk | ||
| 519–597 ↗ | (On Diff #56342) | The new options helpers need to be documented in the header and added to _OPTIONS_FLAGS/_ALL_OPTIONS_HELPERS too. |
| 522–524 ↗ | (On Diff #56342) | Is this really needed when we already have the opt_USE helper? |
| devel/hs-darcs/Makefile | ||
| 28–45 ↗ | (On Diff #56342) | These are in the wrong place now. See 15. Order of Variables in Port Makefiles. |
| 47 ↗ | (On Diff #56342) | What does MAN1PAGES do? With bsd.cabal.mk gone it does not seem to do anything. |
| Mk/bsd.options.mk | ||
|---|---|---|
| 519–597 ↗ | (On Diff #56342) | Not sure about last part. ${opt}_CABAL_FLAGS works differently from other opt helpers - it appends some value in any case, so it actually handles _OFF case too. As for ${opt}_USE_CABAL and ${opt}_EXECUTABLES - they don't need any handling for _OFF case. |
| 522–524 ↗ | (On Diff #56342) | It is not, but I'm hesistant to change that now. Moreover, I find opt_USE to look untidy when there are many items and you have to list them separated by , instead of . |
| Mk/bsd.options.mk | ||
|---|---|---|
| 519–597 ↗ | (On Diff #56342) | Adding them to just _ALL_OPTIONS_HELPERS directly should be fine then. |
| Mk/bsd.options.mk | ||
|---|---|---|
| 519–597 ↗ | (On Diff #56342) | grep -r _ALL_OPTIONS_HELPERS Mk yields nothing. What's _ALL_OPTIONS_HELPERS and where is it located? |
| Mk/Uses/cabal.mk | ||
|---|---|---|
| 141–146 ↗ | (On Diff #56590) | Put the .if !defined(SKIP_CABAL_PLIST) around the target, no need to create an empty target. |
| Mk/bsd.options.mk | ||
| 531–533 ↗ | (On Diff #56590) | Note that this is not needed, you already have <opt>_USE= cabal=foo. |
| 534–536 ↗ | (On Diff #56590) | If all you are doing is: .if defined(<opt>_FOO) FOO+= <opt>_FOO .endif all you need to do is add FOO to _OPTIONS_FLAGS. |
| Mk/bsd.options.mk | ||
|---|---|---|
| 531–533 ↗ | (On Diff #56590) | Please, let's leave these 3 lines as they are. While opt_USE= xorg:xft,xwhatever looks nice, because there aren't many arguments for USE_XORG exists, the opt_USE= cabal:foo-1.2.3_4,bar-5.6.7_8,baz-9.10.11_12,... is awful, IMO. And these lines can be pretty large, in case some option pulls a bunch of Haskell packages. |
Other than my last small comment, the part in Mk looks ok to me.
| x11/hs-xmobar/pkg-plist | ||
|---|---|---|
| 2 ↗ | (On Diff #57118) | empty line. (only noticed it because it's the last file in the review, make sure there are no other.) |
| x11/hs-xmobar/pkg-plist | ||
|---|---|---|
| 1 ↗ | (On Diff #57168) | Note that this file can be replaced by having: PORTEXAMPLES= xmobar.config in the port's Makefile. |
@tcberner , @mat I missed another port that was using Haskell directly - games/hedgewars. I split off Haskell part into separate port and had to make some more changes to cabal.mk. Please, review these changes.
@AMDmi3 , you're maintainer for games/hedgewars. Please also take a look at my changes to it and new games/hedgewars-server port.
Looks good to me.
| games/hedgewars/Makefile | ||
|---|---|---|
| 54 ↗ | (On Diff #57196) | CMAKE_ON=NOSERVER is nicer |