On 8/10/2018 12:05 AM, Alessandro DE LAURENZIS wrote:
> Hi Brian,
>
> [...]
>> Could you explain why ports-gcc is needed? Is there a reason that
>> gcc-4.9.4 would be preferred over clang-6.0.0?
>> Usually, if you have a COMPILER line, you have a comment above it
>> with a reason why. I don't know if it's true for this port, but
>> something like:
>> # C++11
>> COMPILER = base-clang ports-gcc
>>
>> is usually what is wanted here.
>
> Actually, when compiled with base-clang, I was observing some
> segmentation faults during tests (the first in test/fsm no. 43).
>
> After the upgrade to a more recent snapshot (I'm currently using the
> one dated 4th Aug), the test suite completes without errors even using
> clang. So, I agree with your suggestion.
>
Regardless, if you have a COMPILER line, you should leave a comment
saying why. There's no way someone would know there were problems unless
you tell us.
> Please also note that we need a pre-build "conditional", since the
> "config-" target in upstream Makefile is different for clang
> (config-clang) and gcc (config-gcc-4.8); I defined the variable
> CONFIG_TARGET and set it after the ".include <bsd.port.mk>", according
> to the value of CHOSEN_COMPILER:
>
> [... snip ...]
> pre-build:
> ${SUBST_CMD} ${WRKSRC}/kernel/yosys.cc
> @cd ${WRKBUILD} && exec ${MAKE_PROGRAM} config-$(CONFIG_TARGET)
>
> [...]
>
> .include <bsd.port.mk>
>
> .if ${CHOSEN_COMPILER} == "base-clang"
> CONFIG_TARGET = clang
> .else
> CONFIG_TARGET = gcc-4.8
> .endif
> [... snip ...]
>
> Please confirm that this is acceptable (or suggest any wiser
> alternatives).
>
It's late, so I'll have to revisit this bit in the morning, but there
are examples where we simply just pick one single config target (I think
in my stuff I always pick clang and massage it to also work with gcc).
I'll find you a more concrete example of this tomorrow.
>
> [...]
>> Some other notes:
>>
>> As I understand Makefile.template, you should prefer the GH_*
>> variables for this port, since everything is auto-generated anyway.
>> It will also let you get rid of your VERSION variable, since the GH_*
>> variables will take care of it.
>
> I was following the comment in Makefile.template:
>
> [... snip ...]
> #
> # github:
> # /releases/ -> preferred. ignore GH_*, just use MASTER_SITES and
> DISTNAME.
> # /archive/ -> GH_ACCOUNT and GH_PROJECT, plus either GH_TAGNAME or
> GH_COMMIT.
> #
> [... snip ...]
>
> (in this case we have a release). I see a bunch of other ports doing
> similar things (e.g., games/minetest, textproc/py-rdflib, ...).
>
Huh? You don't have a release. Your MASTER_SITES line clearly has
/archive/ in it. Github is needlessly confusing, and it is possible that
comment could be worded better because I can see where the confusion
comes from.
> Should I use GH_TAGNAME instead?
>
In this case, yes.
>> Your LDFLAGS line might be better located as a part of your
>> MAKE_FLAGS line.
>
> Done.
>
> Updated tarball enclosed.
>
Thanks. I'll take a closer look at this tomorrow. But at a quick glance,
if we decide to keep it, that conditional at the very bottom is going to
have to be pulled up above pre-build.
~Brian
No comments:
Post a Comment