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

🚸 Building Shared Libraries #538

Closed
wants to merge 16 commits into from
Closed

Conversation

burgholzer
Copy link
Member

@burgholzer burgholzer commented Jan 24, 2024

Description

This PR started out as a PR that should "just" improve the project installation before top-level projects eventually switch to the new mqt-core version. As part of that, it tried to address #530 by switching the library to build dynamic libraries by default. This considerably decreases the binary size and would, in general, be nice to support.

The corresponding changes need appropriate testing across all different projects to make sure that nothing is missed.
I will open corresponding PRs in all top-level projects that use mqt-core to make sure that the new library structure works for all those projects.

In particular:

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@burgholzer burgholzer added enhancement New feature or request refactor Anything related to code refactoring minor Minor version update c++ Anything related to C++ code packaging Anything related to Python packaging labels Jan 24, 2024
@burgholzer burgholzer added this to the MQT Core milestone Jan 24, 2024
@burgholzer burgholzer self-assigned this Jan 24, 2024
@burgholzer
Copy link
Member Author

Ok. Getting shared libraries to work properly is more work than anticipated.
I am going to split this PR apart and will focus on getting the changes that are not related to shared libraries merged as soon as possible so that development in the top-level projects can resume.

@burgholzer burgholzer changed the title 🚸 Improve project installation 🚸 Building Shared Libraries Jan 26, 2024
@burgholzer
Copy link
Member Author

Just rebased this PR on top of main and cleaned up the commits a little bit.
This should be a solid start once there is more time to look into this.

burgholzer added a commit that referenced this pull request Feb 7, 2024
## Description

This PR brings in a couple code quality improvements discovered when
working on #538.

## Checklist:

<!---
This checklist serves as a reminder of a couple of things that ensure
your pull request will be merged swiftly.
-->

- [x] The pull request only contains commits that are related to it.
- [x] I have added appropriate tests and documentation.
- [x] I have made sure that all CI jobs on GitHub pass.
- [x] The pull request introduces no new warnings and follows the
project's style guidelines.
burgholzer added a commit that referenced this pull request Feb 8, 2024
…ation` (#548)

## Description

This PR ensures that all the different `Operation` sub-classes get a
corresponding `typeinfo` and `vtable` symbol in the compiled binaries.
This is important for RTTI when using shared libraries and ensured that
`dynamic_cast`s across library boundaries work as intended.
Specifically, the `CompoundOperation` class was only defined in a
header, which led to the omission of said information. This is fixed by
creating a dedicated `.cpp` file and moving at least one definition
there.

This was discovered while working on #538 in combination with mqt-ddsim.

## Checklist:

<!---
This checklist serves as a reminder of a couple of things that ensure
your pull request will be merged swiftly.
-->

- [x] The pull request only contains commits that are related to it.
- [x] I have added appropriate tests and documentation.
- [x] I have made sure that all CI jobs on GitHub pass.
- [x] The pull request introduces no new warnings and follows the
project's style guidelines.
@burgholzer
Copy link
Member Author

Ok. Further progress here. To keep the history of this PR, I just squashed the previous commits and added another commit that reverts the respective changes.
It turns out, that it might actually not be as hard as expected to get this working since hidden symbol visibility apparently is not a must.

The current state of the PR just changes very few things and, at least locally, that was enough to allow building all of our top-level Python packages based on the installed mqt.core package.

@burgholzer
Copy link
Member Author

@burgholzer
Copy link
Member Author

The Python errors on Windows are interesting as I can't quite make sense of them. This needs some further investigation. It has something to do with the mqt-core-python target. So maybe (finally) removing that resolves the error.
Interestingly, the C++ tests work just fine on Windows.

The package check failure is due to the SONAME and Version symlinks as well as the shared libraries in a non-importable location. I am fairly sure it is fine to simply ignore these, but this also needs further investigation.

@burgholzer
Copy link
Member Author

Alright. The C++ tests working under Windows were simply because no shared libraries were built there.
The reason for the failure in the Python module was symbol visibility, which is hidden by default on Windows.
However, there is a convenient CMAKE option to export all symbols under Windows.
Once that was working, Python could not finde the mqt-core DLLs, which was fixed by adding the bin directory of the package installation to the DLL path.

@burgholzer burgholzer marked this pull request as ready for review February 12, 2024 15:43
@burgholzer burgholzer linked an issue Feb 12, 2024 that may be closed by this pull request
This reverts commit 8eb3fd98e98126ff2871945eedc0dfae1c2c70c7.
this avoids codesign issues under macOS which does not allow to load libraries that have been tampered with, e.g., by modifying the rpath information during installation
# Conflicts:
#	include/mqt-core/operations/CompoundOperation.hpp
#	include/mqt-core/python/qiskit/QuantumCircuit.hpp
#	src/CMakeLists.txt
Signed-off-by: burgholzer <[email protected]>
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.6%. Comparing base (7eb0ab7) to head (dc4f55c).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #538   +/-   ##
=====================================
  Coverage   91.6%   91.6%           
=====================================
  Files        148     148           
  Lines      14738   14745    +7     
  Branches    2365    2365           
=====================================
+ Hits       13506   13514    +8     
+ Misses      1232    1231    -1     
Flag Coverage Δ
cpp 91.4% <ø> (+<0.1%) ⬆️
python 99.7% <100.0%> (+<0.1%) ⬆️
Files Coverage Δ
include/mqt-core/dd/RealNumber.hpp 100.0% <ø> (ø)
src/mqt/core/__init__.py 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@burgholzer
Copy link
Member Author

Closing in favor of #662

@burgholzer burgholzer closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code enhancement New feature or request minor Minor version update packaging Anything related to Python packaging refactor Anything related to code refactoring
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

✨ Smaller Python wheels
1 participant