Update recv, change to this-error, address clippy warnings, fix unsafe pack_vec #37

bruingineer · 2025-10-03T09:48:36Z

updates recv for both linux and windows to use read() instead of read_exact() since the ipv4 tests would hang when using read_exact() [BUG] ipv4 tests failing on windows 10 #35
switch from error-chain to this-error [Feature] Remove error-chain and use thiserror #17
address all clippy warnings
use resize() in pack_vec() instead of reserve_exact() and set_length() [BUG] Undefined behavior in pack_vec #33
add docker-linux folder that has a dockerfile and bash script used to run the ipv4 tests on a linux container. (Windows containers have proven to be very troublesome and I don't see the Windows ip tests being run in a github action anytime soon). [Feature] Run automatic tests in multiple platforms #28 [Feature] Auto Run Integration Testing #6
updates example naming in cargo.toml to get rid of intellisense errors

* Update receive.rs * use different read for windows vs non windows os * remove cfg check since impl is per OS already * Update receive.rs

* use different read for windows vs non windows os * remove cfg check since impl is per OS already

* use different read for windows vs non windows os * remove cfg check since impl is per OS already * update linux recv to make tests pass * Update receive.rs

* use thiserror instead of error-chain refactor to use thiserror instead of error-chain * remove old error file * Update ipv6_tests.rs * Update main.rs * Update main.rs * update empty data test * use different read for windows vs non windows os * remove cfg check since impl is per OS already * Update .gitignore * update linux recv to make tests pass * Update receive.rs * use thiserror instead of error-chain refactor to use thiserror instead of error-chain * remove old error file * Update ipv6_tests.rs * Update main.rs * Update main.rs * update empty data test * update windows recv error * Update receive.rs

Update packet.rs

see https://doc.rust-lang.org/cargo/reference/cargo-targets.html#examples

Correct cargo example target name

* correct example target in cargo see https://doc.rust-lang.org/cargo/reference/cargo-targets.html#examples * Update Cargo.toml * use thiserror instead of error-chain refactor to use thiserror instead of error-chain * remove old error file * Update ipv6_tests.rs * Update Cargo.toml * Update Cargo.toml * Update main.rs * Update main.rs * Update receive.rs * update empty data test * update empty data test * Update receive.rs Update receive.rs Update receive.rs * Revert "Update receive.rs" This reverts commit 7a9ce39. * clippy updates * clippy fixes * clippy lifetimes * fix rebase * more advanced clippy fixes * Update sacn_parse_pack_error.rs * clippy fixes * update tests * clippy

bruingineer · 2025-11-05T07:58:30Z

any comments?
What is the preferred github action test organization? Put all tests (linux/windows/linux ipv4) under one "test" action, or separate per OS?
I think this should be a new version of the library (v0.11.0). @lschmierer @Lan2u @stewartadam

stewartadam · 2025-11-05T10:11:26Z

@Lan2u happy to help review and merge once added as maintainer if you're still interested - lmk

lschmierer · 2025-11-05T20:44:56Z

Hi @stewartadam, I would be happy to invite you to the group or transfer ownership of the project.
I am the original author of this lib, but (while I remember it to being fun) frankly had no involvement in years.
While your Github profile, blog and microsoft relations seem solid, I am usually very careful in transferring ownership to unknown accounts (the risk of supply chain attacks is real, even though not huge for users of this lib).
I just sent you a request on LinkedIn, I think a 2nd trust channel gives me enough confidence to add you to the group or project!

Finally, I am very happy that people are interested in this after all :)

stewartadam · 2025-11-06T08:37:17Z

Appreciate the caution around it - I replied on LinkedIn to confirm my identity.

stewartadam

@bruingineer thanks for your work on this! Looking over the code this looks good, I'm currently travelling so don't have access to my home dev workstation to run a build + test in full -- after doing so when I return later this week I'll give it an approval.

I've requested some minor changes to the Dockerfile (clearing apt cache and adding sysctl -p) that would be ideal, but happy to merge this without and apply those fixes afterwards.

Understandably swapping out error-chain for thiserror is going to lead to some large diffs, but combined with the clippy and whitespace fixes this is quite a large diff -- if it is easy for you to break apart the clippy/formatting changes, I'd be happy to merge the non-functional changes separately from the functional ones as well for clarity in the repo history.

docker-linux/Dockerfile

stewartadam · 2025-11-11T04:34:34Z

docker-linux/Dockerfile

+RUN apt-get update && apt-get install iproute2 -y
+
+WORKDIR /work
+COPY Cargo.toml Cargo.lock ./


Similarly, not blocking merge but if you're interested: I would love to get something like cargo-chef in here so that a single change to the source code doesn't trigger a recompile of every dependency in the docker image - see https://hackmd.io/jgkoQ24YRW6i0xWd73S64A

Happy to merge this and tackle that separately though.

to do in separate pr

docker-linux/run_tests.sh

bruingineer · 2025-11-11T05:07:30Z

I hear you about the clips changes large diffs on top of the error changes. I made the mistake of trying to fix some clippy warnings while I fixed the error use in related sections. I had a branch of some clippy fixes, but then merge conflicts started to appear between the code that used this error vs error chain.

I’ll take a look at my commit history to see if I can pull things out. Or if it’s easy to make 2 new branches now.

stewartadam · 2025-11-11T07:46:07Z

Don't sweat it if it's gonna take you more than a few mins. If you had the commits already split up that's one thing but no need to spend extra time redoing the branches 😊

sysctl -p requires elevation. tests pass without updating configs. we do not require them at this time.

bruingineer · 2025-11-11T09:53:37Z

Sounds good. It is ready for review, testing, and merging now. Running the Dockerfile in a GitHub Action will be a separate PR.

stewartadam

Had a chance to look at this in more detail and test it - looks great, thank you for working on this! Looks like the CI configuration is experiencing some issues preventing merge, working with repo owners to get me permissions to fix.

bruingineer added 15 commits September 18, 2025 16:59

Update packet.rs

c127d52

Receiver change read exact to read (#3)

b0a7761

* Update receive.rs * use different read for windows vs non windows os * remove cfg check since impl is per OS already * Update receive.rs

Receiver change read exact to read (#4)

0bb9732

* use different read for windows vs non windows os * remove cfg check since impl is per OS already

Update .gitignore

5e964e2

Receiver change read exact to read (#5)

c648b7c

* use different read for windows vs non windows os * remove cfg check since impl is per OS already * update linux recv to make tests pass * Update receive.rs

Merge pull request #7 from bruingineer/fix-ub-in-pack_vec

c61460c

Update packet.rs

correct example target in cargo

bfc346a

see https://doc.rust-lang.org/cargo/reference/cargo-targets.html#examples

Update Cargo.toml

e3733c9

Update Cargo.toml

fb28ec3

Update Cargo.toml

c5ba2bb

Merge branch 'stable' into correct-cargo-example-target-name

b273a7c

Merge pull request #8 from bruingineer/correct-cargo-example-target-name

2fe7171

Correct cargo example target name

add linux dockerfile

8ab9009

stewartadam mentioned this pull request Nov 6, 2025

Offer for co-maintainership #38

Closed

stewartadam reviewed Nov 11, 2025

View reviewed changes

bruingineer added 3 commits November 11, 2025 01:35

use lf in bash script

740ded7

simple test docs and dockerfile docs

c15ab9f

remove ipv4 configs

50f5541

sysctl -p requires elevation. tests pass without updating configs. we do not require them at this time.

bruingineer added 2 commits November 11, 2025 01:53

Update run_tests.sh

556eadb

fix testing ip addresses

f13a347

stewartadam approved these changes Nov 15, 2025

View reviewed changes

stewartadam merged commit 673b879 into RustLight:main Nov 15, 2025
2 checks passed

This was referenced Nov 15, 2025

[BUG] Undefined behavior in pack_vec #33

Closed

[Feature] Remove error-chain and use thiserror #17

Closed

Update recv, change to this-error, address clippy warnings, fix unsafe pack_vec #37

Update recv, change to this-error, address clippy warnings, fix unsafe pack_vec #37

Uh oh!

Conversation

bruingineer commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruingineer commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stewartadam commented Nov 5, 2025

Uh oh!

lschmierer commented Nov 5, 2025

Uh oh!

stewartadam commented Nov 6, 2025

Uh oh!

stewartadam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stewartadam Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bruingineer Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bruingineer commented Nov 11, 2025

Uh oh!

stewartadam commented Nov 11, 2025

Uh oh!

bruingineer commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stewartadam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bruingineer commented Oct 3, 2025 •

edited

Loading

bruingineer commented Nov 5, 2025 •

edited

Loading

bruingineer commented Nov 11, 2025 •

edited

Loading

stewartadam left a comment •

edited

Loading