Remove `schemars` pin from manifest #1696

tcharding · 2023-03-06T00:53:24Z

This has grown due to now including pinning work also done in #1736, I decided to do this because the PRs conflict and doing it all here saves accidentally getting out of sync. And #1764 requires this PR.

Patch 1 is unchanged
Patch 2 now fixes pinning in bitcoin and hashes CI scripts and in the docs of both as well as the manifest stuff relating to schemars - phew.

Fix: #1687

Kixunil

because it does not take into account the version of cargo in use.

That's not right, version of cargo is irrelevant. The reason it's wrong is if a different crate requires higher version those will be incompatible leading to at best duplication, at worst impossibility to compile.

hashes/Cargo.toml

Kixunil · 2023-03-06T07:25:43Z

You also need cargo update -p schemars --precise 0.8.3

apoelstra · 2023-03-06T14:23:33Z

I didn't see this before opening #1697. But:

We don't actually need to pin schemars to compile with 1.41.1. We do need to pin url (2.2.2) and form_urlencoded (1.0.1).
However Jeremy's test fails, on all compilers, with schemars >0.8.3.

So I dunno, maybe the pin was correct.

tcharding · 2023-03-06T22:02:16Z

That's not right, version of cargo is irrelevant. The reason it's wrong is if a different crate requires higher version those will be incompatible leading to at best duplication, at worst impossibility to compile.

Cool, thanks for the explanation.

tcharding · 2023-03-06T22:57:33Z

We don't actually need to pin schemars to compile with 1.41.1.

I discovered this too but the versions of schemars used by hashes and extended_tests/schemars have to be the same when running extended_tests/schemars tests, and the tests fail without pinning so I'm guessing dowstream users will hit problems too if they do not pin. Did not verify that assumption.

apoelstra · 2023-03-07T01:52:59Z

The new README text is kinda misleading. You do need to pin schemars ... but this has nothing to do with the MSRV. For the MSRV you just need to pin url and formurll_encoded.

tcharding · 2023-03-07T03:16:19Z

I'm a bit confused then,

rust-bitcoin

cargo test --features=schemars - works fine
cd hashes && cargo +1.41.1 test --features=schemars - works fine (can't use this command in workspace root with +1.41.1 for some reason)

extended_tests/schemars_test

cargo test fails unless schemars is pinned with --precise 0.8.3
cargo +1.41.1 test fails unless schemars, url, form_urlencdode are all pinned

tcharding · 2023-03-07T03:23:54Z

The answer doesn't matter really, I could have easily made a mistake while checking every combination I could think of.

I changed hashes/README.md to be a hopefully loose enough to not be misleading and correct enough to help users out (based on your comment @apoelstra)

apoelstra · 2023-03-07T12:54:47Z

@tcharding to use 1.41.1 you need to pin url and form_urlencoded. There is no need to do this unless you are using 1.41.

To use schemars at all you need to pin schemars. Which begs the question of why we are unpinning it at all.

Kixunil · 2023-03-07T16:34:59Z

To use schemars at all you need to pin schemars

Do you mean the test failure after doing the following?

vim Cargo.toml - remove the pin
cd extended_tests/schemars
vim Cargo.toml - remove the pin
cargo test

I printed the error:

ValidationError { msg: "Invalid type.", instance: Some(String("6cfb35868c4465b7c289d7d5641563aa973db6a929655282a7bf95c8257f53ef")), schema: Some(Object {"$schema": String("http://json-schema.org/draft-07/schema#"), "description": String("Output of the SHA256t hash function."), "items": Array [Object {"maxLength": Number(64), "minLength": Number(64), "pattern": String("[0-9a-fA-F]+"), "type": String("string")}], "maxItems": Number(1), "minItems": Number(1), "title": String("Hash_for_TestHashTag"), "type": String("array")}), instance_path: [], schema_path: ["type"] }

If I understand it correctly the type is OK, just jsonschema_valid thinks it's not? (In hurry so good chance I'm wrong.)

apoelstra · 2023-03-07T17:11:05Z

Do you mean the test failure after doing the following?

Yes, I mean that test failure. I wasn't able to print the error because somehow nothing returned by the failing assert implements Debug. And I don't know what jsonschema_valid is or how it relates to schemars.

jeandudey · 2023-03-07T18:57:40Z

The schema generated is wrong because schemars is interpreting the struct tuple as a JSON array.

Can be easily solved by:

#[cfg(feature = "schemars")]
impl<T: Tag> actual_schemars::JsonSchema for Hash<T> {
    fn schema_name() -> String {
        "Hash".to_owned()
    }

    fn json_schema(gen: &mut actual_schemars::gen::SchemaGenerator) -> actual_schemars::schema::Schema {
        crate::util::json_hex_string::len_32(gen)
    }
}

To use schemars at all you need to pin schemars.

I think its better to unpin it and every other dependency and just pin using Cargo.lock with the commands to pin dependencies since it prevents rust-bitcoin for being packaged easily, for example, in GNU Guix as it provides [email protected] which is not compatible with the current version spec, so one has to go and patch the Cargo.toml of the crate to be able to use it with newer versions. So that needs to be done for every pinned crate where a new version is available.

I think that's true for other distros that provide only a single version of a package instead of including all of the minor versions needed.

jeandudey · 2023-03-07T18:59:16Z

That would also effectively remove the need to pin other crates that we don't use directly in the Cargo.toml and allows to rename actual-schemars to schemars.

jeandudey · 2023-03-07T19:04:28Z

hashes/Cargo.toml

 # because 1.0.8 does not build with Rust 1.41.1 (because of useage of `Arc::as_ptr`).
 dyn-clone = { version = "<=1.0.7", default_features = false, optional = true }


If schemars if being unpinned this should be too IMO, by removing it completely, should allow for #1696 (comment)

apoelstra · 2023-03-07T19:33:40Z

@jeandudey thanks for the fix!

We are all onboard with removing pins (all of them). The issue is that nobody besides you in this conversation has ever actually used schemars, the person who originally advocated for its inclusion is no longer working on it, and they evidently made some breaking change between 0.8.3 and 0.8.4 which is causing our only test case to break.

ffee8ad Bump version to v0.30.0 (Tobin C. Harding) Pull request description: Add changelog notes and bump the version number to v0.30.0. ## TODO - pre-merge - [x] Release `bitcoin_hashes` 0.12: #1694 - [x] Release secp 0.27: rust-bitcoin/rust-secp256k1#588 - rust-bitcoin/rust-secp256k1#590 - [x] Update `secp256k1` dependency to use newly released v0.27: #1714 - [x] Merge - ~#1696 - #1695 - #1111 - [x] If time permits merge these: - #1710 - #1705 - #1713 - [x] Set the release date in changelog header - [x] And merge these: - #1721 - #1720 - #1719 - #1717 ## TODO - post release - [ ] Release the blogpost: rust-bitcoin/www.rust-bitcoin.org#2 - ~Set the date in the blog post to match the date 0.30 is released~ ACKs for top commit: sanket1729: reACK ffee8ad Kixunil: ACK ffee8ad apoelstra: ACK ffee8ad Tree-SHA512: b0ea113ee1726fd9b263d0e01fe14bd544c007c05a9ac43b6c2d4edbeef3bb3ad456b061ef086626e1e1b27a0cda49cb6bc28aac3ad1691d72ffe00400ed5b45

tcharding · 2023-03-28T02:48:45Z

Implemented all review suggestions, thanks crew.

Kixunil

ACK d02a984a1906fd5175e0e7eed7e54e0fed2d554f

apoelstra · 2023-03-28T12:54:52Z

hashes/contrib/test.sh

@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash


This should be /usr/bin/env bash. Not all systems have bash in /bin.

Sure thing, can do.

apoelstra · 2023-03-28T12:55:59Z

hashes/contrib/test.sh

-    (cd extended_tests/schemars && cargo test)
+    pushd "$REPO_DIR/hashes/extended_tests/schemars" > /dev/null
+    cargo test
+    popd


In d02a984a1906fd5175e0e7eed7e54e0fed2d554f:

May want to redirect popd to null as well if you're redirecting pushd.

I added the redirect to null, FTR on my system popd does not output anything anyways. Are there different implementations of popd that do?

Interesting. On my system popd also outputs the directory that it's changing to. I have bash 5.1. Maybe this is an obscure setting somewhere.

derrrr me no Linux - I'm using zsh as my shell.

apoelstra · 2023-03-28T12:56:42Z

Done reviewing d02a984a1906fd5175e0e7eed7e54e0fed2d554f. Looks good, but I would like the shebang to be fixed.

tcharding · 2023-03-28T21:41:17Z

Fixed shebang and added redirect as requested. No other changes.

The derived implementation of `JsonSchema` recently stopped working as described here: rust-bitcoin#1696 (comment) Implement the suggestion.

tcharding · 2023-03-31T00:34:27Z

Please see PR description for what is in the last force push. Don't be surprised If I've confused you, I confused myself there for a bit.

apoelstra

ACK 5d29cd6daeda258b4f7ac999aac40d8ff72c2358

Kixunil

Not sure if I should ACK, am I developing OCD?

hashes/src/sha256t.rs

Kixunil · 2023-03-31T05:02:54Z

hashes/contrib/test.sh

@@ -17,6 +17,12 @@ if cargo --version | grep nightly >/dev/null; then
    NIGHTLY=true
 fi

+# Pin dependencies as required if we are using MSRV toolchain.
+if cargo --version | grep "1\.48"; then


Some sort of version comparison would be nice. sort can compare versions, so this hack should work:

if "$(echo -e "$(cargo | cut -d ' ' -f 2)"'\n1.48.0' | sort -V | tail -n 1)" = "1.48.0"; then

I agree, but I don't think it belongs in this PR -- in a future thing we should maybe try to rewrite this script entirely to clean it up (though I think it's mostly in good shape) and to make it easier/more intuitive to run locally.

Yeah, I didn't ACK because of the attribute.

tcharding · 2023-04-03T04:31:56Z

Actually, it should be removed because of #1763 Sorry about the noise.

I don't follow - this is a feature guard, nothing to do with docs?

EDIT: Ignore this comment by me, I was confused.

Done as is single patch to make sure all the docs and CI are in sync and correct. We currently pin the `schemars` dependency using `<=0.8.3` as well as a the `dyn-clone` transient dependency in the manifest (`hashes` and the extended test crate). This is incorrect because it makes usage of the crate klunky (or possibly impossible) if downstream users wish to use a later version of `schemars`. Observe also that we do not have to pin `schemars`, we do however have to pin the `serde` crate if either `serde` or `schemars` features are enabled. Do so in CI and document in the readme file within hashes. Currently we have a pin remaining from the old MSRV (`syn` due to use of `matches!`). Fix pinning by: - Remove pin in manifest for `schemars` - Fix pinning for MSRV in CI and docs (this includes documenting pinning requirements for `schemars` feature because it is related to the other pin of `serde`) in both `hashes` readme and main repo readme.

tcharding · 2023-04-19T00:18:06Z

~~Hey @Kixunil can you take a look at this one please, I think your suggestion ways misplaced unless I'm misunderstanding you. #1696 (comment)~~

Is there a reason for you not ack'ing this one @Kixunil?

Force push is to fix multiple types s/serder/serde in the commit log and also in one of the readmes. No other changes.

Kixunil

ACK 6c61e10

hashes/src/sha256t.rs

Kixunil · 2023-04-19T06:42:10Z

hashes/extended_tests/schemars/README.md

+To run the tests with the MSRV you will need to pin some dependencies:
+
+- `cargo update -p serde --precise 1.0.156`
+- `cargo update -p syn --precise 1.0.107`


I think this isn't needed anymore?

I'm unsure. The serde line definitely is, but syn you may be right that we can drop. In any case, can wait for a followup PR.

Verified, we no longer need the syn pin. Will do as follow up because this PR is blocking other work. Thanks

Yep, that's what I meant.

apoelstra

ACK 6c61e10

53d75c7 extended_tests: Remove stale docs (Tobin C. Harding) Pull request description: We no longer need to pin `syn`; remove stale docs. This is a follow up to #1696 ACKs for top commit: Kixunil: ACK 53d75c7 apoelstra: ACK 53d75c7 Tree-SHA512: 9e939860cfb3731942e92a611632f0b24acc88e06a96e905ee2ae50e489d1610f7b9c79e20384196482aad4d5da182f6f919d18726e079a98a667532806d5aae

tcharding changed the title ~~hashes: Remove schemars pin~~ WIP: hashes: Remove schemars pin Mar 6, 2023

Kixunil reviewed Mar 6, 2023

View reviewed changes

hashes/Cargo.toml Outdated Show resolved Hide resolved

apoelstra mentioned this pull request Mar 6, 2023

hashes: remove version pin on schemars. #1697

Closed

tcharding force-pushed the 03-36-schemars branch from 8ea5bec to 9c8a855 Compare March 6, 2023 22:51

tcharding changed the title ~~WIP: hashes: Remove schemars pin~~ Remove schemars pin Mar 6, 2023

