tls-codec #56

franziskuskiefer · 2021-09-23T12:26:25Z

Moving code from https://github.com/openmls/tls-codec/

I'm sure there are a couple things in here we want to touch. Some things I noticed

There's currently only an MIT license. We have to add Apache
We should agree on a badge style
I added CI, let's see if that's working. I also added Windows and macOS targets.
Are there binaries or why do we have a Cargo.lock?
There are no-std CI targets. tls-codec wasn't written with no-std in mind so far.

I'm sure there's more. Let me know what you think @tarcieri

Moving code from https://github.com/openmls/tls-codec/ here

tarcieri · 2021-09-23T14:48:36Z

Are there binaries or why do we have a Cargo.lock?

We've generally checked in Cargo.lock to keep the build reproducible and make it easier to spot regressions in dependencies since we always have a known good CI. That goes against the Rust convention of "check Cargo.lock in for binaries, don't for libs" but with Dependabot it's generally kept up to date and the PRs it opens make it quite easy to figure out exactly what's breaking the build (often MSRV-breaking changes in dependencies)

There are no-std CI targets. tls-codec wasn't written with no-std in mind so far

You'll need to remove those then. FWIW all of our other crates support no_std. Perhaps I can take a look after it's merged. I imagine it wouldn't be too difficult if liballoc is used instead?

franziskuskiefer · 2021-09-24T06:59:37Z

You'll need to remove those then. FWIW all of our other crates support no_std. Perhaps I can take a look after it's merged. I imagine it wouldn't be too difficult if liballoc is used instead?

Yeah I tried to merge all CI targets you had in for the other crates and what I had in before. I think adding no_std shouldn't be a big deal.

- drop thumbv7em-none-eabi because it requires no_std - add target to windows and macos builds

tarcieri · 2021-09-24T14:46:33Z

Seems hung trying to get a builder for test-mac-windows. I even restarted it after two hours and it's still hanging...

franziskuskiefer · 2021-09-24T15:01:27Z

Seems hung trying to get a builder for test-mac-windows. I even restarted it after two hours and it's still hanging...

Strange. It says

Can't find any online and idle self-hosted runner in the current repository, account/organization that matches the required labels: 'windows-latest , macos-latest'
Waiting for a self-hosted runner to pickup this job...

I haven't seen this before. Sounds like an org-wide issue.

tarcieri · 2021-09-24T16:21:34Z

Weird. We do have macOS/Windows CI elsewhere, for example here:

https://github.com/RustCrypto/asm-hashes/blob/master/.github/workflows/sha2.yml#L51-L91

Looking at your config though, I'm not sure why it would be trying to use a self-hosted runner.

Maybe remove it for now and we can circle back on it?

They fail for some reason on the RustCrypto repository right now

tarcieri · 2021-09-24T18:22:11Z

Cool, CI passing now at least 👍

tls-codec/tls_codec/benches/tls_vec.rs

tarcieri · 2021-09-27T14:44:42Z

tls-codec/tls_codec/src/arrays.rs

+impl_array!(
+    1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26,
+    27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50,
+    51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74,
+    75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98,
+    99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117,
+    118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128
+);


Seems like something which would nicely be replaced with const generics, whenever you're ready to bump MSRV to 1.51.

Indeed, we should replace this with const generics. I always forget that they are actually a thing now.

tarcieri

One minor nit but I think this is fine for an initial import.

I now see this is a bit more std-dependent than I realized due to the extensive use of std::io::{Read, Write}.

tarcieri · 2021-09-27T14:52:20Z

I also noticed the directory name is tls-codec, but the crate name is tls_codec? Perhaps they should match.

Co-authored-by: Tony Arcieri <bascule@gmail.com>

franziskuskiefer · 2021-09-28T07:45:29Z

I now see this is a bit more std-dependent than I realized due to the extensive use of std::io::{Read, Write}.

Indeed. I think it's only used with byte slices right now. But the idea is that the crate should allow serializing straight to whatever Write target (and the same for deserializing).

franziskuskiefer · 2021-09-30T11:04:53Z

@tarcieri let me know how you want to handle this. I'd be happy to merge and address remaining issues in follow ups. (I don't have access yet and can't merge 😔 )
I filed #64 and #65 for that.

