Implementing C++ and Rust wrappers packages for native bindings with parsing and reading - #85
Merged
Conversation
…apper for class parsing
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The checked-in excelreader-phase1.def has no LIBRARY statement, so lib.exe and dlltool had no way to know what DLL name to record in the generated import lib's import descriptors. lib.exe fell back to the .def's own basename (excelreader-phase1.dll, which never exists); dlltool was told a fixed release-asset name via -D that doesn't match a locally-built EXCELREADER_NATIVE_LIB override. Generate a temporary .def with an explicit LIBRARY line naming the actual basename of the file at _lib_path (download or override alike), and feed that to both lib.exe /def: and dlltool -d, dropping dlltool's now-redundant -D flag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The C++ package's include/ symlinked straight to src/ExcelReader.Native/include, so headers landed at the top level (include/excelreader.hpp) instead of under an xl/ prefix - but both the package README and every consumer (per the wrapper's own doc comments) use #include <xl/excelreader.hpp>. Re-point the symlink one level deeper (include/xl -> ../../src/ExcelReader.Native/include) so that path actually resolves, and update FetchNativeLib.cmake's reference to the phase-1 .def file to match. Also add xl_open_file to excelreader-phase1.def: xl::Workbook::open calls xl_open_file (not just the _ex variant), but the DLL only exported the _ex symbol via the def, so the C++ smoke test failed to link with 'undefined reference to xl_open_file'. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Exercises xl::Workbook::open and xl::parse_sheet<Row> end to end
against the RealExcel.xlsb fixture at the repo root, asserting the
same known-stable values tests/ExcelReader.NativeSmoke/smoke.c already
checks: 100 data rows, first row's Coluna1 == "Valor1" and
Coluna3 == 1.
Verified locally:
dotnet publish src/ExcelReader.Native -c Release -r win-x64 -f net10.0
cmake -S cpp -B build/cpp -G "MinGW Makefiles" -DEXCELREADER_BUILD_TESTS=ON
(EXCELREADER_NATIVE_LIB pointing at the published DLL)
cmake --build build/cpp --config Release
ctest --test-dir build/cpp --output-on-failure -C Release
-> 1/1 Test #1: excelreader_cpp_smoke ... Passed, prints
'OK: C++ smoke test passed'
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…import lib Applies the fix Task 4 discovered for the equivalent CMake problem: the shared excelreader-phase1.def has no LIBRARY statement, so lib.exe/dlltool would otherwise bake the wrong DLL name (the .def's own basename) into the generated import lib. build.rs now writes a temporary copy of the .def with an explicit LIBRARY line naming the real DLL - discovered by scanning EXCELREADER_NATIVE_LIB_DIR for its one *.dll when that override is set, since it may not be named like the downloaded release asset. Also works around a dlltool quirk found while verifying under MinGW: it mangles the -l output path into a temporary file name and fails to open it when that path is a long absolute path (a real Cargo OUT_DIR nested several directories deep). Runs dlltool with OUT_DIR as its working directory and a bare relative -l filename instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cargo:rustc-link-lib=dylib=excelreader_native was unconditional, so on
macOS/Linux it always searched for libexcelreader_native.{so,dylib} - a name
that never matches the real file (excelreader-native-<os>-<arch>.<ext> on the
download path, or whatever find_native_lib discovers on the
EXCELREADER_NATIVE_LIB_DIR override path). Same 'wrong name baked in' bug
class as the Windows .def fix, just missing the platform gate.
Windows keeps the fixed-name link (correct: the generated import library is
always named excelreader_native.lib regardless of the real DLL's name).
macOS/Linux now link via dylib:+verbatim=<real filename> instead, since there
is no import-library indirection to hide the real name behind on those
platforms.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verified a real compile succeeds now that workbook.rs fills in the previously-declared-but-missing module: dotnet publish src/ExcelReader.Native -c Release -r win-x64 -f net10.0, then EXCELREADER_NATIVE_LIB_DIR=<publish dir with only the .dll> cargo build --target x86_64-pc-windows-gnu from rust/excelreader. Clean build (cargo clean first) succeeds with zero warnings. Also adds rust/.gitignore for target/ and Cargo.lock, generated by this verification build and previously untracked.
Also fixes build.rs: on windows it never added OUT_DIR (where the generated excelreader_native.lib import library lives) to the linker search path, only the DLL's own directory. cargo build hid this bug because an rlib doesn't need real link-time symbol resolution; any actual link step (cargo test) failed with an unhelpful, output-free ld exit status 1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e Rust crate Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Lib.cmake
cpp.yml's 'Locate published native library' step set a path relative to
github.workspace, unlike rust.yml's equivalent step. CMake's EXISTS check
on a relative path resolves against the process cwd (passes at configure
time), but the relative string is then baked verbatim into
IMPORTED_LOCATION/IMPORTED_IMPLIB and later resolved against the build
directory instead - breaking the link step on Linux/macOS and the test
binary's POST_BUILD DLL-copy step on Windows.
Prefix each matrix leg's output path with ${{ github.workspace }},
matching rust.yml. As defense in depth, also absolutize
EXCELREADER_NATIVE_LIB in FetchNativeLib.cmake right after it is read,
so any caller passing a relative path (e.g. the common cd-build-dir &&
cmake .. invocation style) gets a stable absolute path baked in.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The publish-rust job seds rust/excelreader/Cargo.toml's version in a git checkout, then ran bare 'cargo publish', which refuses to publish with uncommitted changes. --allow-dirty is the documented flag for exactly this situation (verified via 'cargo publish --help'). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…f reading it from outside the crate
build.rs read ../../src/ExcelReader.Native/include/excelreader-phase1.def
via a CARGO_MANIFEST_DIR-relative path outside the crate directory.
'cargo package --list' confirms this path is not included in what gets
published, so a Windows user building the crate from crates.io hit a
build.rs panic ('failed to read ... excelreader-phase1.def').
include_str! resolves against build.rs's own source location, not
CARGO_MANIFEST_DIR at runtime - but since build.rs's SOURCE (not a
precompiled binary) ships in the package, cargo recompiles it, re-running
the include_str! at the consumer's build time too. An include_str! of a
path outside the crate directory therefore fails there exactly like the
fs::read_to_string it replaces would.
The fix is to keep a copy of the .def file inside the crate
(rust/excelreader/excelreader-phase1.def, added in a prior commit,
kept in sync with the canonical copy at
src/ExcelReader.Native/include/excelreader-phase1.def) and embed *that*
via include_str!. Verified: 'cargo package --list' now lists the .def
file; cargo build succeeds for both x86_64-pc-windows-gnu and
x86_64-pc-windows-msvc against a locally-built native lib; and the build
still succeeds with the external repo-root .def file renamed aside,
proving the packaged crate no longer depends on it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ers and cpp/README.md inaccuracies I7: root README's 'Other languages' section never mentioned the new cpp/ and rust/excelreader/ packages - add brief pointers matching the existing Python entry's style. M1: cpp/README.md's FetchContent example and rust/excelreader/README.md's Cargo.toml example showed placeholder versions (v0.1.0 / "0.1") that will never exist. Both packages version in lockstep with the main release tag (latest v2.1.2), so update to v2.1.3 / "2.1" - the next patch version these packages could actually ship in. M2: cpp/README.md said 'this package's include/ is a symlink' - only include/xl is the symlink (verified via ls -la); include/ itself is a real directory. Also documented the previously-undocumented EXCELREADER_NATIVE_LIB (env var, local binary override) and EXCELREADER_VERSION (CMake cache var, release tag override, falls back to v0.0.0 off-tag) variables, and corrected the opening prose's native function name from xl_open_file_ex to xl_open_file (verified against Workbook::open in excelreader.hpp). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…out .stderr) - error paths stay covered by unit tests
…o, guard release sed rewrites
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #85 +/- ##
==========================================
- Coverage 86.57% 86.56% -0.02%
==========================================
Files 128 129 +1
Lines 9500 9533 +33
Branches 1781 1789 +8
==========================================
+ Hits 8225 8252 +27
- Misses 952 958 +6
Partials 323 323 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Benchmark ResultsMeasured on ExcelReader.Benchmarks.ColdStartBenchmark
ExcelReader.Benchmarks.CsvParseBenchmark
ExcelReader.Benchmarks.CsvReadBenchmark
ExcelReader.Benchmarks.CsvWriteBenchmark
ExcelReader.Benchmarks.ParseBenchmark
ExcelReader.Benchmarks.ReadBenchmark
ExcelReader.Benchmarks.RealDataReadBenchmark
ExcelReader.Benchmarks.RecordWriteBenchmark
ExcelReader.Benchmarks.WriteBenchmark
ExcelReader.Benchmarks.XlsReadBenchmark
ExcelReader.Benchmarks.XlsWriteBenchmark
|
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 6 to 8. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v6...v8) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6 to 7. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6...v7) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [softprops/action-gh-release](https://github.com/softprops/action-gh-release) from 2 to 3. - [Release notes](https://github.com/softprops/action-gh-release/releases) - [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md) - [Commits](softprops/action-gh-release@v2...v3) --- updated-dependencies: - dependency-name: softprops/action-gh-release dependency-version: '3' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…dings Extends the single-name-per-field ABI (xl_column_spec) to an ordered candidate list resolved first-match-wins, matching the C# core's existing ExcelColumnAttribute(AllowMultiple) semantics, and threads it through the native core, C++, Rust, and Python bindings.
Bumps xunit.runner.visualstudio from 3.1.5 to 4.0.0 Bumps xunit.v3.mtp-v2 from 3.2.2 to 4.0.0 --- updated-dependencies: - dependency-name: xunit.runner.visualstudio dependency-version: 4.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: test - dependency-name: xunit.v3.mtp-v2 dependency-version: 4.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: test ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps Microsoft.CodeAnalysis.Analyzers from 5.6.0 to 5.9.0-1.26328.17 Bumps Microsoft.CodeAnalysis.CSharp from 5.6.0 to 5.9.0 --- updated-dependencies: - dependency-name: Microsoft.CodeAnalysis.Analyzers dependency-version: 5.9.0-1.26328.17 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: Microsoft.CodeAnalysis.CSharp dependency-version: 5.9.0 dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: Microsoft.CodeAnalysis.CSharp dependency-version: 5.9.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
--- updated-dependencies: - dependency-name: Sep dependency-version: 0.17.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Six tasks covering the ABI struct change, the C# native core (read, write, schema-inference), and the C, C++, Rust, and Python bindings, each with TDD steps grounded in the actual existing test files/helpers.
… list Bumps XL_ABI_VERSION to 2. xl_column_spec.name/name_len become names/name_lens/name_count; the read path (xl_parse_typed/xl_parse_arrow) tries each candidate in order and binds the first one present in the header row.
…location Adds NativeLimits.MaxNamesPerSpec and IsValidNameCount, mirroring the existing spec-count/name-length guards, and checks it first in TryDecodeColumnSpecs before any candidate-name pointer is read. Addresses code review finding on the multi-name column binding task.
…_column_spec layout
FieldBinding<Class, T, N> stores N candidate names (default N=1, source- compatible with the existing single-name make_field); build_specs marshals them into xl_column_spec's names/name_lens/name_count.
…esolution ColumnBinding.names replaces .name (a &'static [&'static str] instead of a single &'static str); the derive macro collects name plus every alias in declared order. Bumps XL_ABI_VERSION to 2.
ColumnSpec.name widens to str | Sequence[str] | None; _native gains column_spec_by_names alongside the existing single-name helper. Bumps XL_ABI_VERSION to 2 to match the native library.
…s/ExcelReader.Benchmarks/develop/Sep-0.17.0 Bump Sep from 0.15.2 to 0.17.0
…lop/multi-f1e6270e1a Bump Microsoft.CodeAnalysis.Analyzers and Microsoft.CodeAnalysis.CSharp
…s/ExcelReader.Tests/develop/test-0a3d55c981 Bump the test group with 2 updates
…ions/develop/softprops/action-gh-release-3 build(deps): bump softprops/action-gh-release from 2 to 3
…ions/develop/actions/setup-python-7 build(deps): bump actions/setup-python from 6 to 7
…ions/develop/actions/download-artifact-8 build(deps): bump actions/download-artifact from 6 to 8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.