tcharding changed the title ~~Remove schemars pin~~ Remove schemars pin from manifest Mar 6, 2023

tcharding force-pushed the 03-36-schemars branch 4 times, most recently from 196fcc3 to a970c72 Compare March 6, 2023 23:35

tcharding mentioned this pull request Mar 7, 2023

Release tracking PR - v0.30.0 #1699

Merged

8 tasks

tcharding force-pushed the 03-36-schemars branch from a970c72 to be68ee0 Compare March 7, 2023 03:23

apoelstra mentioned this pull request Mar 7, 2023

release bitcoin_hashes 0.12 #1694

Merged

jeandudey reviewed Mar 7, 2023

View reviewed changes

tcharding marked this pull request as draft March 8, 2023 23:31

Kixunil previously approved these changes Mar 28, 2023

View reviewed changes

apoelstra reviewed Mar 28, 2023

View reviewed changes

tcharding dismissed Kixunil’s stale review via 5d29cd6 March 28, 2023 21:41

tcharding force-pushed the 03-36-schemars branch from d02a984 to 5d29cd6 Compare March 28, 2023 21:41

tcharding mentioned this pull request Mar 30, 2023

Test with minimal dependency versions #1764

Merged

hashes: Implement JsonSchema for sha256t::Hash<T>

c8e38d6

The derived implementation of `JsonSchema` recently stopped working as described here: rust-bitcoin#1696 (comment) Implement the suggestion.

tcharding force-pushed the 03-36-schemars branch from 5d29cd6 to 9043c55 Compare March 31, 2023 00:29

tcharding added the P-high High priority label Mar 31, 2023

tcharding mentioned this pull request Mar 31, 2023

Improve README #1736

Merged

apoelstra previously approved these changes Mar 31, 2023

View reviewed changes

Kixunil reviewed Mar 31, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via 6c61e10 April 19, 2023 00:17

tcharding force-pushed the 03-36-schemars branch from 9043c55 to 6c61e10 Compare April 19, 2023 00:17

Kixunil approved these changes Apr 19, 2023

View reviewed changes

apoelstra approved these changes Apr 19, 2023

View reviewed changes

apoelstra merged commit 8440d80 into rust-bitcoin:master Apr 19, 2023

tcharding deleted the 03-36-schemars branch April 19, 2023 23:25

tcharding mentioned this pull request Apr 19, 2023

extended_tests: Remove stale docs #1804

Merged

		# because 1.0.8 does not build with Rust 1.41.1 (because of useage of `Arc::as_ptr`).
		dyn-clone = { version = "<=1.0.7", default_features = false, optional = true }

		@@ -1,4 +1,4 @@
		#!/bin/sh
		#!/bin/bash

Remove schemars pin from manifest #1696

Remove schemars pin from manifest #1696

Conversation

tcharding commented Mar 6, 2023 • edited Loading

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil commented Mar 6, 2023

apoelstra commented Mar 6, 2023

tcharding commented Mar 6, 2023

tcharding commented Mar 6, 2023 • edited Loading

apoelstra commented Mar 7, 2023

tcharding commented Mar 7, 2023 • edited Loading

rust-bitcoin

extended_tests/schemars_test

tcharding commented Mar 7, 2023 • edited Loading

apoelstra commented Mar 7, 2023 • edited Loading

Kixunil commented Mar 7, 2023

apoelstra commented Mar 7, 2023

jeandudey commented Mar 7, 2023 • edited Loading

jeandudey commented Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

apoelstra commented Mar 7, 2023

tcharding commented Mar 28, 2023

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Mar 28, 2023

tcharding commented Mar 28, 2023

tcharding commented Mar 31, 2023

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Apr 3, 2023 • edited Loading

tcharding commented Apr 19, 2023 • edited Loading

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Remove `schemars` pin from manifest #1696

Remove `schemars` pin from manifest #1696

tcharding commented Mar 6, 2023 •

edited

Loading

tcharding commented Mar 6, 2023 •

edited

Loading

tcharding commented Mar 7, 2023 •

edited

Loading

tcharding commented Mar 7, 2023 •

edited

Loading

apoelstra commented Mar 7, 2023 •

edited

Loading

jeandudey commented Mar 7, 2023 •

edited

Loading

jeandudey commented Mar 7, 2023 •

edited

Loading

tcharding Mar 28, 2023 •

edited

Loading

tcharding commented Apr 3, 2023 •

edited

Loading

tcharding commented Apr 19, 2023 •

edited

Loading