Skip to content
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

Preserialization dylib, Connor's work #2218

Merged
merged 49 commits into from
Apr 24, 2023
Merged

Preserialization dylib, Connor's work #2218

merged 49 commits into from
Apr 24, 2023

Conversation

maackle
Copy link
Member

@maackle maackle commented Apr 13, 2023

Summary

Running @Connoropolous's #2203 on CI

TODO:

  • CHANGELOG(s) updated with appropriate info
  • Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

@maackle
Copy link
Member Author

maackle commented Apr 18, 2023

@Connoropolous also could you just write (here is fine) a line for the CHANGELOG about this change?

@Connoropolous
Copy link
Contributor

@Connoropolous as for actually running the tests, I'm not sure what you're running into that wouldn't allow it. I did update the wasmer dep, was that what was stopping you?

Holochain being a complex project, i dont know what commands ro run

@Connoropolous
Copy link
Contributor

@Connoropolous also could you just write (here is fine) a line for the CHANGELOG about this change?

Yes sure. When by? Having a busy day.

@Connoropolous
Copy link
Contributor

I made a commit to make this test pass but we should revert it if you think we should update the test to do wasm serialization f777f60

That makes sense. Leave it in i suggest

@maackle
Copy link
Member Author

maackle commented Apr 18, 2023

Yes sure. When by? Having a busy day.

Passing tests and your changelog are the only things left before merging. So whenever. @Connoropolous

@maackle maackle requested a review from jost-s April 18, 2023 23:27
@maackle
Copy link
Member Author

maackle commented Apr 18, 2023

oh yeah, and another review would help!


#[derive(Serialize, Deserialize, Hash, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
#[cfg_attr(feature = "test_utils", derive(arbitrary::Arbitrary))]
pub struct WasmZome {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this in zome types? this crate is for types we need compiled into both the wasm host and guest, but this seems like something we only need on the host?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it's been in this file for years, it just got shifted up a few lines in this PR. But yes, sounds like it should probably be moved to holochain_types

Copy link
Member Author

Choose a reason for hiding this comment

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

@thedavidmeister I agree but there's a bunch of stuff that would have to be rearranged so I'll add some TODOs but want to leave this out of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@Connoropolous
Copy link
Contributor

@maackle
should there be a changelog item for each changed crate?

@maackle
Copy link
Member Author

maackle commented Apr 19, 2023

@Connoropolous only for the public-facing stuff. I know that's a bit fuzzy since it depends who's using what crate but at least something in hc_bundle with the high level details. We're not too meticulous about making each crate's Changelog be complete on its own.

@Connoropolous
Copy link
Contributor

Connoropolous commented Apr 19, 2023

@maackle

hc dna pack command now takes --dylib-ios option, which produces iOS optimized Zomes. These can be utilized by passing dylib: PathBuf for Zome configurations in dna.yaml files and other data structures based on ZomeManifest where Zomes are constructed.

ZomeManifest now takes a dylib argument, with the type Option<PathBuf>. It can be safely ignored in cases other than trying to execute on native iOS. It is used with artifacts produced by hc dna pack when it has been called with the --dylib-ios option.

@Connoropolous
Copy link
Contributor

@thedavidmeister can we get a clear on your change request?

@maackle maackle enabled auto-merge (squash) April 24, 2023 21:03
@maackle maackle merged commit 07d9be3 into develop Apr 24, 2023
12 checks passed
@maackle maackle deleted the preserialization-dylib branch April 24, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants