On 2023/09/23 13:32:49 -0400, Thomas Frohwein <[email protected]> wrote:
> Hi,
>
> Took some time to overhaul this and I have a middle-ground proposition:
> Build on all arches, not just MONO_ARCHS, but only on MONO_ARCHS build
> with mono bits and only there install the -sharp subpackage.
I haven't yet managed to do the split in flavors I anticipated, sorry :(
> Tested the build and packaging for both scenarios on my amd64 (by
> tweaking the IS_MONO_ARCH test if you want to know).
>
> This seems to be a bit unconventional use of bsd.port.arch.mk, let me
> know if I'm overlooking something.
>
> Smaller adjustments with comments inline and then the updated diff...
I have only one real comment about the diff, see below, but I'm
fearing we're over-engineering this. I have still a few "complains"
generally:
- NOBTCFI / WXNEEDED are set also for the main package that shouldn't
need it.
- we're really reimplementing flavors over MULTI_PACKAGES needlessly
complicating the Makefile.
However, depending on how much time there is before the freeze we
might commit this to have mono support in 7.4. don't know.
> On Sat, Sep 02, 2023 at 07:04:29PM +0200, Omar Polo wrote:
>
> [...]
>
> > Would be too hard to turn this into a flavor? Then in the future we
> > might eventually also move -tools to a flavor, and build a set of
> > packages:
> >
> > godot (this would be the current -main)
> > godot-tools (this would be the current -tools)
> > godot-mono
> > godot-mono-tools
> >
> > just an idea. I'm fine with your current diff as is if it simplify
> > future work, we can iterate in-tree :)
>
> Now the packages would look like this:
>
> MONO_ARCHS:
> godot
> godot-tools
> godot-sharp
>
> !MONO_ARCHS:
> godot
> godot-tools
>
> > > post-extract:
> > > + @# install backends from FILESDIR
> >
> > first time i'm seeing @ added in front of a comment ^^"
> >
> > I know (at least) Emacs syntax highlighting complains, but just
> > # ... should be enough.
>
> I removed the @ in my updated diff; wouldn't want to get emacs angry...
Just for clarity, let me explain better my point. In Emacs (and maybe
other editors)
foo:
# do x y z...
cmd...
the '# do x y z...' line will be highlighted in red (which I assume
it's bad). Your trick to prepend '@' silences this warning, but I
don't think we need to use '@' with a comment.
> The reason for the @ was that the comment is only there to make it
> easier for anyone working with the Makefile to understand the steps.
I'm actually happy to have those comments, makes understanding why
we're doing those things simpler since we have a few things to move
and adjust.
> [...]
> MASTER_SITES = https://downloads.tuxfamily.org/godotengine/${V}/
> -MASTER_SITES0 =
> https://github.com/CoaguCo-Industries/GodotSteam/archive/refs/tags/
> -DISTFILES = ${DISTNAME}${EXTRACT_SUFX} \
> - ${GODOTSTEAM_V}.tar.gz:0
> +DISTFILES = ${DISTNAME}${EXTRACT_SUFX}
> EXTRACT_SUFX = .tar.xz
> +
> +SITES.sharp = https://thfr.info/distfiles/
> +DISTFILES.sharp = godot-${V}-mono-glue.tar.gz
> godot-${V}-nuget-packages.tar.xz
> +
> +DIST_TUPLE += github CoaguCo-Industries GodotSteam v3.20 godotsteam #
> MIT
> +
+1 for this SITES.sharp and DIST_TUPLE usage!
> DIST_SUBDIR = ${PKGNAME}
>
> MODULES = devel/scons
>
> -# Building with module_mono_enabled requires msbuild and to fix the
> -# sharedlib_ext in modules/mono/config.py to '.so.1.0'
> MODSCONS_FLAGS = CC="${CC}" \
> CXX="${CXX}" \
> CFLAGS="${CFLAGS}" \
> @@ -93,15 +95,57 @@ LIB_DEPENDS = archivers/zstd \
> multimedia/libvpx \
> net/enet \
> security/polarssl
> -
nit: if possible, I'd like to keep this newline. I find easier to
visually parse these long lists when they're separated by one blank.
> RUN_DEPENDS-tools = devel/desktop-file-utils
>
> -DEBUG_PACKAGES = ${BUILD_PACKAGES}
> -NO_TEST = Yes
> -
> DPB_PROPERTIES = parallel
>
> .include <bsd.port.arch.mk>
> +
> +IS_MONO_ARCH=
> +.for _arch in ${MONO_ARCHS}
> +. if ${MACHINE_ARCH} == ${_arch}
> +IS_MONO_ARCH = 1
> +. endif
> +.endfor
While this reads a bit strange, I don't have a better suggestion. At
first, I tohught of
.if ${MONO_ARCHS:M${MACHINE_ARCH}}
but I guess that make doesn't substitute MACHINE_ARCH in there (even
if make 'show=MONO_ARCHS:${MACHINE_ARCHS}' seems to do)
> +.if !empty(IS_MONO_ARCH)
> +USE_WXNEEDED = Yes
> +USE_NOBTCFI = Yes
> +MONOSGEN != /bin/ls -1 /usr/local/lib/libmonosgen*.so.* | tail -1
This has to be fixed, as it breaks the port if mono is not installed.
if you disinstall mono, issue a 'make clean' and then a 'make build'
it fails. actually, even 'make clean' without mono should fail.
haven't tested if we can get away with some lazy points using !!= since...
> +MONOSGEN_V = ${MONOSGEN:C/.*\.so//}
we substitute the value here
> [...]
> +SUBST_VARS += MONOSGEN_V
for SUBST_VARS usage.
Or maybe it's better to do this in python in the patch. Given that
Godot 4 moved to .NET the mono bit shouldn't change much anymore.
With the MONOSGEN_V thing fixed it's OK op@ to go ahead with this,
even if I would have preferred to move to flavors first. I'm looking
into this right now.
(even if we move to flavors your patch should still -except for the
Makefile and PLISTs- apply and be fine. But then the makefile changes
will be smaller and easier to write.)
Thanks!
Omar Polo
