Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More distro friendly cmake fixes. #655

Merged
merged 1 commit into from
Nov 13, 2022
Merged

More distro friendly cmake fixes. #655

merged 1 commit into from
Nov 13, 2022

Conversation

cbalint13
Copy link
Contributor

@cbalint13 cbalint13 commented Nov 13, 2022

(x ) This PR address few issues:

  1. Fix and advertise the optional SLANG_LIB_NAME for either STATIC or SHARED (see #650).
  2. Fix SHARED mode, add PIC related flags and condition library visibility settings.
  3. Automate adding of needed stdc++fs / c++fs variants for various compiler needs.

(x) This PR preserves and does not affect current build behaviour.

Cc: @jrudess


(x) See the introduced advertisements:

%cmake .. -Wno-dev \
       -DCMAKE_SKIP_RPATH=ON \
       -DCMAKE_VERBOSE_MAKEFILE=OFF \
       -DCMAKE_BUILD_TYPE=RelWithDebInfo \
       -DBUILD_SHARED_LIB=ON \
       -DSLANG_LIB_NAME="svlang" \
       -DSLANG_INCLUDE_LLVM=OFF \
       -DSLANG_INCLUDE_DOCS=OFF \
       -DSLANG_INCLUDE_TESTS=ON \
       -DSLANG_INCLUDE_PYLIB=ON \
       -DSLANG_INCLUDE_TOOLS=ON \
       -DSLANG_INCLUDE_INSTALL=ON
-- CMake version: 3.20.2
-- slang version: 2.0.0+211ccf5
-- The CXX compiler identification is GNU 8.5.0
{...}
-- Linking with: stdc++fs
-- Build SHARED library as: svlang
{..}

(x) Builds pass on {x86_64, ppc64le, aarch64} x {rhel8, rhel9, fc35, fc36, fc37, fc38}, see:
https://copr.fedorainfracloud.org/coprs/rezso/HDL/build/5034654/


@MikePopoloski,

Thank you for having this excellent tool !
~Cristian.

@codecov
Copy link

codecov bot commented Nov 13, 2022

Codecov Report

Merging #655 (37c9a6b) into master (211ccf5) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   91.30%   91.31%   +0.01%     
==========================================
  Files         185      185              
  Lines       42677    42690      +13     
==========================================
+ Hits        38967    38984      +17     
+ Misses       3710     3706       -4     
Impacted Files Coverage Δ
source/ast/symbols/PortSymbols.cpp 96.72% <0.00%> (ø)
include/slang/ast/symbols/InstanceSymbols.h 100.00% <0.00%> (ø)
source/driver/Driver.cpp 98.88% <0.00%> (+<0.01%) ⬆️
source/ast/symbols/InstanceSymbols.cpp 85.46% <0.00%> (+0.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 211ccf5...37c9a6b. Read the comment docs.

external/CMakeLists.txt Outdated Show resolved Hide resolved
source/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@@ -154,6 +163,59 @@ if(NOT CMAKE_INSTALL_RPATH AND BUILD_SHARED_LIBS)
set(CMAKE_INSTALL_RPATH ${base} ${base}/${relDir})
endif()

set(STD_FS_LIB "")
# ==== The test below is based on:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example compiler where this is still needed? Note that slang does not build with a pre-C++17 compiler, and the minimum supported gcc version is 9 (and clang is 10) neither of which need the separate lib. In a few months I will bump the minimum gcc version to 10 and start requiring C++20 support so I'm wondering if there's some old system that will be broken if not supporting an older version.

Copy link
Contributor Author

@cbalint13 cbalint13 Nov 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Yes, RHEL 8 (having gcc 8.x ) will fail without this.
  • Not tested (myself) with other combinations outside fedora/rhel products.

A well proven routine is used from pybind11 library.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can merge this for now, but please be aware that I don't have the ability to maintain the codebase for very old compilers and so the build may or may not work for GCC 8 (and as mentioned I will be raising the minimum required C++ version to 20 early in 2023) and may be broken by any future changes that I make. Even the GCC team does not support GCC 8 anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Currently rhel8 is fine, in case you start require c++20, that's it, will drop out rhel8 support.
  • There would be still some releng way to invoke gcc 10 even on RHEL 8, will update packaging at that time.

If interested in rhel/fedora native repo having sv-lang the automated (weekly) builds are here .

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Balint Cristian <cristian.balint@gmail.com>
Copy link
Owner

@MikePopoloski MikePopoloski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks a lot for the PR!

@cbalint13
Copy link
Contributor Author

lgtm, thanks a lot for the PR!

Also, thank you for your patience !
~Cristian.

@MikePopoloski MikePopoloski merged commit c04fd52 into MikePopoloski:master Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants