Move kt sys arch set to inside UniFFI.kt #111
Conversation
KendallWeihe
requested review from
nitro-neal,
kirahsapong,
shamilovtim,
diehuxx,
decentralgabe and
frankhinek
as code owners
KendallWeihe
force-pushed
the
kendall/pipeline-debug-logs
branch
from
d7a591d to
a79b833
Compare
KendallWeihe
changed the title
| @@ -106,7 +107,7 @@ jobs: | |||
| - build_x86_64_apple_darwin | |||
| - build_x86_64_unknown_linux_gnu | |||
| - build_x86_64_unknown_linux_musl | |||
| runs-on: macos-latest | |||
| runs-on: ubuntu-latest | |||
There was a problem hiding this comment.
I don't see an issue with this, but why this switch to Ubuntu building here? Seems fine.
There was a problem hiding this comment.
this is so our ci.yml's deploy (snapshot) is consistency with the release.yml's deploy (the final release)
AKA we would've caught this sooner had we run this on ubuntu
There was a problem hiding this comment.
I really wish mvn deploy could deploy to 2 servers in one go without rebuilding, but this is the best I could figure
| @@ -39,6 +39,7 @@ bind-kotlin: setup | |||
| generate --library bound/kt/src/main/resources/libtbdex_uniffi_aarch64_apple_darwin.dylib \ | |||
| --language kotlin \ | |||
| --out-dir target/bindgen-kotlin | |||
| sed -i '' 's/findLibraryName(componentName)/detectSystemTarget()/' target/bindgen-kotlin/tbdex/sdk/rust/tbdex.kt | |||
There was a problem hiding this comment.
Do you need to reflect this in the local build commands as well or nah?
There was a problem hiding this comment.
nope, just here
I have an action item to consider improvement around the just bind here
|
Recommend putting some more meat behind the Acceptance Test to help you catch this stuff; perhaps doing a simple tbDEX operation to flow through more of the stack @KendallWeihe ? |
agreed! #108 (comment) |
Original cause of change was a failing release pipeline https://github.com/TBD54566975/tbdex-rs/actions/runs/10322927501/job/28580932157#step:9:375
Originally, I thought the issue was that the
SystemArchitecture.set()code was detecting a mac environment even though the runner is ubuntu. I came to find out that the issue was actually that we have code paths which exist whereinSystemArchitecture.set()is not called prior to the initial loading of theUniFFI.kt'sloadIndirect()function (here). Previously we had to carefully construct all code such thatSystemArchitecture.set()was always the first function to be called, for example you can see so in thegetOfferings()function here. But, this is not a very resilient way forward. Instead, what we should really do is augment theUniFFI.ktcode such that it always sets the system arch prior-to it's native lib loading. For this, it may appear like a lot of code changes but here's what is changed:ci.yml'skotlin-build-test-deploy-snapshotjob to run onubuntu-latest. This way, therelease.ymlandci.ymlare always in-sync. (You can see I was able to recreate the original issue in theci.ymlhere).TBDEX_SDK_LOG_LEVEL: debugenv var in both workflows such that we always see the what the system target detected issedcommand inside thejust bindrecipe to augment the UniFFI code-gen'd file such that the system is always detected prior-to any UniFFI interactionsSystemArchitecture.set()solution in place of the newdetectSystemArchitecture()SystemArchitecture.set()and instead you can see the change toUniFFI.ktwhich makes use of the newdetectSystemArchitecture()