ghdl: use DESTDIR instead of --prefix

[umarcor]

Currently, compile_ghdl.sh uses --prefix for modifying the location where make install places the artifacts. However, configuring GHDL with a custom prefix has other effects. It is hardcoded in the tool, and it is later used for searching libs. As a result, it can produce errors with weird paths, such as https://github.com/umarcor/fomu-workshop/runs/1243053586?check_suite_focus=true#step:9:46.

In this PR, the prefix is not modified. Instead, DESTDIR is used for telling make install where to put the content.

Ref: https://github.com/ghdl/ghdl/blob/master/dist/ci-run.sh#L317

[umarcor]

The default prefix of GHDL is /usr/local. However, fpga-toolchain expects tools to be moved to /. I updated the PR for setting --prefix=/ and using DESTDIR.

[edbordin]

If you merge the latest from the main branch that CI error should be fixed now - it was unrelated to this.

Will this make it so that GHDL_PREFIX doesn't need to be set? if so, please remove this line from the test scripts so we can verify https://github.com/open-tool-forge/fpga-toolchain/blob/071ccd2/scripts/test/install_toolchain.sh#L40

[edbordin]

Also, the goal is to have the toolchain set up so hopefully it can be run from any prefix, not just from /. I think I would actually prefer to continue having the wrong absolute path configured than setting / because it makes it less likely to clash with a system-wide installation of any of the tools.

edit: See here for some more details I found about the ghdl search path #4 (comment). In summary, there is a list of search paths it goes through including relative to the binary. It works correctly on windows, for some reason it's broken on the other platforms

[umarcor]

The motivation for using / is that the build scripts in this repo expect tools to be $PACKAGE_DIR/$NAME/bin. However, using the default prefix, that should be $PACKAGE_DIR/$NAME/usr/local/bin. Then, instead of packaging $PACKAGE_DIR/$NAME, you would package $PACKAGE_DIR/$NAME/usr/local.

However, that would not solve the issue with GHDL_PREFIX. This PR is just because "hardcoding" a path in the tools which is unlikely to exist feels weird. I do understand why you don't want to use /. By the same token, you probably don't want to use /usr/local, as it might also conflict with other tools.

What do you think about using /opt/fpga-toolchain/ as a prefix? Then, users would have two options:

• Extract the package to /opt/fpga-toolchain.
• Set GHDL_PREFIX.

edit: See here for some more details I found about the ghdl search path #4 (comment). In summary, there is a list of search paths it goes through including relative to the binary. It works correctly on windows, for some reason it's broken on the other platforms

