build: one cargo workspace for the repo #133

mattyg · 2024-12-19T00:08:55Z

Resolves #124

mattyg · 2024-12-19T00:11:09Z

crates/guest/src/guest.rs

@@ -51,6 +51,7 @@ where
 /// - Deserialize whatever bytes we can import from the host after calling the host function
 /// - Return a `Result` of the deserialized output type `O`
 #[inline(always)]
+#[allow(improper_ctypes_definitions)]
 pub fn host_call<I, O>(
    f: unsafe extern "C" fn(usize, usize) -> DoubleUSize,


This gives a warning that u128 is not an ffi safe interface. Since our CI requires that clippy passes with no warnings, this became blocking.

Yeah this is a funny one. It's helpful to allow 32-bit or 64-bit builds but it would be invalid with u128. I'm not aware of a type that include the first two but not the other. Reasonable to ignore I think

mattyg · 2024-12-19T00:12:38Z

crates/guest/src/guest.rs

@@ -159,26 +160,3 @@ macro_rules! try_ptr {
        }
    }};
 }
-
-#[cfg(test)]
-pub mod tests {


These tests were never being run.

If they are worth keeping, should they be moved into the host crate and only run when the feature flag error_as_host is not enabled? I don't totally understand the utility since it seems like we always have error_as_host enabled.

The tests seem reasonable enough to me, I probably would try to keep them.

If we always use the feature flag though, I don't see a reason not to remove that?

Ah okay I was confused. This is about the default behavior of passing a string into wasm_error!().

By default this will produce a WasmErrorInner::Guest("my error"), in hdi it follows this behavior. When the error_as_host feature is enabled, it will produce a WasmErrorInner:Host("my error"), in holochain and holochain_wasmer_host it follows this behavior.

Because the wasm_error!() is defined in crates/common, I moved both this test and the mirroring test (in crates/host/src/guest.rs) into crates/common/src/result.rs. I put each test behind the feature flags required for their behavior. Let me know if that looks good.

mattyg · 2024-12-19T00:14:08Z

scripts/test-wasmer_sys_dev.sh

@@ -6,18 +6,14 @@ export WASMER_BACKTRACE=1

 # static tests
 cargo fmt --check
-( cd test && cargo fmt --check )
-( cd crates/guest && cargo fmt --check )


crates/guest and test are now in the root workspace, so no need to check separately

mattyg · 2024-12-19T00:14:14Z

scripts/test-wasmer_sys_dev.sh

 cargo clippy -- --deny warnings
-( cd test && cargo clippy -- --deny warnings )


test is now in the root workspace

mattyg · 2024-12-19T00:14:46Z

scripts/test-wasmer_sys_prod.sh

 cargo test --no-default-features --features error_as_host,wasmer_sys_prod ${1-} -- --nocapture

-# test that everything builds
-cargo build --release --manifest-path test/test_wasm/Cargo.toml --target wasm32-unknown-unknown


I think these are redundant -- we don't need to run them for each feature flag.

mattyg · 2024-12-19T00:15:15Z

test/Cargo.toml

@@ -34,9 +34,6 @@ name = "test"
 crate-type = ["cdylib", "rlib"]
 path = "src/test.rs"

-[profile.release]
-debug = true


Moved to the workspace-level cargo.toml so it is actually applied

…ort shadowing

mattyg · 2024-12-19T00:33:16Z

crates/host/src/module.rs

@@ -372,48 +372,3 @@ impl ModuleCache {
        Ok(module)
    }
 }
