Open jmp-0x7C0 opened 8 months ago
I'm guessing it might make sense to also add a small repro example to run xcodebuild in release mode on CI.
This is a very good idea.
Here's one solution off the top of my head
mkdir -p SwiftRustIntegrationTestRunner/test-release
touch SwiftRustIntegrationTestRunner/test-release/README.md
In the README
mention that we're confirming that the generated FFI glue does not get optimized away in release mode. Mention that we test it in its own crate so that the rests of our integration tests can run in a Debug mode (faster tests)
For inspiration on how to create a simple Swift+Rust binary, look at https://github.com/chinedufn/swift-bridge/tree/53b118d17f2f1a2922e969de528b99a2ffbc7dde/examples/async-functions
Confirm that without your changes SwiftRustIntegrationTestRunner/test-release
cannot be compiled
Confirm that with your changes SwiftRustIntegrationTestRunner/test-release
can be compiled
Run ./SwiftRustIntegrationTestRunner/test-release/build.sh
at the bottom of https://github.com/chinedufn/swift-bridge/blob/53b118d17f2f1a2922e969de528b99a2ffbc7dde/test-swift-rust-integration.sh#L1
Now we can be confident that release builds work
Just a quick idea. You might have a better solution in mind.
@chinedufn Ok I've create both a reproduction case that fails to build against the master
branch, and a case that builds successfully against this branch.
I'm not sure we want this amount of code duplication for the repro case and the success case. There's probably also a smarter way to specify which versions of the dependencies to build e.g. using the cargo
--config
flag but I couldn't get it to work so I just created two separate projects. Also if this PR would be merged as is the tests would start failing as I'm currently pointing the failing case at the master
branch.
If you have any suggestions for how to structure this code better, avoid the duplication etc. I'd be happy to make those changes.
I did have a go at reproducing the issue by just calling swiftc
directly in the same way as the async-functions
example but with release optimisation flags and code stripping enabled but I couldn't reproduce the issue. I also tried using swift build -c release
, which also built successfully. But I was immediately able to reproduce the issue by creating a Swift Package and calling xcodebuild archive
as reported by @bes.
Thanks for putting this together.
I'm not sure we want this amount of code duplication for the repro case and the success case.
We don't need to maintain two cases.
The purpose of making the failure case was for us to be sure that we weren't accidentally landing a test that would have always passed.
For example, you found that the swift build -c release
example was always passing, so that would have been a bad test.
we just need to land one of the test crates, not both. Then we can have confidence that your code for going from func
-> public func
is working as intended.
I did have a go at reproducing the issue by just calling swiftc directly in the same way as the async-functions example but with release optimisation flags and code stripping enabled but I couldn't reproduce the issue. I also tried using swift build -c release, which also built successfully. But I was immediately able to reproduce the issue by creating a Swift Package and calling xcodebuild archive.
Great. Thanks for figuring this out.
In the test crate's README Let's document what we tried and why we think it didn't work.
This helps with:
future maintainers will know what we've tried so that they don't try and do it themselves unless they think they have a new idea on how to approach it
someone in the future might have an idea of how to simplify the test by making it not need to build a Swift packages / using xcode
In short, a maintainer should be able to clearly understand why we are using a more complex solution (Swift Package + xcodebuild) instead of a simpler one (swiftc).
Are you able to complete this pull request? Looks like the main remaining work is to delete one of the test packages.
Is there anything that I can help with?
would be awesome to get this merged
@chinedufn
Are you able to complete this pull request?
Thank you for the review. I've been quite busy with other projects recently, but I should have a bit more time to get this finished next week.
Hello, hello. Anticipate having any time for this?
I had a quick stab at fixing #166, if I have understood the instructions correctly this would resolve the issue at the cost of changing the visibility of functions in the module.
@chinedufn let me know if I'm on the right track here.
So far I have only added tests for the
swift-bridge-ir
crate, I'm guessing it might make sense to also add a small repro example to runxcodebuild
in release mode on CI.