According to @tgingold (im-tomu/fomu-workshop#334 (comment)) those search paths are for the GHDL binary, not for libghdl or the plugin. Therefore, it is surprising that it works on Windows. However, I removed my MSYS2 installation of GHDL, and I can confirm that fpga-toolchain works ok without it.

[edbordin]

The motivation for using / is that the build scripts in this repo expect tools to be $PACKAGE_DIR/$NAME/bin. However, using the default prefix, that should be $PACKAGE_DIR/$NAME/usr/local/bin

I'm not sure what you mean. When compiling ghdl and libghdl, the script runs ./configure --prefix=$PACKAGE_DIR/$NAME and then make install. The result is an executable installed at $PACKAGE_DIR/$NAME/bin/ghdl. At runtime, this prefix path from CI is invalid so it skips over it when searching for vhdl libraries.

According to @tgingold (im-tomu/fomu-workshop#334 (comment)) those search paths are for the GHDL binary, not for libghdl or the plugin.

For ghdl-yosys-plugin (or libghdl), the library path depends on how it was configured.

As far as I can tell, what happens on windows is that it looks in the configured search paths which are wrong so then it looks relative to argv[0] which is the yosys binary rather than the ghdl binary. That works well (even if it's maybe unintended). For some reason that same API call returns NULL/empty string for argv[0] on Linux and OS X. I don't know if @tgingold would be willing to offer any insights?

What do you think about using /opt/fpga-toolchain/ as a prefix?

I think the behaviour on Windows is the ideal situation but given I wasn't able to replicate it on Linux and OS X that sounds like a good compromise. It seems less likely to clash with existing installations. Basically I just really want to make sure it doesn't end up loading vhdl libraries other than the ones bundled into this package.

[umarcor]

I'm not sure what you mean. When compiling ghdl and libghdl, the script runs ./configure --prefix=$PACKAGE_DIR/$NAME and then make install. The result is an executable installed at $PACKAGE_DIR/$NAME/bin/ghdl. At runtime, this prefix path from CI is invalid so it skips over it when searching for vhdl libraries.

My first change was:

./configure
...
make DESTDIR=$PACKAGE_DIR/$NAME install

But that placed the GHDL binary in $PACKAGE_DIR/$NAME/usr/local/bin/ghdl (because the default prefix is /usr/local). That's why I changed it to:

./configure --prefix=/
...
make DESTDIR=$PACKAGE_DIR/$NAME install

Which is the current state of this PR. However, I agree it might produce conflicts.

As far as I can tell, what happens on windows is that it looks in the configured search paths which are wrong so then it looks relative to argv[0] which is the yosys binary rather than the ghdl binary. That works well (even if it's maybe unintended). For some reason that same API call returns NULL/empty string for argv[0] on Linux and OS X. I don't know if @tgingold would be willing to offer any insights?

That's a very interesting insight. Yes, let's wait for Tristan.

What do you think about using /opt/fpga-toolchain/ as a prefix?

I think the behaviour on Windows is the ideal situation but given I wasn't able to replicate it on Linux and OS X that sounds like a good compromise. It seems less likely to clash with existing installations. Basically I just really want to make sure it doesn't end up loading vhdl libraries other than the ones bundled into this package.

Let's see whether Tristan considers that behaviour (taking the location of the yosys binary) acceptable. If so, there is nothing to fix. Otherwise, we can go with /opt/fpga-toolchain/.

A issue I see with that behaviour is that it might be problematic when the plugin is loaded dynamically. yosys -m ghdl.so allows the plugin to be located anywhere. Hence, it might need to be enabled in the patch only.

[edbordin]

I'm manually re-running the CI build thanks to this bug >_< #61
(It will likely not matter anyway if we change that prefix and the CI build gets triggered again)

[tgingold]

As far as I can tell, what happens on windows is that it looks in the configured search paths which are wrong so then it looks relative to argv[0] which is the yosys binary rather than the ghdl binary. That works well (even if it's maybe unintended). For some reason that same API call returns NULL/empty string for argv[0] on Linux and OS X. I don't know if @tgingold would be willing to offer any insights?

A different API is used on Windows to get the process path. Maybe it is not a good idea for libghdl.

[edbordin]

A different API is used on Windows to get the process path.

Ah, that explains the different behaviour then! I think I found the code you're talking about here https://github.com/ghdl/ghdl/blob/657fcfde5fb93c1311fe5fd2d28146c89852614d/dist/windows/mcode/windows_default_path.adb#L44

[umarcor]

So, @tgingold, do you suggest that we enforce using GHDL_PREFIX or installing the package to the location used as the build time prefix?

[edbordin]

Thinking about it more, I'd really prefer a solution that makes it simple to have multiple nightly builds installed at once without risk of "cross-contaminating" them by relying on an absolute search path.

A yosys wrapper script like this would probably do the trick (except on systems without bash installed):

#!/usr/bin/env bash

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
# set GHDL_PREFIX relative to this script, unless it has already been set
export GHDL_PREFIX="${GHDL_PREFIX:-${DIR}/../lib/ghdl}"

${DIR}/_yosys "$@"

(the yosys binary would be renamed to _yosys)

edit: we would probably need a similar wrapper script for ghdl too for consistency

[tgingold]

What about modifying the ghdl-yosys-plugin to use the same prefix as Yosys ? That would avoid the need of the wrapper script.

[edbordin]

What about modifying the ghdl-yosys-plugin to use the same prefix as Yosys ? That would avoid the need of the wrapper script.

That sounds like a great solution!

[umarcor]

Agree, that'd be ideal, as it would make GHDL_PREFIX required only in specific development contexts.

@edbordin if a wrapper was used, I believe it'd be easier to use fpga-tool as a single wrapper for all the supported features. In some Makefiles, the approach for using Docker is prepending command names. See https://github.com/ghdl/ghdl-yosys-plugin/blob/master/examples/docker.mk. A similar solution might work for using fpga-toolchain.

Nevertheless, if this issue is fixed upstream, the only motivation for maintaining this PR is providing nicer errors. Shall I close it or are you ok with using /opt/fpga-toolchain as a prefix for cosmetic purposes?

[edbordin]

@umarcor I think I'm happy to merge it with /opt/fpga-toolchain as a prefix if this is something you want. Unsure if the error messages would be better fixed upstream in yosys... fwiw I expanded the instructions for using ghdl-yosys-plugin while I was working on the documentation yesterday so that it explains the meaning of that error and how to fix it.

[umarcor]

@edbordin, let's deal with #62 first. This is currently quite specific to GHDL. We might slightly change our minds as a result of that. Overall, I agree that this toolchain should allow different versions to coexist.