Skip to content

Conversation

@ausbin
Copy link
Collaborator

@ausbin ausbin commented Oct 7, 2025

With these changes, the compilation flow I need in the Qwerty compiler works. That is, I can create a qiree_sys crate that builds a static QIR-EE and provides Rust bindings, call QIR-EE from Rust code, and link the whole thing into a Python extension module (a self-contained shared library).

I think this PR is easiest reviewed commit-by-commit. I tried to leave an explanation of each change in each commit.

Testing

The unit tests pass and bin/qir-qsim ../examples/bv11010.ll runs successfully. (Note that I built only with -DQIREE_USE_QSIM=ON -DQIREE_USE_XACC=OFF, though.)

I can also call qiree_create() and qiree_destroy() from inside the Qwerty runtime without any obvious explosions.

ausbin added 5 commits October 6, 2025 22:13
Passing `MODULE` to `add_library` seems to force a shared library to be
built even if `BUILD_SHARED_LIBS=OFF`.
Without this, the C headers are not installed.
`#include <cstdint>` does not compile on a C compiler.
Without this, the C++ compiler on my GNU/Linux system cannot find
`std::sort()`.
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! (If you move those to a separate PR I can approve and merge them immediately.) The one-line change removing MODULE may have consequences that are untested: I'm actually a little surprised it works as is in all configurations, which might just mean we need a little more testing (e.g. a runner that builds static libs).

@ausbin ausbin force-pushed the feature/cqiree-static-build branch 3 times, most recently from 1b75ab3 to ae86b9d Compare October 7, 2025 17:49
ausbin added 2 commits October 7, 2025 13:50
Otherwise, when modifying CMakePresets.json and running `git
clang-format`, I get:

    Configuration file(s) do(es) not support Json

Also update the pre-commit-hooks configuration to allow the second YAML
document that this commit adds in .clang-format.
@ausbin ausbin force-pushed the feature/cqiree-static-build branch from ae86b9d to f43ef9c Compare October 7, 2025 17:50
@ausbin ausbin force-pushed the feature/cqiree-static-build branch from f43ef9c to ef6d847 Compare October 7, 2025 17:53
@ausbin ausbin requested a review from sethrj October 7, 2025 18:04
@sethrj
Copy link
Member

sethrj commented Oct 7, 2025

Please avoid force pushing! 🙂

@ausbin
Copy link
Collaborator Author

ausbin commented Oct 7, 2025

Why?

@sethrj
Copy link
Member

sethrj commented Oct 8, 2025

@ausbin It interferes with the review process. When you force push:

  1. It becomes difficult/impossible to tell changes you've made since the last review.
  2. The automatic record of whether your previous changes passed or failed the CI are lost.
  3. The reviewer clicking on a notification on the update for the PR gets a page that says "the commit range can't be found" rather than a page showing the changes you pushed.
  4. You are noncompliant with policy in the CONTRIBUTING guidelines (which also documents most of these reasons)

@ausbin
Copy link
Collaborator Author

ausbin commented Oct 8, 2025

Thanks for sharing your perspective. It sounds like we have different styles of using git — agree to disagree.

In this case, I was constructing a readable commit history while also fighting with CI. When you have time, please feel free to check out the four new commits added today. I did not modify previous commits.

@sethrj
Copy link
Member

sethrj commented Oct 8, 2025

@ausbin I use force push all the time and am a huge supporter of rebasing, but only before a review has started. I personally do prefer readable commit histories, but after a review starts the review process needs to become part of the history.

Because we use the "squash" method, the changes and merges and so forth done during review are silently flattened into a single clean commit which is the only history most people see. The iterations made during review are saved in this pull request's commit history which is also very useful.

You can disagree with style and policies, but part of working in an open source collaboration is agreeing to follow the development practices required by the project.

set(_LIB_TYPE)
endif()

qiree_add_library(cqiree ${_LIB_TYPE}
Copy link
Member

Choose a reason for hiding this comment

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

Another alternative to this is to use an object library that allows us to build a dlopen-compatible module and a linkable (shared or static) library... @miniskar thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if you want me to attempt this

@ausbin ausbin requested a review from sethrj October 8, 2025 16:53
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.

2 participants