ghdl / ghdl

VHDL 2008/93/87 simulator
GNU General Public License v2.0
2.39k stars 363 forks source link

Possible hardcoded path in libghdl*.so #844

Closed eine closed 5 years ago

eine commented 5 years ago

The following Dockerfile builds and installs GHDL and the ghdl-language-server VSC extension; and installs libghdl and ghdl-ls. It produces a docker image which VSC can attach to through the Remote - Containers extension. As a result, it allows to easily test/use all the tools.

FROM "ghdl/build:ls-ubuntu" AS build

RUN mkdir /tmp/ghdl-dist

# Build GHDL and libghdl

RUN mkdir -p /tmp/ghdl && cd /tmp/ghdl \
 && curl -fsSL https://codeload.github.com/1138-4EB/ghdl/tar.gz/feat-libghdl_pkg | tar xzf - --strip-components=1 \
 && CONFIG_OPTS="--default-pic" ./dist/travis/build.sh -b llvm-6.0 -p ghdl-llvm-fPIC --enable-python \
 && mv ghdl-llvm-fPIC.tgz /tmp/ghdl-dist/ \
 && mv libghdl-py.tgz /tmp/ghdl-dist/

# Build VCS extension

RUN mkdir -p /tmp/vscode-repo && cd /tmp/vscode-repo \
 && curl -fsSL https://codeload.github.com/1138-4EB/ghdl-language-server/tar.gz/reorganize | tar xzf - --strip-components=2 ghdl-language-server-reorganize/vscode-client \
 && npm install \
 && vsce package \
 && mv $(ls vhdl-lsp-*.vsix) /tmp/ghdl-dist/

# In a separate container/image

FROM "ghdl/run:ls-ubuntu" AS run

# Copy ghdl-llvm-fPIC.tgz, libghdl-py.tgz and vhdl-lsp-*.vsix

COPY --from=build /tmp/ghdl-dist /tmp/dist

# Install libghdl and ghdl-ls

RUN cd /tmp/dist \
 && tar -xzf ghdl-llvm-fPIC.tgz -C /usr/local \
 && pip3 install libghdl-py.tgz \
 && mkdir -p /tmp/ghdl-ls && cd /tmp/ghdl-ls \
 && curl -fsSL https://codeload.github.com/1138-4EB/ghdl-language-server/tar.gz/reorganize | tar xzf - --strip-components=2 ghdl-language-server-reorganize/ghdl-ls \
 && pip3 install . \
 && mv tests/files /tmp/dist/ \
 && cd /tmp \
 && rm -rf ghdl-ls \
 && mkdir -p ghdl/install-llvm-6.0 \
 && tar -xzf dist/ghdl-llvm-fPIC.tgz -C ghdl/install-llvm-6.0

Note that in the second container GHDL is installed in /tmp/ghdl/install-llvm-6.0, which is the prefix that was used when GHDL was built (see first container). This should not be required, because libghdl is independent from GHDL for now. However, if GHDL is not available at /tmp/ghdl/install-llvm-6.0, the VSC extension fails:

[Info  - 01:51:47] Connection to server got closed. Server will restart.
Args: ['/usr/local/bin/ghdl-ls', '-v']
Current directory: /tmp/dist
2019-06-16 01:51:47,192 [INFO] Args: ['/usr/local/bin/ghdl-ls', '-v']
2019-06-16 01:51:47,192 [INFO] Current directory is /tmp/dist
2019-06-16 01:51:47,192 [INFO] project file /tmp/dist/hdl-prj.json not found

raised ERROROUT.OPTION_ERROR : errorout.adb:399
[Error - 01:51:47] Connection to server got closed. Server will not be restarted.

The relevant part is raised ERROROUT.OPTION_ERROR : errorout.adb:399.

Surprisingly, this seems to be some kind of startup check. I.e., if the content is available at /tmp/ghdl/install-llvm-6.0 when a *.vhdl is opened in VSC, the extension works ok. I can then move or remove the ghdl folder and opening other VHDL files works too. However, if I open a different folder with VSC (which makes it reload the window and reconnect to the container) or I just Ctrl+R, it fails again.

As a result, I think that some code in libghdl*.so might depend on the prefix that was used at build time, and not on the current location. I have not guessed which specific resources is it trying to find.


The Dockerfile above is based on two images which are not publicly available yet (ghdl/build:ls-ubuntu and ghdl/run:ls-ubuntu). This is the source, in case you want to build them:

docker build -t ghdl/build:ls-ubuntu . -f ubuntu --target=build
docker build -t ghdl/run:ls-ubuntu . -f ubuntu --target=run
ARG IMAGE="ubuntu:bionic"

#---

FROM $IMAGE AS base

RUN apt-get update -qq \
 && DEBIAN_FRONTEND=noninteractive apt-get -y install --no-install-recommends \
    ca-certificates \
    curl \
    gcc \
    make \
    python3 \
    python3-pip \
    zlib1g-dev \
 && apt-get autoclean && apt-get clean && apt-get -y autoremove \
 && update-ca-certificates \
 && pip3 install setuptools wheel

#---

FROM base AS build

RUN apt-get update -qq \
 && DEBIAN_FRONTEND=noninteractive apt-get -y install --no-install-recommends \
    clang-6.0 \
    git \
    gnat \
    llvm-6.0-dev \
    npm \
 && apt-get autoclean && apt-get clean && apt-get -y autoremove \
 && update-ca-certificates \
 && npm install -g vsce

#---

FROM base AS run

RUN apt-get update -qq \
 && DEBIAN_FRONTEND=noninteractive apt-get -y install --no-install-recommends \
    apt-transport-https \
    libgnat-7 \
    libllvm6.0 \
 && apt-get autoclean && apt-get clean && apt-get -y autoremove \
 && update-ca-certificates
tgingold commented 5 years ago

The absolute path is the one given by --prefix and is used to locate the vhdl libraries. So even if the libraries are copied by setup.py, they are currently not used.

To use them, just re-enable the 'set the default prefix' part of libghdl/init.py

Sorry, libghdl.py integration/build is still WIP!

eine commented 5 years ago

The absolute path is the one given by --prefix and is used to locate the vhdl libraries. So even if the libraries are copied by setup.py, they are currently not used.

All makes sense now... Is this built in the shared library or is it defined in the Python part?

Sorry, libghdl.py integration/build is still WIP!

It's ok. I just wanted to make sure that this was a 'bug' and not an error on my side.

This is something to check when libghdl*.so is made part of GHDL in #840. GHDL itself does not honour the --prefix I think. At least, it is possible to make install it to any location, and the package and move that location.

To use them, just re-enable the 'set the default prefix' part of libghdl/init.py

I will try this.

tgingold commented 5 years ago

The prefix is built in libghdl.so; but as you noticed ghdl tries to find where it is installed. This is almost required for Windows, a platform where the user may install the ghdl binary at any place.

eine commented 5 years ago

FTR, setting the prefix in __init__.py, as suggested by @tgingold, works. Therefore, no 'big' change is requires to fix this. It would be possible to use os.environ.get['LIBGHDL_PREFIX'], instead of harcoding a True/False.

Moreover, in the current state of the codebase, I think it would be better if the default was ./. Nevertheless, it is not worth changing it now, becase #840 is almost ready.