Skip to content
This repository was archived by the owner on Feb 3, 2023. It is now read-only.

Conversation

@jamesray1
Copy link
Contributor

@jamesray1 jamesray1 commented Nov 11, 2019

PR summary

Aims to close #946

By changing EntryWithHeader to a ChainPair tuple struct (with getters etc.), and using a try_from_header_and_entry() as a constructor and not a new() constructor, this introduces a log of changes, and not only with renaming. (If people prefer I can rename back to EntryWithHeader, etc., however I made the change as I thought it would be best to make it clear that it is backwards incompatible, and it is also shows intent more clearly since the header and entry must match in a pair, rather than being any header and any entry.) The change also broke a couple of tests, since the header and entry in them didn't match, and these tests have been fixed.

I also ran cargo fmt, using a recent nightly, rather than from within the next shell. I think this is why there are a lot of changes with formatting.

  hcr git:(946-feature-chain-pair) rustc --version
rustc 1.40.0-nightly (38048763e 2019-11-06)

Contextual notes: Please bear in mind that work has been much less productive and frustratingly slow as I'm yet to sort out connecting my desktop to the internet, and my laptop only has 4 GB RAM so is slow to use and compile and/or freezes, and adjusting the swappiness etc. hasn't been effective (high swappiness—too laggy, low—too freeze-prone). This is combined with fixing merge conflicts, and further compiling, adjustments etc. Admittedly I should've arranged switching to my desktop ASAP much more quickly, and not have tried to persist with the CI build and then the laptop for so long. I'm expected to have fixed, fast, fibre internet working on Dec 2 (rather than using mobile data), so can then switch to using my desktop!

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

@jamesray1 jamesray1 changed the title [On ice] Add incomplete work on chain-pair [WIP] Add incomplete work on chain-pair Nov 15, 2019
@jamesray1 jamesray1 changed the title [WIP] Add incomplete work on chain-pair [WIP] Add chain-pair to address 946 Nov 21, 2019
@jamesray1
Copy link
Contributor Author

Still working on this, just offline. I'm not pushing to not trigger as many CI builds.

@jamesray1
Copy link
Contributor Author

jamesray1 commented Nov 25, 2019

Contextual notes:
cargo build and fmt passed for me within nh (nix-shell https://holochain.love) so I don't know why fmt fails with CI.
Nevermind, I didn't run nix-shell --run hc-rust-test and hn-rust-clippy
Tomorrow I'll get a Linux compatible (and hopefully 802.11ac compatible) WiFi dongle from a local store. I'll test that with using internet from mobile data and phone hotspot. Will defer to setting up NBN as a more expensive fallback.

@thedavidmeister
Copy link
Contributor

i like the naming of "pair", i always have, but it seems to be very confusing to other people so 🤷‍♂️

@maackle
Copy link
Collaborator

maackle commented Dec 12, 2019

Looks good to me, if tests pass...

I noticed there is no diff to remove EntryWithHeader itself, I wonder if there's still other code that uses it?

@jamesray1
Copy link
Contributor Author

@thedavidmeister I can do a CTRL+F to rename it if preferred. Calling it just Pair isn't exactly clear what it is a pair of at a glance, ChainPair is a bit more suggestive. I suppose you could call it EntryHeaderPair but it is a bit verbose. I think I mentioned above that EntryWithHeader may mislead that the Entry and Header could be any Entry and Header, which was true as this wasn't made impossible.

@maackle The tests were passing before the latest upstream changes were merged. Currently I can't test the latest builds since CircleCI says I have exceeded the resource usage for my plan, so tests don't build, and my laptop freezes when trying to test locally, even with swappiness on 100. Internet still hasn't been setup, there's ongoing delays. I thought I changed all EntryWithHeaders, but I'll check again.

@jamesray1
Copy link
Contributor Author

jamesray1 commented Dec 12, 2019

Notes: I've fixed more tests locally, but can't get nix-shell --run hc-rust-test to complete as it freezes. I've ordered a laptop (with 16 GB of RAM, i7 3.0 GHz, 256 GB SSD). It'll arrive ETA 20-23, so if the fixed internet isn't setup by then for my desktop (which has no WiFi) then I can use that. Only $319, used condition, so pretty good value. I should've ordered one months ago, but didn't see that I could get one at that low a cost before.

I might be able to get tests to pass but it's very slow and inefficient...

@thedavidmeister
Copy link
Contributor

yeah, i found this too, rust development on anything under 16GB is not fun :/

@jamesray1
Copy link
Contributor Author

It's failing locally on linking, presumably due to the RAM being insufficient, even with swappiness set to 100:

➜  hcr git:(946-feature-chain-pair) ✗ alias nt
nt='nix-shell --run hc-rust-test'
➜  hcr git:(946-feature-chain-pair) ✗ nt
<snip>
   Compiling holochain_conductor_lib v0.0.40-alpha1 (/home/jr/hcr/crates/conductor_lib)
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/nix/store/2h5dvmcmd885m1h06cxvm2cz3fvzrxzy-rust-1.41.0-nightly-2019-11-15-1bd30ce2a/lib/rustlib/x86_64-unknown-linux-gnu/lib" <snip>
  = note: collect2: fatal error: ld terminated with signal 9 [Killed]
          compilation terminated.

A related question: https://stackoverflow.com/questions/5682854/why-is-the-linker-terminating-on-me-when-i-build-clang, although I don't think there's a way for me to fix this other than using a VM or my desktop with more RAM.

Note: I actually cancelled the laptop order and instead ordered a wireless router that can work in client mode (this, so I can use my hotspot with the desktop via Ethernet.

@thedavidmeister
Copy link
Contributor

right, this is the main reason our CI tasks are all set to xlarge :(

i don't have a solution for you (otherwise i would have fixed it for our CI) right now, but even if we fix the RAM issue it is relatively well known that compiling rust on a higher end machine runs much faster than something resource constrained

RAM and CPUs make a big different i have found

@jamesray1
Copy link
Contributor Author

jamesray1 commented Dec 18, 2019

No worries, I just received the router today, and am just getting my mobile data back. So I should be able to use the internet on the desktop soon, and thus get everything set up and usable.

@thedavidmeister
Copy link
Contributor

@jamesray1 nice, unfortunately i think there is a price tag associated with rust work, i also had to buy several new machines to hit windows, linux, etc.

@jamesray1
Copy link
Contributor Author

Pushing from laptop, going to pull to desktop to test and finish up.

@jamesray1 jamesray1 changed the title [Review needed] Add chain-pair to prevent a header and entry mismatch [WIP] Add chain-pair to prevent a header and entry mismatch Dec 20, 2019
@jamesray1
Copy link
Contributor Author

See #2006

@jamesray1 jamesray1 closed this Dec 21, 2019
@jamesray1 jamesray1 changed the title [WIP] Add chain-pair to prevent a header and entry mismatch Add chain-pair to prevent a header and entry mismatch Dec 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

impossible states: EntryWithHeader to ChainItem

3 participants