New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add resources to package #138
Conversation
Thanks a lot for working on this! Can you explain what led you to wanting this feature and how/when you expect to use it? This way we can evaluate the design and feel confident that we're taking the best approach. I don't have much Swift experience and I'm not familiar with bundle extensions. |
I'm creating a product which uses Leptos for building a server binary & client-side wasm and I want to package that into a Swift package in order to use it in a WebView-based iOS/macOS app. The server binary needs the site data (css, js, wasm etc) accessible, which is where this feature comes in handy.
You make me curious. Why are you creating this (really good) swift-bridge? See Bundling resources with a Swift package and SPM Resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks so much for contributing this.
I think your work here will close #129 .
I left some questions and a few comments about adding tests. After those are addressed this LGTM and I can merge and cut a new release.
Thanks for your great work!
@@ -100,4 +100,16 @@ fn create_package_command() -> Command<'static> { | |||
.required(true) | |||
.help("The name for the Swift Package"), | |||
) | |||
.arg( | |||
Arg::new("resource") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you land on the :
separator approach? Were there any other alternatives considered?
Also, can we leave a TODO comment that whenever clap supports array arguments we should switch to switch to using [PathBuf; 2]
instead clap-rs/clap#1682
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, it's not a great solution to use :
, but I didn't really find any easy and good solution so I went for the easy one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the TODO
@@ -18,6 +18,8 @@ pub struct CreatePackageConfig { | |||
pub out_dir: PathBuf, | |||
/// The name for the Swift package | |||
pub package_name: String, | |||
/// Additional resources to copy into the package, first PathBuf is source and second is destination | |||
pub resources: Vec<(PathBuf, PathBuf)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the semantics of the destination more clear?
Right now it's hard to tell what the destination is.
Maybe pub resources: Vec<BundleResource>
/// Describes a bundle resource to copy from some source path to some destination.
///
/// The source is a path to an existing file or directory that you wish to bundle.
///
/// The destination is a relative path that the resource will get copied to within the
/// generated Swift package's `Sources/YourLibraryName` directory.
///
/// # Examples
///
/// ```
/// # use crate::CopyBundleResourceDesc;
///
/// // If you are creating a Swift Package called "MyLibrary", this descriptor will
/// // lead to the package generator copying the `/path/to/images` directory to the
/// // the Swift package's `Sources/MyLibrary/assets/icons` directory.
/// let resource = CopyBundleResourceDesc::new("/path/to/images", "assets/icons");
/// ```
pub struct CopyBundleResourceDesc {
source: PathBuf,
destination: PathBuf,
}
impl CopyBundleResourceDesc {
/// See [`CopyBundleResourceDesc`] for documentation.
///
/// # Panics
/// Panics if the destination is an absolute path.
pub fn new(source: impl Into<PathBuf>, destination: impl Into<PathBuf>) -> Self {
let destination = destination.into();
assert!(destination.is_relative());
Self {
source: source.into(),
destination,
}
}
}
Just a quick idea, open to other approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, refactoring into CopyBundleResourceDesc done.
@@ -15,5 +15,6 @@ fn main() { | |||
)]), | |||
out_dir: PathBuf::from("swift-package-rust-library-fixture/MySwiftPackage"), | |||
package_name: "MySwiftPackage".to_string(), | |||
resources: vec![], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some resources?
- A file
resource-test.txt
- A directory with 2 files `directory-with-files/{a,b}.txt
- A directory that contains a directory that contains a file
nested/directory/test.txt
Then right after this create_package
call we can call a fn assert_resources_bundled
Which checks the "swift-package-rust-library-fixture/MySwiftPackage" Sources dir to make sure that the resources are in there.
Also, if there's a way to check that the resources are accessible on the Swift side, that would be great. Could check in here
Lines 5 to 17 in a207c24
final class swift_package_test_packageTests: XCTestCase { | |
func testPackageRun() throws { | |
XCTAssertEqual("Hello, From Rust!", hello_rust().toString()) | |
} | |
func testInstantiateSharedStruct() throws { | |
XCTAssertEqual(SomeStruct(field: 1).field, 1); | |
} | |
func testInstantiateSharedStructUnnamed() throws { | |
XCTAssertEqual(UnnamedStruct(_0: 1)._0, 1); | |
} | |
} |
Cargo.toml
Outdated
@@ -40,3 +40,4 @@ members = [ | |||
"examples/codegen-visualizer", | |||
"examples/rust-binary-calls-swift-package", | |||
] | |||
default-members = ["crates/swift-bridge-cli"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
{bridge_swift} | ||
|
||
extension Bundle {{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment in code
.takes_value(true) | ||
.value_name("SRC_PATH:DEST_PATH") | ||
.multiple_values(true) | ||
.help( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the book example to include bundling a resource?
swift-bridge/book/src/building/swift-packages/README.md
Lines 132 to 171 in 2783798
```rust | |
use std::path::PathBuf; | |
use std::collections::HashMap; | |
use swift_bridge_build::{CreatePackageConfig, ApplePlatform}; | |
fn main() { | |
swift_bridge_build::create_package(CreatePackageConfig { | |
bridge_dir: PathBuf::from("./generated"), | |
paths: HashMap::from([ | |
(ApplePlatform::IOS, "target/aarch64-apple-ios/debug/libmy_rust_lib.a".into()), | |
(ApplePlatform::Simulator, "target/universal-ios/debug/libmy_rust_lib.a".into()), | |
(ApplePlatform::MacOS, "target/universal-macos/debug/libmy_rust_lib.a".into()), | |
]), | |
out_dir: PathBuf::from("MySwiftPackage"), | |
package_name: PathBuf::from("MySwiftPackage") | |
}); | |
} | |
``` | |
#### CLI | |
You can use the `swift-bridge` CLI's `create-package` command in order to create a Swift Package. | |
First, install the CLI. | |
```bash | |
cargo install -f swift-bridge-cli | |
swift-bridge-cli --help | |
``` | |
Then, run the following to package up your generated bridges and your Rust libraries into a Swift Package. | |
```bash | |
swift-bridge-cli create-package \ | |
--bridges-dir ./generated \ | |
--out-dir MySwiftPackage \ | |
--ios target/aarch64-apple-ios/debug/libmy_rust_lib.a \ | |
--simulator target/universal-ios/debug/libmy_rust_lib.a \ | |
--macos target/universal-macos/debug/libmy_rust_lib.a \ | |
--name MySwiftPackage | |
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to the end as it is not mandatory to add resources.
pairs | ||
.into_iter() | ||
.map(|pair| { | ||
let mut split = pair.split(':'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave a comment explaining that this breaks for paths that contain :
, i.e. "/path/to/my\:file.txt".
Or just assert_eq(pair.split(":").count(), 2)
or something like that?
Since I would imagine we'll want to eventually switch to clap-rs/clap#1682 , not fixing this bug is fine.
I'm sure that way fewer than 1% of people would ever run into this in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #138 (comment)
@@ -328,12 +350,46 @@ let package = Package( | |||
), | |||
.target( | |||
name: "{package_name}", | |||
dependencies: ["RustXcframework"]) | |||
dependencies: ["RustXcframework"], | |||
resources: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the package string generation out into its own function
swift-bridge/crates/swift-bridge-build/src/package.rs
Lines 313 to 335 in 03de7ea
let package_swift = format!( | |
r#"// swift-tools-version:5.5.0 | |
import PackageDescription | |
let package = Package( | |
name: "{package_name}", | |
products: [ | |
.library( | |
name: "{package_name}", | |
targets: ["{package_name}"]), | |
], | |
dependencies: [], | |
targets: [ | |
.binaryTarget( | |
name: "RustXcframework", | |
path: "RustXcframework.xcframework" | |
), | |
.target( | |
name: "{package_name}", | |
dependencies: ["RustXcframework"]) | |
] | |
) | |
"# | |
); |
Then add a test to the bottom of this file that generates the package description for the CreatePackageConfig
that has resources and asserts on the contents of the generated string?
My main concern here is that right now the writing to disk / process spawning stuff is tightly coupled to things that it doesn't need to be (I know this was here before you, so not saying that you did that!).
So I'm asking to add a test for this resources thing so that we at least begin take a small step in the right direction and make it easier to make our Swift package creation more powerful over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did make an effort to follow your (current) style of programming, imitating how things are done. I think it is better if you, as the owner of this project, set the new precedence instead of me as I will probably not do it as you like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually all of the package stuff was contributed by a contributor.
However, makes sense to me!
Ok thanks for explaining and thanks for the links. Read them. They were helpful.
Thanks for the kindness, this made me smile. |
I'm afraid that I won't have more time to spend on this. Feel free to reject the PR if it is not up to your standards. |
Ok thanks for the heads up. It's just missing tests. I can take get them added when I have time. Thanks for your work! |
Thanks to you and sorry if my previous comments were a bit snappy. |
4cda9a7
to
29d54eb
Compare
Closing this as my focus is now on UniFFI |
Added a
--resource
option to the cli which copies resources into the package and adds the appropriate Bundle extension.