Re: [NEW/WIP] Qflow porting // [3/7] graywolf

Hi Alessandro --

On 08/11/18 13:36, Alessandro DE LAURENZIS wrote:
> Ciao Brian,
>
> thanks for your prompt feedback.
>
> On 08/11/18 16:41, Brian Callahan wrote:
> [...]
>> I would try with the latest version sooner rather than later.
>
> I'll do that soon, I promise :-) The problem is that Qflow is... well,
> a flow; and changing the version of one of the components requires
> double checking the consistency of the entire sequence of steps, using
> some test-cases; this takes a while.
>
> [...]
>> In patches/patch_CMakeLists_txt, you don't want to hardcode
>> /usr/X11R6 -- instead you should use ${OPENBSD_X11BASE}, which CMake
>> understands.
>
> Done
>
>>> - adding "-Wno-return-type" to CFLAGS (otherwise the code doesn't
>>> compile correctly with clang);
>>>
>>
>> And I think more is warranted. This is really really not C99 (which
>> clang defaults to), I would add
>> # Really really not C99
>> CONFIGURE_ARGS =        -DCMAKE_C_FLAGS="${CFLAGS} -std=gnu89"
>>
>> to give people a fighting chance to read the other warnings. I
>> noticed some -Wformat warnings that look like they could matter with
>> this.
>
> Done; I put there "-Wno-return-type" too; this is more readable
> (instead of jumping between the Makefile and the patch); what do you
> think?
>

I'm ok with this. You maybe want to work with upstream to fix it so you
don't need to silence the warnings, though :)

> I would be tempted to use -D variable assignments also for
> include_directories and link_directories, if possible; but I do not
> know if such variables exists... I tried with CMAKE_INCLUDE_PATH,
> without success. Let me know.
>
You can add the include directories right into -DCMAKE_C_FLAGS. If
you're going to put it into the port Makefile, it'll look like
-I${X11BASE}/include. For the linker flags
-DCMAKE_EXE_LINKER_FLAGS="${LDFLAGS} -L${X11BASE}/lib ..." (and maybe
-DCMAKE_SHARED_LINKER_FLAGS="${LDFLAGS} -L${X11BASE}/lib ..." for the
shared library if necessary).

>>> - removing a symbolic link required after the install (I moved it in
>>> the post-install target, since doing it the way it was doesn't work,
>>> even if I didn't fully understand why... please comment!)
>
> What about this point? Is it ok?
>

If it works, sure.

~Brian

> [...]
>> Other stuff:
>> You don't need HOMEPAGE or DISTNAME, the GH_* variables do that for
>> you in this case.
>
> Removed
>
>> Please put a newline between WANTLIB and MODULES.
>>
>> Pull the SHARED_LIBS line up to where it belongs.
>>
>> License is GPLv2+.
>
> Fixed
>
> Attached please find the updated tarball.
>  --
> Alessandro DE LAURENZIS
> [mailto:just22@atlantide.t28.net]
> Web: http://www.atlantide.t28.net
> LinkedIn: http://it.linkedin.com/in/delaurenzis