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

Preperation needed for new release #49

Open
BlackHoleFox opened this issue Jan 8, 2022 · 15 comments
Open

Preperation needed for new release #49

BlackHoleFox opened this issue Jan 8, 2022 · 15 comments

Comments

@BlackHoleFox
Copy link
Collaborator

At this point its probably getting very close, but there may be some things I haven't thought of.

The only thing left I can conjure is tests that use include_bytes!() output from OpenBSD signfiy and run through signing and verifying, and then again with embedded signatures. This would make it easier to run a "probably ok" test suite locally and then just rely on CI to do a live test against OpenBSD signify's current version.

@badboy anything else you can think of?

@BlackHoleFox BlackHoleFox changed the title Things needed for new release Preperation needed for new release Jan 8, 2022
@BlackHoleFox
Copy link
Collaborator Author

One thing for sure: to_file_encoding needs to return a Result and properly validate the comment length. Its problematic now since it lets you create signatures that we and OpenBSSD signify will reject verifying since the comment line could be too long.

@BlackHoleFox
Copy link
Collaborator Author

Another point, which was brought up by @thomcc over Discord, is that the Codeable trait is currently a bit of a footgun. It works great for the provided CLI, but may let people more easily mess-up if they use libsignify themselves. Once again quoting, consider this:

send_key_over_net(public_key: impl Codeable) { ... }

There's no typesafety at all and the compiler will happily let you beam your signing keys to the other side of the web if you give it the wrong field :p

Given that this trait is also the reason we need to unconditionally depend on liballoc, it might not be worth keeping around in libsignify and just moving it to signify so the base64 helper methods can keep working.

@badboy
Copy link
Owner

badboy commented Jan 10, 2022

Which discord is that?

@thomcc
Copy link
Contributor

thomcc commented Jan 10, 2022

Erm, "private correspondence" -- BHF and I go way back. Anyway, I don't feel that strongly about this — at work I spend a lot of time worrying about unnecessary alloc deps (our code runs on microcontrollers), so it was surprising to see one here.

@badboy
Copy link
Owner

badboy commented Jan 10, 2022

Erm, "private correspondence" -- BHF and I go way back. Anyway, I don't feel that strongly about this — at work I spend a lot of time worrying about unnecessary alloc deps (our code runs on microcontrollers), so it was surprising to see one here.

ehehe, fine!
Yeah, it's certainly a thing we can change and it sounds very sensible for me to move that out of the lib.

I need to re-read the code once again to see what we do there and if I have a good understanding of my own.

@BlackHoleFox
Copy link
Collaborator Author

I need to re-read the code once again to see what we do there and if I have a good understanding of my own.

Somewhere here tells me that the code isn't clear enough as is?

I've bee poking at how to remove the footgun of the trait on and off the last few days but so far ive ended up with either a lot of duplication or AnotherGun:tm: The duplication of the code isnt what annoys me most though tbh, its the duplication of the doc comments for all the as_bytes(&self), etc methods.

@muuvmuuv
Copy link

fyi: current cargo release 0.4.1 contains an incompatible ring version (I think it already got removed in the latest build). I had built it locally on my m1, and it seems stable to me, anything crucial needed for 0.5.0 to be released?

; cargo install signify
    Updating crates.io index
  Installing signify v0.4.1
error: failed to compile `signify v0.4.1`, intermediate artifacts can be found at `/var/folders/p_/k06bmy8s1jb7nqrdwy7r_9qc0000gn/T/cargo-install6atkpq`

Caused by:
  failed to select a version for the requirement `ring = "^0.12"`
  candidate versions found which didn't match: 0.16.20, 0.16.19, 0.16.18, ...
  location searched: crates.io index
  required by package `signify v0.4.1`

@badboy
Copy link
Owner

badboy commented Jan 27, 2022

I'll put it on my list for this weekend. We should be able to get something out by then.
We can fix up some of the bigger things mentioned above in followups if we don't get that by the weekend.

@muuvmuuv
Copy link

Cool! No hurry have a great weekend and stay safe! I am here for testing if you need an ARM M1 test. Not very familar with Rust but I am open to help where I can :)

@badboy
Copy link
Owner

badboy commented Feb 6, 2022

Well, took one weekend longer than planned but there's now a v0.5.0 published (well, also a v0.5.1 because I want the release workflow to properly run).

That shouldn't stop us from the mentioned changes though for a 0.6 release

@badboy
Copy link
Owner

badboy commented Feb 6, 2022

Oh, one note: Releasing should now be as easy as cargo release -x minor and CI takes care of building binaries as well.

@BlackHoleFox
Copy link
Collaborator Author

BlackHoleFox commented Jun 3, 2022

@badboy Tried to run that locally to release 0.5.3 with the type inference fix + new examples but it turns out cargo release needs a token by default and I have no publishing perms. I could do --no-publish and put a PR up with just the git stuff if that works for you?

@badboy
Copy link
Owner

badboy commented Jun 3, 2022

@badboy Tried to run that locally to release 0.5.3 with the type inference fix + new examples but it turns out cargo release needs a token by default and I have no publishing perms. I could do --no-publish and put a PR up with just the git stuff if that works for you?

I sent you an invite on crates.io, you should have emails about that just now.

@BlackHoleFox
Copy link
Collaborator Author

BlackHoleFox commented Jun 4, 2022

Yep, they came through, thanks.

0.5.3 has just been published. Weirdly cargo release didn't do a dry-run first so hopefully nothing was wrong :rip: Hopefully next time I'm more used to it.

@badboy
Copy link
Owner

badboy commented Jun 4, 2022

Yep, they came through, thanks.

0.5.3 has just been published. Weirdly cargo release didn't do a dry-run first so hopefully nothing was wrong :rip: Hopefully next time I'm more used to it.

Hm, I know that was a change in some recent version. It should be fine now, but you might want to update your cargo-release.

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

No branches or pull requests

4 participants