--- Log opened Wed May 21 00:00:50 2025
04:30 < stickies-v> I pushed another commit (same branch) to fix missing includes which was causing CI failure on macos and windows
04:31 < stickies-v> but the win64 native job is still failing during linking because `bitcoinkernel.lib` can't be found (logs indicate a `bitcoinkernel.dll` is produced) https://github.com/stickies-v/bitcoin/actions/runs/15159768806/job/42622979356
04:31 < stickies-v> will keep looking but am a bit out of my depth here
06:46 < stickies-v> okay, thats fixed with a new commit. but now msvc can't find the C++ kernel_header classes 🤦 https://github.com/stickies-v/bitcoin/actions/runs/15162095304/job/42630413343
06:46 < TheCharlatan> thanks stickies-v , incorporated your patches into the various commits. 
06:46 < TheCharlatan> Oh, or not yet :P
06:48 < stickies-v> i think we're close, just one build failing at linking stage
06:49 < stickies-v> for bitcoinkernel.h we're using BITCOINKERNEL_API to export symbols for WIN32, reckon we need to do the same for bitcoinkernel.hpp?
06:51 < TheCharlatan> yup that is it. Just didn't bother to do that yet with the cpp header.
06:54 < stickies-v> alright got a new ci run going with that fix, hopefully all sorted then: https://github.com/stickies-v/bitcoin/actions/runs/15164085800
07:08 < TheCharlatan> you probably also need to export from the other headers that are now installed. 
07:27 < stickies-v> i wish i know what that means
07:45 < TheCharlatan> seems to be working though by your logs. But not sure yet how to handle that warning.
07:46 < stickies-v> just pushed a new commit with a 4251 pragma disable warning, this seems to be a common "issue" with using e.g. std::unique_ptr in DLLs
07:50 < stickies-v> https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4251 seems useful
07:56 < stickies-v> an alternative to exporting the class, and suppressing the warning, is to export all the class members, as well as factory methods. that actually might work well for this library?
07:58 < TheCharlatan> mmh, that might be and option too.
08:18 < stickies-v> all builds now fixed: https://github.com/stickies-v/bitcoin/actions/runs/15165295448/job/42641393740 (using suppression)
08:27 < stickies-v> hmm. I think it's fine to suppress 4251 for all the private std::unique_ptr usage, because the client should never see that, but I think it's probably not safe to have e.g. a `explicit Transaction(std::span<const unsigned char> raw_transaction) noexcept;` ctor that takes a `std::span`?
08:50 < stickies-v> (upon further reading, it seems like `std::span` should actually be pretty safe)
08:51 < stickies-v> then again - we have the C header for users that require ABI stability. so maybe sacrificing the C++ header's usability for increased (but still not complete ABI stability is a dumb trade-off? so suppressions could be fine?
09:54 -!- cfields [~cfields@user/cfields] has joined #bitcoin-kernel
12:03 < TheCharlatan> yes, I think it should be fine.
12:03 < TheCharlatan> I'll apply this last patch too then :)
12:39 < stickies-v> Oh sorry forgot to message you, added another patch to fix the chainstate functional test by adding the envvar
12:40 < stickies-v> Hmm, looks like it still hasn't fixed the functional test suite but on my phone ATM: https://github.com/stickies-v/bitcoin/actions/runs/15168196897/job/42651411661
12:54 < TheCharlatan> yeah, probs missing this commit https://github.com/bitcoin/bitcoin/pull/30595/commits/65fe5d03e7a2d0d00d7d37bd426fd6532fff3c06
15:00 < TheCharlatan> seems to have fixed it for the native test, the cross test is still broken
15:00 < TheCharlatan> https://github.com/TheCharlatan/bitcoin/actions/runs/15171548625
--- Log closed Thu May 22 00:00:52 2025