-
-#[cfg(test)]
-mod tests {


moved these into crates/host/src/module/wasmer_sys.rs to avoid the clippy warning "warning: private item shadows public glob re-export". I don't totally understand it.

Some naming clash with a pub use a_module::*. We had masses of these in Holochain at some point with the endless re-exports for "convenience" that ended up causing headaches with where types actually lived. This seems fine here

…same level within "test-crates" dir

… wasms used for tests vs actual test implementations

mattyg · 2024-12-19T19:57:12Z

test-crates/wasm_core/Cargo.toml

Renamed test_wasm to wasm_core to use the same directory and crate naming schema as the other wasm crates. Intended to clarify that this is an example wasm that is used in tests, not actually tests.

mattyg · 2024-12-19T19:57:51Z

test-crates/tests/src/test.rs

@@ -391,7 +391,7 @@ pub mod tests {
        match err {
            Err(runtime_error) => assert_eq!(
                WasmError {
-                    file: "src/wasm.rs".into(),
+                    file: "test-crates/wasm_core/src/wasm.rs".into(),


Different file path since this is part of the root workspace now.

test-crates/tests/build.rs

mattyg · 2024-12-19T19:59:39Z

test-crates/wasm_memory/src/wasm.rs

All formatting changes

mattyg · 2024-12-19T19:59:45Z

test-crates/wasm_io/src/wasm.rs

All formatting changes

mattyg · 2024-12-19T19:59:50Z

test-crates/wasm_core/src/wasm.rs

All formatting changes

mattyg · 2024-12-19T22:44:59Z

scripts/bump_version.sh

This is not being used.

mattyg · 2024-12-20T00:00:49Z

crates/common/src/result.rs

+            // i.e. "test-crates\\wasm_core\\src/wasm.rs"
+            //
+            // To remedy this we normalize the formatting here.
+            file: file!().replace('\\', "/").to_string(),


This is the only issue I found noting this: rust-lang/libs-team#503

Alternatively we could just keep the inconsistent path formatting. Or I could use a real "path canonicalization" lib instead of a replace().

ThetaSinner

This is great! Genuinely wasn't sure it would work out going this far with the change, but it looks like it's come out really nicely.

I'm going to check this out and give it a look just to make sure it looks like it does in my head after the review but really just a couple of minor comments :)

ThetaSinner · 2024-12-20T11:28:45Z

Yeah looks great locally!

Possible the test-creates doesn't need to be flat? The tests and common crates could be directly under test-crates then the wasm_* could go into a wasm directory instead of having name prefixes. But that really would be fiddling about :D Just a thought, not a request

…or shared deps

…kspace for shared deps" This reverts commit 21d0d6a.

…crate where it is defined

ThetaSinner

Really nice! Thank you taking this on :)

jost-s

Awesome! ❤️

build: one cargo workspace for the repo

7cf1906

mattyg commented Dec 19, 2024

View reviewed changes

chore: move tests from module -> wasmer_sys to avoid public glob rexp…

2798fd8

…ort shadowing

mattyg commented Dec 19, 2024

View reviewed changes

mattyg added 4 commits December 18, 2024 17:39

chore: root workspace includes test crate

8ba0b55

chore: changelog

89d85ea

Merge branch 'main' into chore/one-workspace

7f8ae9c

chore: missed a cargo.lock

b3787b8

mattyg marked this pull request as ready for review December 19, 2024 00:47

mattyg added 4 commits December 18, 2024 17:48

chore: move building test wasms before testing root workspace

50bd5a2

refactor: instead of nesting crates within "test" crate, move all to …

067fb8e

…same level within "test-crates" dir

chore: rename test_wasm to test_wasm_core to clarify which crates are…

cecca25

… wasms used for tests vs actual test implementations

chore: changelog clarity

1287eba

mattyg commented Dec 19, 2024

View reviewed changes

test-crates/tests/build.rs Outdated Show resolved Hide resolved

mattyg commented Dec 19, 2024

View reviewed changes

test-crates/wasm_memory/src/wasm.rs Outdated

Copy link

Member Author

mattyg Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All formatting changes

mattyg commented Dec 19, 2024

View reviewed changes

test-crates/wasm_io/src/wasm.rs Outdated

Copy link

Member Author

mattyg Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All formatting changes

mattyg commented Dec 19, 2024

View reviewed changes

test-crates/wasm_core/src/wasm.rs Outdated

Copy link

Member Author

mattyg Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All formatting changes

mattyg added 2 commits December 19, 2024 13:01

fix: path to test crate

71cc10d

chore: fix bench paths, remove unused script

2e43788

mattyg commented Dec 19, 2024

View reviewed changes

scripts/bump_version.sh Outdated

Copy link

Member Author

mattyg Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not being used.

mattyg added 3 commits December 19, 2024 16:07

fix: windows tests

c9e7d6e

wip: debug windows file path

e8274e7

fix: normalize file path formatting on windows and explain with comment

81ae4a1

chore: fmt

a386df5

mattyg commented Dec 20, 2024

View reviewed changes

mattyg requested a review from a team December 20, 2024 00:18

ThetaSinner reviewed Dec 20, 2024

View reviewed changes

mattyg added 6 commits December 20, 2024 10:02

chore: long arg

44cdb17

chore: move example wasms into test-crates/wasms dir, use workspace f…

21d0d6a

…or shared deps

Revert "chore: move example wasms into test-crates/wasms dir, use wor…

83eec5b

…kspace for shared deps" This reverts commit 21d0d6a.

chore: move example wasms into test-crates/wasms dir

d2d18c7

chore: move tests of default behavior of 'wasm_error!()' into common …

abcdab4

…crate where it is defined

chore: fmt + clippy

40088c0

mattyg requested a review from ThetaSinner December 20, 2024 18:19

ThetaSinner approved these changes Dec 20, 2024

View reviewed changes

jost-s approved these changes Dec 20, 2024

View reviewed changes

mattyg merged commit d136c6c into main Dec 20, 2024
13 checks passed

mattyg deleted the chore/one-workspace branch December 20, 2024 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: one cargo workspace for the repo #133

build: one cargo workspace for the repo #133

mattyg commented Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024

ThetaSinner Dec 20, 2024

mattyg Dec 19, 2024 •

edited

Loading

ThetaSinner Dec 20, 2024

mattyg Dec 20, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024

mattyg Dec 19, 2024

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading

ThetaSinner Dec 20, 2024

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024

mattyg Dec 19, 2024

mattyg Dec 19, 2024

mattyg Dec 19, 2024

mattyg Dec 19, 2024

mattyg Dec 20, 2024

ThetaSinner left a comment

ThetaSinner commented Dec 20, 2024

ThetaSinner left a comment

jost-s left a comment

		cargo clippy -- --deny warnings
		( cd test && cargo clippy -- --deny warnings )

build: one cargo workspace for the repo #133

build: one cargo workspace for the repo #133

Conversation

mattyg commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyg Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyg Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

mattyg Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyg Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

mattyg Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyg Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThetaSinner left a comment

Choose a reason for hiding this comment

ThetaSinner commented Dec 20, 2024

ThetaSinner left a comment

Choose a reason for hiding this comment

jost-s left a comment

Choose a reason for hiding this comment

mattyg commented Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 20, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading

mattyg Dec 19, 2024 •

edited

Loading