vvp: create libvvp as versioned library#1310
vvp: create libvvp as versioned library#1310rhabacker wants to merge 4 commits intosteveicarus:masterfrom
Conversation
There was a problem hiding this comment.
I need to look at this in more detail, but the first thing I noticed is I think the library SO version should match the program version and that should be determined using the version program.
Also what systems have you validated this on. This is where we often have breakage because of differences in compilers/OSs.
378d799 to
86e5bff
Compare
The
Validated on native GCC on Linux/openSUSE and with the openSUSE MinGW cross-compiler targeting Windows. The build correctly generates the import library, which is compatible with both GCC and MSVC compilers. |
|
This passes the CI, but if I remember correctly none of the CI tests are enabling VVP to be built with a library, so we have no guarantee this is compatible across all the compiler/OS combinations. My fundamental issue with your latest changes is you moved the version information to a new place and we have scripts that help use do new branches and releases that are now broken (see the scripts directory). Why did you not generate what you needed in the Makefile using the For example I use the following to create the name for the versioned PDF of the combined manual pages: Also the To properly check this I likely need to locally build both a normal and library enabled version of the repo and verify both work properly for the various systems I test locally. We should also likely add at least one or two library builds to the CI flow. |
Extension is straightforward if needed.
The issue is that the version program is only generated after running configure and make, but the library version must already be set during the configure step. In the autoconf workflow, the version is typically defined with AC_INIT.
This requires a full configuration and build, which is too late for defining the library version needed at configure time.
This can be changed easily.
Suggested fixes:
|
|
Thanks for the detailed reply, let me look into this. I'm sure we can come up with something that will work. If we switch to AC_INIT I think we should eliminate the version header file to avoid confusion. I'll reply more later. |
Such a header, provided it is also installed by the library, can be useful in projects that use this library for managing or ensuring version-specific aspects in the source code, for example in code models. |
86e5bff to
1798516
Compare
dc935ed to
c4ea2ae
Compare
fixed
I have added libvvp to the GitHub Workflow jobs and ensured that the shared library can be found by the vvp executable on Unix-like operating systems by using LD_LIBRARY_PATH. It also works on Windows, since the shared library is located in the same directory as the executable file (I verified this with a cross-build for Windows on the Linux distribution openSUSE Leap). |
|
I got a related feedback from the ngspice maintainers at https://sourceforge.net/p/ngspice/ngspice/merge-requests/49/#ec46
This can be achieved by making the Autotools variable LIBVVP_SOVERSION independent of the package version, so that it changes only when there is actually a backward-incompatible change in the library's interface. |
c4ea2ae to
26d60d2
Compare
This has been fixed in latest push |
26d60d2 to
6165f77
Compare
|
6165f77 fixes issues with installing the pkgconfig file during cross-compilation for Mingw. It should only be installed when using ./configure --enable-libvvp and should take DESTDIR into account during installation. |
|
The issue with making the library version independent of the compiler version is that the vvp input file API is allowed to change between major versions. So even when the libvvp client ABI hasn't changed, a new version will not necessarily work with previously compiled simulation code. I would suggest making the library major version reflect the client ABI, and making the library minor and extra version match the compiler major and minor version. Then clients can decide for themselves how much compatibility they want to enforce. On a quick look at your patch, I can see changes to the makefile install target, but no corresponding changes to the uninstall target. Changing the CI to test the libvvp build means we are no longer testing the non-libvvp build, which is the normal use case. |
|
I've been thinking about this specific issue the last few days and I agree with Martin. People may not know this but there is code in the compiler and run-time that make sure the user compiles and runs code that is compatible. While the ABI for using the library may not change which is what the library version is primarily concerned with and I agree with the NGSPICE folks it does not need to match the Icarus version. We also need the version in the name somewhere to help the user make sure they are using compatible version. See: Currently 13.0 and 14.0 are compatible if you hack the version, but there will be changes in 14 that will eventually cause it to diverge from 13.0. One of the requirements in a stable branch (e.g. 13) is that we do not break the syntax between the compiler and run-time. The fundamental rule is that the major versions must match between the compiler and the run-time and a warning is generated if the minor version of the compiler is greater than the run-time version indicating that there could be compatibility issues. Maybe maybe making a 1.13.0 for stable and 1.14.0 for our current development is the best way to go. |
6165f77 to
b08aeef
Compare
|
Updated patch with uninstalling library files added. |
|
Actually one problem with making it 1.<ivl_version> is you cannot have V13 and V14 installed at the same time. Which may not be an issue for general users, but does have an impact on development testing. The advantage for general users is they can upgrade the the Icarus version without needing to recompile other tools (like NGSPICE). |
Uses the package version for the SONAME and full library version. For linking, a pkg-config file is generated, and when building on Windows, an import library is created that can be used with both GCC and MSVC compilers. On non-Windows platforms, all object files are compiled with -fPIC to ensure compatibility with shared libraries. The shared library is assumed to be ABI-stable within the same major version, so the SONAME reflects only the major version number.
… only Other compiler like MSVC normally are not using any library prefix.
b08aeef to
37f3bee
Compare
|
Updated patches
|
With these changes, the vpp library can be compiled as a shared library on Unix-like operating systems and on Windows using the MINGW compiler.
The build support for Windows has been tested with the openSUSE MinGW project (see https://build.opensuse.org/project/show/windows:mingw:win64)
Since this library is required by ngspice, a merge request was created at (https://sourceforge.net/p/ngspice/ngspice/merge-requests/49/) to allow the library to be used directly, which simplifies the packaging process. The patch was built and tested on openSUSE Leap with gcc and on the openSUSE MinGW project with a gcc cross-compiler for Windows.