tarcieri · 2021-09-30T12:55:41Z

I think the remaining issues in this PR:

GH Actions workflow name doesn't match crate name: tls-codec.yml vs tls_codec
Directory structure of crate vs proc macro

On the latter, the der crate uses:

/der: location of the der crate
/der/derive: location of the der_derive crate

I'm not particularly attached to this layout, but I think we should be consistent with how we structure custom derive.

So I would either suggest:

/tls_codec: location of the tls_codec crate
/tls_codec/derive: location of the tls_codec_derive crate

Or: we could switch to a flat layout that looks like this:

/der
/der_derive
/tls_codec
/tls_codec_derive

I think there are good arguments for a flat layout regarding discoverability of the custom derive crates and not having indirection between the crate name and it's location, but I could go either way.

franziskuskiefer · 2021-10-06T19:01:04Z

So I would either suggest:

* `/tls_codec`: location of the `tls_codec` crate

* `/tls_codec/derive`: location of the `tls_codec_derive` crate

Sorry for being a little slow on this. Too many other things need attention 😬
But let me go with this layout for now. The flat layout increases discoverability but also pollutes the high-level directory a little.

tarcieri · 2021-10-08T16:03:40Z

Not sure what you want to do with the badges, but I'm fine to merge as-is and we can figure that out separately

tls-codec initial commit

e0e8fbd

Moving code from https://github.com/openmls/tls-codec/ here

franziskuskiefer added 2 commits September 24, 2021 09:05

fix ci

60fc347

- drop thumbv7em-none-eabi because it requires no_std - add target to windows and macos builds

don't try to run Windows on Ubunutu

3975222

franziskuskiefer added 2 commits September 24, 2021 20:05

no mac and windows CI for now

abad375

They fail for some reason on the RustCrypto repository right now

Merge branch 'RustCrypto:master' into tls-codec

4e41274

add Apache license

7a485fd

franziskuskiefer marked this pull request as ready for review September 27, 2021 14:32

tarcieri reviewed Sep 27, 2021

View reviewed changes

tls-codec/tls_codec/benches/tls_vec.rs Outdated Show resolved Hide resolved

tarcieri reviewed Sep 27, 2021

View reviewed changes

tarcieri approved these changes Sep 27, 2021

View reviewed changes

franziskuskiefer and others added 2 commits September 28, 2021 09:32

Update tls-codec/tls_codec/benches/tls_vec.rs

2b57c5d

Co-authored-by: Tony Arcieri <bascule@gmail.com>

rename to tls_codec

9519a86

This was referenced Sep 30, 2021

tls_codec: no_std support #64

Closed

Use const generics in tls_codec array.rs #65

Closed

franziskuskiefer added 3 commits October 8, 2021 08:25

Merge branch 'master' into tls-codec

04008a0

new direcotry structure

517ecf8

fix directory in gh action for tls_codec

9e241fa

tarcieri approved these changes Oct 8, 2021

View reviewed changes

tarcieri merged commit 8df9765 into RustCrypto:master Oct 9, 2021

franziskuskiefer deleted the tls-codec branch October 11, 2021 07:52

tls-codec #56

tls-codec #56

Uh oh!

Conversation

franziskuskiefer commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarcieri commented Sep 23, 2021

Uh oh!

franziskuskiefer commented Sep 24, 2021

Uh oh!

tarcieri commented Sep 24, 2021

Uh oh!

franziskuskiefer commented Sep 24, 2021

Uh oh!

tarcieri commented Sep 24, 2021

Uh oh!

tarcieri commented Sep 24, 2021

Uh oh!

Uh oh!

tarcieri Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

franziskuskiefer Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

tarcieri left a comment

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Sep 27, 2021

Uh oh!

franziskuskiefer commented Sep 28, 2021

Uh oh!

franziskuskiefer commented Sep 30, 2021

Uh oh!

tarcieri commented Sep 30, 2021

Uh oh!

franziskuskiefer commented Oct 6, 2021

Uh oh!

tarcieri commented Oct 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

franziskuskiefer commented Sep 23, 2021 •

edited

Loading