From e6b424eb128b8dde0d386bcc31b11c8d371d1a44 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:05:22 -0800 Subject: [PATCH 1/9] feat: Use newer resolver in workspace. Cargo introduced a new dependency resolver in Rust 1.51.0 (released March 25, 2021). This resolver improves handling of cases where a dependency exists multiple times in the dependency graph with different features set, and is intended to resolve some edge cases that had been identified in the old resolver that would produce surprising or unintended behavior. Those edge cases don't necessarily impact us today, but it's good to switch to the newer resolver explicitly anyway. Signed-off-by: Andrew Lilley Brinker --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index 8a37d50..064b9cc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,4 +1,5 @@ [workspace] members = ["omnibor", "gitoid"] +resolver = "2" From d71691dbee3813d76419b340ac6bd7d1fc873f9d Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:07:57 -0800 Subject: [PATCH 2/9] feat: Make GitOid::new_invalid pub(crate) For the C API to handle errors, we have a constructor for GitOid called new_invalid that produces an _invalid_, _unusable_ GitOid. Specifically, this bad GitOid has a length of 0, and the hash value is a zero-initialized array. When FFI functions expected to return a GitOid fail, they can return in invalid GitOid instead, and we also expose an FFI function for that GitOid. However, this is all specific to the C FFI, and only needs to be called by our own FFI functions when they fail. It should _never_ be called directly by any Rust or C users of the GitOid crate, so we can mark it pub(crate) to avoid exposing the symbol to the linker. Signed-off-by: Andrew Lilley Brinker --- gitoid/src/gitoid.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitoid/src/gitoid.rs b/gitoid/src/gitoid.rs index 33d780b..42dd2d5 100644 --- a/gitoid/src/gitoid.rs +++ b/gitoid/src/gitoid.rs @@ -55,7 +55,7 @@ impl GitOid { /// /// This construction should _only_ be used for error-handling purposes /// when using the `gitoid` crate over FFI. - pub fn new_invalid() -> Self { + pub(crate) fn new_invalid() -> Self { GitOid { hash_algorithm: HashAlgorithm::Sha1, object_type: ObjectType::Blob, From 164283a40595ae28086352bfd95b683b6e9c33bb Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:10:11 -0800 Subject: [PATCH 3/9] fix: Add missing conditional compilation for unix imports The `ffi/gitoid.rs` file included some `std::os::unix` imports unconditionally, which wasn't caught in CI but causes builds of the crate to fail on non-Unix systems. This is a limitation of our CI today, and in the future it would be great to have WIndows and other CI platforms set up so we can catch these issues pre-merge. This commit adds conditional compilation annotations so the imports are only done on the appropriate platform. Signed-off-by: Andrew Lilley Brinker --- gitoid/src/ffi/gitoid.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/gitoid/src/ffi/gitoid.rs b/gitoid/src/ffi/gitoid.rs index b9801be..f4c150c 100644 --- a/gitoid/src/ffi/gitoid.rs +++ b/gitoid/src/ffi/gitoid.rs @@ -15,8 +15,14 @@ use std::ffi::CStr; use std::ffi::CString; use std::fs::File; use std::io::BufReader; +#[cfg(target_family = "unix")] use std::os::unix::prelude::FromRawFd; +#[cfg(target_family = "unix")] use std::os::unix::prelude::RawFd; +#[cfg(target_family = "windows")] +use std::os::windows::io::FromRawHandle; +#[cfg(target_family = "windows")] +use std::os::windows::prelude::RawHandle; use std::ptr::null; use std::ptr::null_mut; use std::slice; @@ -64,6 +70,8 @@ pub extern "C" fn gitoid_invalid(gitoid: *const GitOid) -> c_int { /// Construct a new `GitOid` from a buffer of bytes. /// +/// `content_len` is the number of elements, not the number of bytes. +/// /// `content_len` times 8 (byte size) must be less than or equal to the /// maximum size representable with an unsigned integer at the size used by /// the ISA (32-bit or 64-bit usually). @@ -74,7 +82,7 @@ pub extern "C" fn gitoid_invalid(gitoid: *const GitOid) -> c_int { pub extern "C" fn gitoid_new_from_bytes( hash_algorithm: HashAlgorithm, object_type: ObjectType, - content: *const u8, + content: *mut u8, content_len: usize, ) -> GitOid { let output = catch_panic(|| { @@ -277,5 +285,5 @@ pub extern "C" fn gitoid_str_free(s: *mut c_char) { return; } - unsafe { CString::from_raw(s) }; + let _ = unsafe { CString::from_raw(s) }; } From 01b9f9b410de9e3445680e1526e5202af0b61d4b Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:10:44 -0800 Subject: [PATCH 4/9] fix: Hide C test executables When we're running the C test code for the FFI testing, we build some binaries which we don't want to accidentally check in, so we add them to the .gitignore here. Signed-off-by: Andrew Lilley Brinker --- gitoid/test/.gitignore | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 gitoid/test/.gitignore diff --git a/gitoid/test/.gitignore b/gitoid/test/.gitignore new file mode 100644 index 0000000..d0c6e4c --- /dev/null +++ b/gitoid/test/.gitignore @@ -0,0 +1,2 @@ +c_test.exe +c_test From 49feee843f440269a7ead04af51b862d15b0653b Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:12:52 -0800 Subject: [PATCH 5/9] feat: Improve clarity / reliability of FFI tests We use a C program to validate the FFI, and the previous version did an odd thing for testing `gitoid_new_from_bytes`, where it used `sscanf` through a string into a buffer. This actually did't appear to workas intended, and I've opted to replace it with a literal buffer containing some bytes instead. I also took the opportunity to add more validity checks to some other tests, and to simplify construction of the test array by introducing a new TEST macro. Signed-off-by: Andrew Lilley Brinker --- gitoid/test/c/test.c | 52 ++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/gitoid/test/c/test.c b/gitoid/test/c/test.c index b5b3f9c..b68ab14 100644 --- a/gitoid/test/c/test.c +++ b/gitoid/test/c/test.c @@ -5,45 +5,38 @@ #include "gitoid.h" #define LEN(arr) (sizeof(arr) / sizeof(arr[0])); +#define TEST(NAME) {.name = #NAME, .fn = NAME} void test_gitoid_new_from_str() { GitOid gitoid = gitoid_new_from_str(HashAlgorithm_Sha1, ObjectType_Blob, "hello world"); + assert(!gitoid_invalid(&gitoid)); assert(gitoid.len == 20); assert(gitoid.value[0] == 149); } void test_gitoid_new_from_bytes() { - // Section that creates the byte array was heavily inspired by [1]. - // - // Does not do error checking, and is intended solely for test purposes. - // The length of `byte_array` is equal to the length of `string` plus one, - // to make space for the nul-terminator. - // - // [1]: https://stackoverflow.com/a/3409211/2308264 - const char *string = "hello_world"; - const char *position = string; - unsigned char byte_array[12]; - size_t size = LEN(byte_array); - for (size_t count = 0; count < size; ++count) { - sscanf(position, "%2hhx", &byte_array[count]); - position += 2; - } - uint8_t byte_array_length = sizeof byte_array; + unsigned char bytes[] = {0x00, 0x01, 0x02, 0x03, + 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0A, 0x0B, + 0x0C, 0x0D, 0x0E, 0x0F}; + uint8_t bytes_len = LEN(bytes); GitOid gitoid = gitoid_new_from_bytes( HashAlgorithm_Sha1, ObjectType_Blob, - &byte_array_length, - *byte_array + bytes, + bytes_len ); + assert(!gitoid_invalid(&gitoid)); assert(gitoid.len == 20); - assert(gitoid.value[0] == 130); + assert(gitoid.value[0] == 182); } void test_gitoid_new_from_url() { char *url = "gitoid:blob:sha256:fee53a18d32820613c0527aa79be5cb30173c823a9b448fa4817767cc84c6f03"; GitOid gitoid = gitoid_new_from_url(url); + assert(!gitoid_invalid(&gitoid)); assert(gitoid.len == 32); assert(gitoid.value[0] == 254); } @@ -51,6 +44,7 @@ void test_gitoid_new_from_url() { void test_gitoid_get_url() { char *url_in = "gitoid:blob:sha256:fee53a18d32820613c0527aa79be5cb30173c823a9b448fa4817767cc84c6f03"; GitOid gitoid = gitoid_new_from_url(url_in); + assert(!gitoid_invalid(&gitoid)); char *url_out = gitoid_get_url(&gitoid); assert(strncmp(url_in, url_out, 83) == 0); gitoid_str_free(url_out); @@ -58,23 +52,23 @@ void test_gitoid_get_url() { void test_gitoid_hash_algorithm_name() { GitOid gitoid = gitoid_new_from_str(HashAlgorithm_Sha1, ObjectType_Blob, "hello world"); + assert(!gitoid_invalid(&gitoid)); const char *hash_algorithm = gitoid_hash_algorithm_name(gitoid.hash_algorithm); assert(strncmp(hash_algorithm, "sha1", 4) == 0); } void test_gitoid_object_type_name() { GitOid gitoid = gitoid_new_from_str(HashAlgorithm_Sha1, ObjectType_Blob, "hello world"); + assert(!gitoid_invalid(&gitoid)); const char *object_type = gitoid_object_type_name(gitoid.object_type); assert(strncmp(object_type, "blob", 4) == 0); } void test_gitoid_validity() { - // Notice the SHA type is invalid. char *validity_url = "gitoid:blob:sha000:fee53a18d32820613c0527aa79be5cb30173c823a9b448fa4817767cc84c6f03"; GitOid gitoid = gitoid_new_from_url(validity_url); assert(gitoid_invalid(&gitoid)); - // Also test the error message reporting. char *expected_msg = "string is not a valid GitOID URL"; char error_msg[256]; gitoid_get_error_message(error_msg, 256); @@ -91,14 +85,14 @@ typedef struct test { int main() { setvbuf(stdout, NULL, _IONBF, 0); - test_t tests[7] = { - {.name = "gitoid_new_from_str", .fn = test_gitoid_new_from_str}, - {.name = "gitoid_new_from_bytes", .fn = test_gitoid_new_from_bytes}, - {.name = "gitoid_new_from_url", .fn = test_gitoid_new_from_url}, - {.name = "gitoid_get_url", .fn = test_gitoid_get_url}, - {.name = "gitoid_hash_algorithm_name", .fn = test_gitoid_hash_algorithm_name}, - {.name = "gitoid_object_type_name", .fn = test_gitoid_object_type_name}, - {.name = "gitoid_validity", .fn = test_gitoid_validity}, + test_t tests[] = { + TEST(test_gitoid_new_from_str), + TEST(test_gitoid_new_from_bytes), + TEST(test_gitoid_new_from_url), + TEST(test_gitoid_get_url), + TEST(test_gitoid_hash_algorithm_name), + TEST(test_gitoid_object_type_name), + TEST(test_gitoid_validity), }; size_t n_tests = LEN(tests); From b67f550c4aca81100550627b32456c347e4f33ec Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:14:00 -0800 Subject: [PATCH 6/9] feat: Bump dependency versions to latest. This commit bumps the sha1, sha2, and url dependencies to their latest versions. For sha1 and sha2 this is just a patch version bump. For url it's a minor bump. Nothing broke with these updates, and in generally Cargo would select the latest compatible anyway. This is just good for clarity to the resolver. Signed-off-by: Andrew Lilley Brinker --- gitoid/Cargo.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gitoid/Cargo.toml b/gitoid/Cargo.toml index c79d828..58c5bfa 100644 --- a/gitoid/Cargo.toml +++ b/gitoid/Cargo.toml @@ -15,6 +15,6 @@ crate-type = ["rlib", "cdylib"] [dependencies] hex = "0.4.3" -sha1 = "0.10.4" -sha2 = "0.10.5" -url = "2.2.2" +sha1 = "0.10.6" +sha2 = "0.10.8" +url = "2.4.1" From 0e6550fc6aad70c62c9b071ac16fa9195f3aac3f Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:15:20 -0800 Subject: [PATCH 7/9] fix: Remove unused dependencies. The `omnibor` crate doesn't actually use the hex, im, sha1, or sha2 crates anymore, and doesn't need to. All of that functionality is now in the `gitoid` crate that it depends on, so these deps can be removed. Note that the `omnibor` crate still fails to compile on Windows due to a prior issue, which can be resolved (and the deps version bumped) with the publication of a new version of the `gitoid` crate. Signed-off-by: Andrew Lilley Brinker --- omnibor/Cargo.toml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/omnibor/Cargo.toml b/omnibor/Cargo.toml index 55f5a4a..a13d8be 100644 --- a/omnibor/Cargo.toml +++ b/omnibor/Cargo.toml @@ -12,7 +12,3 @@ version = "0.1.7" [dependencies] gitoid = "0.1.5" -hex = "0.4.3" -im = "15.1.0" -sha1 = "0.10.4" -sha2 = "0.10.5" From 0f5c25bc25b26f93bf07e846cc7c08284da53b12 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:27:42 -0800 Subject: [PATCH 8/9] fix: Fix broken compilation of `omnibor` crate. Yes, this "fixes" everything by deleting the (minimal) contents of the crate. However, the existing contents of the crate... 1. Do not do anything meaningful. 2. Overlap in a confusing way with the much-more-developed `gitoid` crate. 3. Are set to be rewritten to reflect updates to the OmniBOR specification. I personally feel it's better to be real about the state of this crate, which is that it is not meaningfully useful in the new OR old form, and move on from there, rather than publish an API that isn't meaningfully useful and is also broken. Signed-off-by: Andrew Lilley Brinker --- omnibor/src/lib.rs | 120 +-------------------------------------------- 1 file changed, 1 insertion(+), 119 deletions(-) diff --git a/omnibor/src/lib.rs b/omnibor/src/lib.rs index 86d8d16..70a23d6 100644 --- a/omnibor/src/lib.rs +++ b/omnibor/src/lib.rs @@ -1,120 +1,2 @@ -use gitoid::GitOid; -use im::{HashSet, Vector}; -/// A [persistent][wiki] collection of [git oids][git_scm]. -/// -/// Why persistent? While Rust and the borrow checker is great about ownership and -/// mutation, always knowing that a Ref will not change if passed as a parameter -/// to a function eliminates a class of errors. -/// -/// [wiki]: https://en.wikipedia.org/wiki/Persistent_data_structure -/// [git_scm]: https://git-scm.com/book/en/v2/Git-Internals-Git-Objects -#[derive(Clone, PartialOrd, Eq, Ord, Debug, Hash, PartialEq)] -pub struct OmniBor { - git_oids: HashSet, -} - -impl FromIterator for OmniBor { - /// Create an OmniBor from many GitOids - fn from_iter(gitoids: T) -> Self - where - T: IntoIterator, - { - let me = OmniBor::new(); - me.add_many(gitoids) - } -} - -impl OmniBor { - /// Create a new instance - #[allow(clippy::new_without_default)] - pub fn new() -> Self { - Self { - git_oids: HashSet::new(), - } - } - - /// Create a OmniBor from many GitOids - pub fn new_from_iterator(gitoids: I) -> Self - where - I: IntoIterator, - { - let me = OmniBor::new(); - me.add_many(gitoids) - } - - /// Add a `GitOid` hash to the `OmniBor`. - /// - /// Note that this creates a new persistent data structure under the hood. - pub fn add(&self, gitoid: GitOid) -> Self { - self.add_many([gitoid]) - } - - /// Append many `GitOid`s and return a new `OmniBor` - pub fn add_many(&self, gitoids: I) -> Self - where - I: IntoIterator, - { - let mut updated = self.git_oids.clone(); // im::HashSet has O(1) cloning - for gitoid in gitoids { - updated = updated.update(gitoid); - } - Self { git_oids: updated } - } - - /// Return the `Vector` of `GitOid`s. - pub fn get_oids(&self) -> HashSet { - self.git_oids.clone() // im::HashSet as O(1) cloning. - } - - /// Get a sorted `Vector` of `GitOid`s. - /// - /// In some cases, getting a sorted `Vector` of oids is desirable. - /// This function (cost O(n log n)) returns a `Vector` of sorted oids - pub fn get_sorted_oids(&self) -> Vector { - let mut ret = self.git_oids.clone().into_iter().collect::>(); - ret.sort(); - ret - } -} - -#[cfg(test)] -mod tests { - use gitoid::{GitOid, HashAlgorithm, ObjectType::Blob}; - use im::vector; - - use super::*; - - #[test] - fn test_add() { - let oid = GitOid::new_from_str(HashAlgorithm::Sha256, Blob, "Hello"); - assert_eq!(OmniBor::new().add(oid).get_sorted_oids(), vector![oid]) - } - - #[test] - fn test_add_many() { - let mut oids: Vector = vec!["eee", "Hello", "Cat", "Dog"] - .into_iter() - .map(|s| GitOid::new_from_str(HashAlgorithm::Sha256, Blob, s)) - .collect(); - - let da_bom = OmniBor::new().add_many(oids.clone()); - oids.sort(); - assert_eq!(da_bom.get_sorted_oids(), oids); - } - - #[test] - fn test_add_gitoid_to_gitbom() { - let input = "hello world".as_bytes(); - - let generated_gitoid = GitOid::new_from_bytes(HashAlgorithm::Sha256, Blob, input); - - let new_gitbom = OmniBor::new(); - let new_gitbom = new_gitbom.add(generated_gitoid); - - assert_eq!( - "fee53a18d32820613c0527aa79be5cb30173c823a9b448fa4817767cc84c6f03", - new_gitbom.get_sorted_oids()[0].hash().as_hex() - ) - } -} +// TODO(abrinker): Complete first draft of the rewrite of this crate. From 16f0028123b1b75fc155b486e8aa760393ceaed5 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Tue, 14 Nov 2023 08:39:10 -0800 Subject: [PATCH 9/9] fix: Moved, cleaned up cbindgen.toml The previous version of cbindgen.toml had a couple of issues: 1. It was specifically for the `gitoid` crate but it was in the root of the project repository. 2. It still included mentioned of the old GitBOM name. 3. Really, it should refer to the crate, not to the project that manages the crate, so it should be GitOID, not OmniBOR, as the correction. This commit moves and fixes the cbindgen configuration, and updates the C testing makefile to refer to the moved file. Signed-off-by: Andrew Lilley Brinker --- gitoid/Makefile | 2 +- cbindgen.toml => gitoid/cbindgen.toml | 24 ++---------------------- 2 files changed, 3 insertions(+), 23 deletions(-) rename cbindgen.toml => gitoid/cbindgen.toml (65%) diff --git a/gitoid/Makefile b/gitoid/Makefile index 951165f..a23a3ac 100644 --- a/gitoid/Makefile +++ b/gitoid/Makefile @@ -4,7 +4,7 @@ # It is intended to be easy for newcomers to the project to understand. # Variables for cbindgen. -CBINDGEN_CONFIG="../cbindgen.toml" +CBINDGEN_CONFIG="./cbindgen.toml" # Variables for the header file. INCLUDE_DIR="./include" diff --git a/cbindgen.toml b/gitoid/cbindgen.toml similarity index 65% rename from cbindgen.toml rename to gitoid/cbindgen.toml index eafad2a..a733025 100644 --- a/cbindgen.toml +++ b/gitoid/cbindgen.toml @@ -2,7 +2,7 @@ language = "C" # Wrap generated file in an include guard. -include_guard = "gitbom_h" +include_guard = "gitoid_h" # Note the version of cbindgen used to generate the header. include_version = true @@ -20,7 +20,7 @@ style = "both" header = """ /** * @file - * @brief "GitBom" + * @brief "GitOID" */ """ @@ -40,23 +40,3 @@ autogen_warning = "/* Warning, this file is autogenerated by cbindgen. Don't mod # Prefix enum variants with the name of the overall type. prefix_with_name = true - - -#============================================================================== -# Parsing Rules -#------------------------------------------------------------------------------ - -[parse] - -# Parse dependencies. -#parse_deps = true -#include = [] - -#============================================================================== -# Macro Expansion Rules -#------------------------------------------------------------------------------ - -#[parse.expand] - -# Expand macros for `gitoid`. -#crates = ["gitoid"]