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

🧱 Include MQT Core via FetchContent instead as submodule #455

Merged
merged 27 commits into from
Jun 19, 2024

Conversation

ystade
Copy link
Contributor

@ystade ystade commented May 22, 2024

Description

MQT Core was included as a submodule so far. This PR removes this submodule and uses the existing fetch content commands to include this dependency.

Furthermore, it adds a dependabot-like update workflow for the FetchContent-managed mqt-core dependency.

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.

@ystade
Copy link
Contributor Author

ystade commented May 22, 2024

This PR will only pass all test when a new release of MQT Core is build that contains the Neutral Atom Support. The version number is already incremented accordingly. If this is not desired, the last commit can also be reverted to reference the appropriate commit directly.

@ystade ystade requested a review from burgholzer May 22, 2024 15:39
@ystade
Copy link
Contributor Author

ystade commented May 22, 2024

As far as I can see, this is also related to #418, and realises a part of it.

@burgholzer burgholzer added dependencies Pull requests that update a dependency file c++ Anything related to C++ code labels May 22, 2024
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

In principle, I am in favour of just merging this. There is one corner case, that gets worse with this change though: By not providing the submodule fallback, any initial build from a fresh clone will require internet access, i.e., it is no longer possible to build the project without network connection given a fresh clone.
In the long run, this shouldn't be an issue for the Python side of things once #418 is in since mqt-core will then be a build and runtime dependency and, as a result, always be available when installing the Python package.
For the C++-only side of the project, it might not be that clean. Sure, people could start installing mqt-core on their systems and then use the find_package part of FetchContent to get that version. This is probably what most packagers would do if they ever where to package our tools for things like FreeBSD, homebrew, conda-forge, etc.

None of the above is opposing this change. I just wanted to put this out there to spread awareness. I just triggered a new mqt-core release, which will be 2.5.0.

cmake/ExternalDependencies.cmake Outdated Show resolved Hide resolved
@burgholzer
Copy link
Member

As far as I can see, this is also related to #418, and realises a part of it.

Yeah. There is some overlap here. And I think that is great. It further reduces the size of #418 and eliminates some of the changes there.

Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
@burgholzer
Copy link
Member

burgholzer commented May 30, 2024

Another small drawback that I just noticed is that we kind of loose the automatic dependency updates (i.e., dependabot) for keeping the submodule up to date. But maybe this isn't too important as updates of mqt-core should be driven by demand anyways.

@burgholzer
Copy link
Member

I just thought about this a little more. I think I would be fine with dropping the offline installation possibility given the fact that, this is hardly possible at the moment anyways since mqt-core also uses FetchContent without backing the dependencies up by git submodules.
As such, this was a rather weak argument.

I'd still like to keep the automatic updates though. However, it should be possible to write a rather simple GitHub workflow that periodically checks for new releases of mqt-core via the GitHub API and automatically opens a PR that updates the version correspondingly.
In the end, that's a simple regex parse of one particular file, a call to the GitHub API for checking if a new version is around, a regex replace with the new version number, and another GitHub API call for creating the update PR (there are actions for that).
I'll see whether I can make that work quickly and push a proposal to this branch.
If we find a solid enough solution for that here, then I would roll that solution out to all top-level repos that make use of mqt-core.

this allows to easily point fetch content to a branch or a simply a specific commit.
`MQT_CORE_REV` can be anything from
 - `origin/branch-name`
 - `v2.5.0` or `v${MQT_CORE_VERSION}`
 - `0e4ff9e0521886449027b252c65913e1afa863b0`

Signed-off-by: burgholzer <burgholzer@me.com>
since `MQT_CORE_REV` is a commit hash past the `v2.5.0` release, the appropriate version is `2.5.1`.
Going forward, it should be ensured that the commit hash and the version always work together, i.e., switching the `rev` might also necessitate updating the version.

Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Alright. This is ready to go in ✅
Let's see how long it takes until the auto update workflow breaks 😅

@burgholzer burgholzer enabled auto-merge (squash) June 19, 2024 07:12
@burgholzer burgholzer merged commit 06a0c22 into main Jun 19, 2024
35 checks passed
@burgholzer burgholzer deleted the wip-fetch-core branch June 19, 2024 08:25
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.4%. Comparing base (7604d32) to head (f87e0ac).
Report is 90 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #455   +/-   ##
=====================================
  Coverage   90.4%   90.4%           
=====================================
  Files         86      86           
  Lines       9982    9982           
  Branches    1696    1696           
=====================================
  Hits        9025    9025           
  Misses       957     957           
Flag Coverage Δ
cpp 90.1% <ø> (ø)
python 95.9% <ø> (ø)

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 dependencies Pull requests that update a dependency file
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants