✨ Foundation for Phase 0 Added - #98
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…s rewriting plan >20000
|
Had to change files-changed-only: true to files-changed-only: false in ci.yml as the PR also includes the docs for the rewriting plan -> files-changed-only: true crashes if more than 20000 lines changed. I will revert it back in the next PR |
marcelwa
left a comment
There was a problem hiding this comment.
Many thanks for getting this started. It goes in the right direction, we just need to get a few high-level things out of the way before we focus more on the details. Expect a few review rounds and don't get discouraged 😉
There was a problem hiding this comment.
Please remove all plans from this branch that are not directly related to the current phase.
| "testsuite": [ | ||
| { | ||
| "name": "PortDetectionDefaultsToManual", | ||
| "file": "\/Users\/michaelfeldmeier\/Documents\/GitHub\/scpd\/test\/design\/test_config_schema.cpp", |
There was a problem hiding this comment.
There should never be file paths to your personal machine anywhere in a PR
| ein = "ein" | ||
| als = "als" |
There was a problem hiding this comment.
There should be no need for German spelling anywhere in the repo
There was a problem hiding this comment.
Let's maybe rename the folder to flatbuffers and also drop the _generated postfix. That makes the point clearer and improves readability.
There was a problem hiding this comment.
Same here: let's rename the folder to flatbuffers.
Additionally, is there any way to group the generated Python files into submodules by association to give more meaning to an otherwise flat directory?
|
Currently, your PR description still carries a
that should be removed. Also, the manual line breaks in the PR description should be eliminated. |
marcelwa
left a comment
There was a problem hiding this comment.
The PR description still contains manual line breaks and outdated information like the referenced generated/ folder or the number of GoogleTests.
I noticed that the code uses British English spelling. I believe the rest of the MQT uses American English. Maybe @denialhaag could comment on whether there is a constraint to use one spelling over the other. Especially for coding symbols, that matters.
| - 👷 Fail CI when the committed schema-generated code is stale ([#97]) | ||
| ([**@FeldmeierMichael**]) | ||
| - ✨ Add the FlatBuffers schemas of the data model, the committed C++ and Python | ||
| code generated from them, and the `nox -s schemas` session ([#97]) | ||
| ([**@FeldmeierMichael**]) | ||
| - 🏗️ Split the core into the eight per-module CMake targets of the architecture | ||
| ([#97]) ([**@FeldmeierMichael**]) |
| table Line { | ||
| start: Point (native_inline); | ||
| end: Point (native_inline); | ||
| } | ||
|
|
||
| /// A circular arc in layout space with an exact centre and radius. Angles are | ||
| /// radians; a positive sweep runs counter-clockwise. | ||
| table Arc { | ||
| centre: Point (native_inline); | ||
| radius: double; | ||
| start_angle: double; | ||
| sweep: double; | ||
| } |
There was a problem hiding this comment.
Line.start and .end as well as Arc.centre are mandatory according to the model but optional on the wire.
Edit: also applies to
Connection.targetCpwCoupler.portandCpwCoupler.centreBridge.centre- probably
DrcFinding.location
| /// The root of every stage artifact. | ||
| table Artifact { | ||
| /// The mqt-scpd version that wrote the artifact, for provenance. | ||
| producer: string; |
There was a problem hiding this comment.
The schema says producer records the version that wrote the artifact, but the field is optional, and producers that are omitted pass verification. Please mark it required if it is required, and test that the verifier rejects an artifact without it.
If not, nothing to do here.
Without a terribly deep reason, I would generally prefer to keep everything in American English. 🙂 |
marcelwa
left a comment
There was a problem hiding this comment.
🤖 AI text below 🤖
Requesting changes for six data-model, serialization, and build-system issues. The blocking issues are the unpersisted coupler-created port and Python's ability to write artifacts that the C++ verifier rejects.
| /// Final stage at exact physical dimensions in layout units. | ||
| table CpwCoupler { | ||
| /// The ResonatorSource port the coupler carries. | ||
| port: PortRef (required, native_inline); |
There was a problem hiding this comment.
🤖 AI text below 🤖
[P1] Persist the coupler-created port in a stage output. CpwCoupler.port is a PortRef into Chip.ports, but Final receives an immutable Chip and returns only FinalRouting. Neither FinalRouting nor the stored 00-chip.json contains the appended port or the updated connection whose source was absent during Assignment. The reference therefore cannot remain valid after reload, which breaks resume, plotting, and DRC. Please persist the new port and its connection mapping in the stage output, or return and store an updated chip.
| def test_final_routing_keeps_scalar_vectors() -> None: | ||
| """A vector of indices survives packing with zero, one and several elements.""" | ||
| for unresolved in ([], [7], [2, 5, 11]): | ||
| artifact = ArtifactT( |
There was a problem hiding this comment.
🤖 AI text below 🤖
[P1] Do not treat this artifact as valid without producer. The generated Python End methods do not call Builder.Required, and the Python FlatBuffers runtime has no structural verifier. With FlatBuffers 25.12.19, I reproduced a 16-byte artifact with producer=None and output=None that Python writes and reads successfully, while C++ VerifyArtifactBuffer rejects it. Because Python is expected to write the stage artifacts, add a checked, non-generated serialization and read layer, make missing required fields fail, and include producer in valid round-trip tests.
| /// carries the ResonatorSource role. | ||
| source: PortRef; | ||
| target: PortRef (required, native_inline); | ||
| source_role: AssignedRole; |
There was a problem hiding this comment.
🤖 AI text below 🤖
[P2] Distinguish missing roles from FeedlineSource. FlatBuffers scalar fields are not required here: when either role is absent, the generated accessor returns enum value 0 and the verifier still succeeds. I reproduced a serialized connection with both role offsets absent that decoded as (FeedlineSource, FeedlineSource). The same silent-default problem affects Port.role, Wire.connection, dimensions, and design-rule values. Please use invalid enum sentinels or optional scalars where absence matters, and add semantic validation after structural verification.
| feedlines_skipped: uint32; | ||
| } | ||
|
|
||
| root_type DrcReport; |
There was a problem hiding this comment.
🤖 AI text below 🤖
[P2] Add a root that can represent the complete drc.json. DrcReport contains one stage, and this declaration makes one report the JSON root, while the pipeline contract says the Final and Finalize reports both go into the same file. A top-level JSON array around two reports would no longer conform to this root schema. Please add a container with a vector of reports, or redesign the root around per-stage groups.
| target_link_libraries(${PROJECT_NAME} PRIVATE MQT::ProjectOptions) | ||
|
|
||
| # add MQT alias | ||
| add_library(MQT::SCPD ALIAS ${PROJECT_NAME}) |
There was a problem hiding this comment.
🤖 AI text below 🤖
[P2] Preserve or document the removal of the public MQT::SCPD target. Existing downstream CMake consumers that link this alias stop configuring after this change. The changelog currently lists only additions, and UPGRADING.md has no migration entry. Please keep a compatibility umbrella target that links the appropriate module, or record the breaking change and its replacement targets in both places.
| set(alias MQT::Scpd${ARG_ALIAS_NAME}) | ||
|
|
||
| # collect headers and source files | ||
| file(GLOB_RECURSE headers ${MQT_SCPD_INCLUDE_BUILD_DIR}/mqt-scpd/${module}/*.hpp) |
There was a problem hiding this comment.
🤖 AI text below 🤖
[P2] Make additions to these globs trigger CMake regeneration. Without CONFIGURE_DEPENDS, adding the first .cpp to a module does not cause an existing build tree to reconfigure, so the target remains an interface library and the new implementation is not compiled. The per-module test globs have the same problem. Please list sources explicitly or use CONFIGURE_DEPENDS for both source and test discovery.
| # CMake writes these into the working directory of gtest_discover_tests | ||
| cmake_test_discovery_*.json |
There was a problem hiding this comment.
With #103, SCPD is now using a templated .gitignore. When resolving the merge conflict, you can just pick the version on main. 🙂
Signed-off-by: FeldmeierMichael <102790823+FeldmeierMichael@users.noreply.github.com>
🤖 AI text below 🤖
Description
This pull request implements phase 0 of the FridgeCAD port: the repository structure and the data model, with no domain logic. It also brings in the architecture document, the data-model and pipeline documents, and the eight decision records that this code relies on.
mqt-scpdtarget.cmake/AddMQTScpdLibrary.cmake, adapted from MQT Core'sAddMQTCoreLibrary.cmake, declaresmqt-scpd-<module>with the aliasMQT::Scpd<Module>, aFILE_SET HEADERSunderinclude/mqt-scpd/<module>/, and an export header. A module without source files is an interface library until its first source file arrives.cmake/CompilerWarnings.cmakeis adopted from MQT Core. The dependency direction ofARCHITECTURE.mdis encoded in the moduleCMakeLists.txtfiles, and each module owns a test directory undertest/<module>/.schemas/define the data model: geometry, the design (ports, the two role enums, design rules, couplers, bridges), the run configuration withdetectionandstart_componentfor phase 1, the six stage artifacts behind oneArtifactroot, and the DRC report. Each schema has its own namespace,mqt.scpd.flatbuffers.<schema>. The schemas hold what phases 0 and 1 read; later stages append their tables and fields when they arrive. Every field that the in-memory model holds by value is markedrequired, so the verifier rejects a buffer that omits it instead of unpacking it to a default.- Generated code is committed: one C++ header per schema underinclude/mqt-scpd/flatbuffers/in the namespacemqt::scpd::flatbuffers::<schema>, and one Python subpackage per schema underpython/mqt/scpd/flatbuffers/with one module per type. Python code is generated for the schemas Python reads or writes itself;drc.jsonis read as JSON, sodrc.fbshas no Python module. No.pyistubs are committed; the modules keep their inline annotations.nox -s schemasregenerates the committed code. It buildsflatcfrom the FlatBuffers source that the C++ build fetches (MQT_SCPD_BUILD_FLATC, off by default and never part of a wheel build), so the generator and the runtime cannot disagree on their version.nox -s schemas -- --checkfails on stale output.schemasjob that runs this check as one of the required checks. cpp-linter now walks the repository instead of fetching the pull request diff, because GitHub refuses diffs above 20,000 lines. Codecov ignores the generated headers, and.gitignorecovers thecmake_test_discovery_*.jsonfiles thatgtest_discover_testswrites into the source tree.addplaceholder from the repository template is removed.ARCHITECTURE.mdanddocs/design/data-model.mddescribe the module declaration, the schema files, the namespaces and the regeneration flow. The generated code is excluded from the Doxygen and autoapi builds.Two details are worth knowing:
--gen-onefileomits the imports of object-API types from included schemas, and--no-python-gen-numpyplacesEndVector()inside the loop (fixed upstream in Fix indention level for --no-python-gen-numpy google/flatbuffers#9049 after the release). The session therefore generates one Python module per type and keeps numpy optional.end-of-file-fixerandtrailing-whitespacehooks therefore exclude the two generated directories; ruff, ty, typos and the license tool already skip them through their own configuration.Dependencies:
flatbuffersfrom PyPI is a new runtime dependency (the runtime of the generated Python) and is also listed in thetestgroup. FlatBuffers 25.12.19 is fetched throughFetchContentfor the header-only C++ runtime. The branch is based onmainafter ⬆️🐍 Adopt nanobind 3 split-mode wheels #96, so it builds with nanobind 3.Verified locally:
cmake --preset releaseandctest --preset release(29 tests, no compiler warnings),uvx nox -s tests-3.13,uvx nox -s lint,uvx nox -s stubs(no change),uvx nox -s schemas -- --checkon a committed snapshot, anduvx nox -s docs.