eif_build, utils: cleanup #31
Conversation
The utils module is inconsistent in its public API w.r.t parsing filesystem paths (some places use String, others use PathBuf and Path). PathBuf/Path is the portable choice because it correctly handles non-utf8 characters and slashes. Because str and String implement AsRef<Path>, this change is backwards compatible. The next commit build on top of this and refactor the CLI implementation in eif_build to use typed arguments (by deriving clap::Parser), making it more readable and maintainable. Signed-off-by: Sabin Rapan <[email protected]>
clap's builder interface currently used is overly verbose and forces the caller to manually convert from String -> T every argument and manually handle errors. Switch to clap's derive interface which does that automatically. Now everything is nice and tidy: * arguments have proper data types (enums, PathBuf, Option<T>, etc). * all constraints between arguments are automatically checked, e.g. --signing-certificate and --private-key are either both Some() or both None. Additionally, update clap from 3.2 to 4.2 (the latest release with MSRV <= 1.68). This will make the transition smoother to 4.X. Signed-off-by: Sabin Rapan <[email protected]>
Move dependencies used by both eif_build and lib into workspace.dependencies so that they are compiled against the same versions and features. Additionally remove the unused clap dependency in lib. Signed-off-by: Sabin Rapan <[email protected]>
Line comments starting with /// refer to the item following them, which
means the doc comment was actually commenting use std::path::{Path,
PathBuf}.
Switch to //! which refers to the parent scope the comment lives in (the
main.rs file in this case).
Also massage the text a bit.
Signed-off-by: Sabin Rapan <[email protected]>
sabin-rapan
force-pushed
the
eif-build-cli-cleanup
branch
from
aa5d362 to
1e2692e
Compare
| [workspace.dependencies] | ||
| chrono = { version = "0.4", default-features = false, features = ["clock"]} | ||
| serde = { version = ">=1.0", features = ["derive"] } | ||
| serde_json = "1.0" | ||
| sha2 = "0.9.5" |
There was a problem hiding this comment.
I'm trying to publish eif_build stand alone on crates.io so that distributions can package it (see PR #35). Is this workspace logic compatible with crates.io packaging? I would expect that it is not, so I'd prefer we don't leverage workspace logic.
There was a problem hiding this comment.
Workspaces are fully compatible with crates.io packaging.
On this branch you can run cargo publish --dry-run which will package aws-nitro-enclaves-image-format like it's going to be stored on crates.io.
If you then look in target/package/aws-nitro-enclaves-image-format-0.2.0/Cargo.toml you'll see that workspace depedencies got expanded as if the crate is standalone (as expected).
Workspaces are just a way to share dependencies between crates that are logically coupled. All cargo commands support a -p <package name> option that allows you to run a command only for the given package (e.g. cargo publish -p eif_build)
Speaking of #35, at https://github.com/aws/aws-nitro-enclaves-image-format/blob/main/eif_build/Cargo.toml#L14 the dependency should've been specified as: aws-nitro-enclaves-image-format = { version = ">= 0.3.0", path = ".." } conforming to https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-locations.
This allows eif_build to depend on the crates.io version of aws-nitro-enclaves-image-format when compiling from crates.io, and the local path dependency when compiling locally.
The latter is useful for example when making a local change to aws-nitro-enclaves-image-format and then you want to run eif_build with that local change to see if it works, otherwise you first have to publish aws-nitro-enclaves-image-format on crates.io and then run eif_build locally against it.
2 participants
Description of changes:
This patch series addresses a few low hanging fruits in
eif_buildandutilsmodule:utilsmodule by representing all filesystem paths viaAsRef<Path>, which meansstr,String,OsString,PathBufandPathcan be used on the caller side.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.