Hi Alessandro --
On 8/4/18 3:04 AM, Alessandro DE LAURENZIS wrote:
> Hi Stuart,
>
> On 08/03/18 17:56, Alessandro DE LAURENZIS wrote:
>> Hi Stuart,
>>
>> On 08/03/18 13:21, Stuart Henderson wrote:
>>
>>> On 2018/08/03 12:23, Alessandro DE LAURENZIS wrote:
>>>> I think we need the line:
>>>>
>>>> FAKE_FLAGS = PREFIX="${LOCALBASE}"
>>>>
>>>> since PREFIX is explicitly set to /usr/local in upstream Makefile when
>>>> undefined.
>>>
>>> That should probably be PREFIX="${TRUEPREFIX}"
>>
>> Fixed
>>
>>
>>>> Finally, portcheck gives the following message:
>>>>
>>>> Python module without compiled version, consider using ${MODPY_BIN}
>>>> ${MODPY_LIBDIR}/compileall.py: share/yosys/python3/smtio.py
>>>> cad/yosys
>>>>
>>>> I don't know if it's acceptable (and, if not, how to remove it...)
>>>
>>> Python normally creates pyc files for imported modules if the directory
>>> is writable. We pre-create these so that if e.g. someone runs the
>>> program
>>> as root, we don't get stray pyc files left behind after uninstall.
>>> You can remove it by using ${MODPY_BIN} ${MODPY_LIBDIR}/compileall.py
>>> to create the pyc file.
>>
>> Added it in post-install target:
>>
>> post-install:
>> ${MODPY_BIN} ${MODPY_LIBDIR}/compileall.py \
>> ${PREFIX}/share/yosys/python3
>>
>>
>>>> - compiler name forced to eg++;
>>>
>>> Don't hardcode this, use MAKE_FLAGS= CXX="${CXX}" etc.
>>
>> Fixed
>>
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.
>>
>>>> Different story for kernel/yosys.cc:
>>>> - we need to include sys/wait.h;
>>>> - code requires the process executable base path; Linux has
>>>> /proc/self/exe
>>>> and I see that there are solutions for APPLE and WIN32 too; OpenBSD
>>>> doesn't
>>>> have a ready-to-use function; this is the closest thing that comes
>>>> to my
>>>> mind:
>>>>
>>>> std::string proc_self_dirname()
>>>> {
>>>> // No direct way to get the process executable base path
>>>> char path[PATH_MAX];
>>>> ssize_t buflen = sizeof(path);
>>>> char *res = realpath(getenv("_"), path);
>>>> if (!res) {
>>>> log_error("getenv(\"_\") failed: %s\n",strerror(errno));
>>>> }
>>>> *(strrchr(path, '/')+1) = '\0';
>>>> while (buflen > 0 && path[buflen-1] != '/')
>>>> buflen--;
>>>> return std::string(path, buflen);
>>>> }
>>>>
>>>> If you think that's acceptable, I'll submit the patch upstream.
>>>
>>> IMO hardcoding the path is the most sensible way for ports.
>>
>> As discussed (thanks for your explanation), I patched to use
>> ${PREFIX} and then added:
>>
>> pre-build:
>> ${SUBST_CMD} ${WRKSRC}/kernel/yosys.cc
>>
>>
>>>> pdflatex is used to compile manual, application notes and
>>>> presentations
>>>> during build; it's part of print/texlive_base, which is a large
>>>> port; should
>>>> we stay with it or do you have alternatives to suggest?
>>>
>>> That's not really a problem. If it needs pdflatex, list the dependency.
>>> >> pdflatex is used to compile manual, application notes and
>>> presentations
>>>> during build; it's part of print/texlive_base, which is a large
>>>> port; should
>>>> we stay with it or do you have alternatives to suggest?
>>>
>>> That's not really a problem. If it needs pdflatex, list the dependency.
>>
>> Actually I noticed that the corresponding target isn't run by
>> default. Should I force it? I mean, is the extra documentation
>> normally installed in other ports?
>>
>> New tarball attached.
>>
>
> There is a local copy of MiniSat in ./libs, so I tweaked a bit the
> license markers:
>
> # ISC (yosys), MIT (MiniSat)
>
> New tarball attached.
>
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.
Your LDFLAGS line might be better located as a part of your MAKE_FLAGS line.
That's all for now from a quick read; I'll try to test it later today.
~Brian
No comments:
Post a Comment