Switch to Github Actions rather than Travis #109
Conversation
apoelstra
force-pushed
the
2020-12--hex-cleanup
branch
4 times, most recently
from
0846d85 to
e28d070
Compare
|
Are you sure that removing schema-rs is the best option? It only got merged recently and seems to be optional already, so I don't think it's super important that this specific, new feature supports our general MSRV. Was |
I'm curious what the schema stuff is for tbh. Who/how is it used? Couldn't projects that care a lot about JSON schemas etc define their own schemas on our types and validate their validity? Or couldn't there be a bitcoin-json-schemas crate that just defines the schemas for the types they re-export? I suppose for my understanding an example usage in a project somewhere would be helpful. |
stevenroose
left a comment
stevenroose
left a comment
There was a problem hiding this comment.
LGTM. The MSRV addition works for me locally.
|
|
||
| /// Create a new newtype around a [Hash] type. | ||
| #[macro_export] | ||
| macro_rules! hash_newtype { |
There was a problem hiding this comment.
I wonder what exactly moving it here fixes. Does it have to do with the fact that we have #[macro_use] on the mod util; declaration?
There was a problem hiding this comment.
It's super dumb. This is called from sha256t.rs, so it needs to appear before the mod sha256t.
There was a problem hiding this comment.
We deliberately have #[macro_use] mod utils as the first mod-import in lib.rs, even though this is out of alphabetical order, for this reason.
| fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error> { | ||
| if s.is_human_readable() { | ||
| s.serialize_str(&self.to_hex()) | ||
| s.collect_str(self) |
There was a problem hiding this comment.
Nice! I noticed this before as well. With internal types I suppose this is safe because we know the fmt encodes just hex. Not sure how that would be for foreign types. I suppose in bitcoin, we can do this as well with bitcoin_hashes types as they're our own.
There was a problem hiding this comment.
I think it's probably generally useful to use Display for human-readable encodings, regardless of what they are.
Though I realize that your point is that we're using from_hex rather than from_str in our Deserialize impl, which means that this won't actually work except for hex-encoding types. Let me fix that.
|
@sgeisler you are welcome to propose an alternative and an incantation to make |
This prevents compilation on 1.29 by fixing serde_json to 1.57 which in turn fixes serde to 1.100, which then fixes serde_derive to 1.100, which depends on quote 1.0.8, which uses Rust 2018.
Also fix the benchmarks which were not building.
Me too 😄 I guess in scripting languages where you don't have to model the data types anyway it's useful for input validation, but would be interesting to know. Maybe @JeremyRubin knows some concrete examples? @apoelstra I built a proof of concept here: sgeisler@724a178#diff-73e17259d77e5fbef83b2bdbbe4dc40a912f807472287f7f45b77e0cbf78792dR77 I still want to move the "if rust=1.29 then pin serde_json" to the shell script, but it works. Basically I remove the build dep if rust=1.29 because it's not needed without the |
apoelstra
force-pushed
the
2020-12--hex-cleanup
branch
from
e28d070 to
4d5975c
Compare
It doesn't build tests (which require dev-deps), but it still build the crate if it's just a dependency. And I doubt anyone is developing on 1.29, which would make this problematic. For me that sounds like "good enough". It's annoying that dev-deps can't be optional, but making it a real dep seems worse imo. |
|
I don't think it's "good enough" that we cannot build this create on a supposedly-supported compiler without either using it indirectly as a dependency, or editing the source code. |
This makes no difference for the types in this crate, but will make a difference if these macros are used outside of it.
|
utACK
16f908e
…On Mon, Dec 28, 2020, 12:35 Sebastian ***@***.*** wrote:
***@***.**** approved this pull request.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#109 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGQLXAKXQPE34HXMVZONJTSXB3S3ANCNFSM4VKIX4TQ>
.
|
|
process nack & concept nack. On process, it is not good form to merge a commit removing a feature during a holiday weekend without time to respond. In terms of the actual commit, the issue is the dev dependency. Just delete the tests for the feature while leaving the schemars optional dep in place. You can also make the jsonvalid an optional dep that is enabled for testing only to enable those tests. Many other options other than removing the schemars code. |
|
Removing the tests seems suboptimal too. I think the best option would be to extract the tests into a separate crate which we only build for stable/nightly. This would have introduced even more scope creep here. I agree that the timing wasn't that good, sorry for that. |
|
@JeremyRubin the original PR which introduced this feature was a process failure, which led to the build having been broken ever since. |
Also fixes the build which was broken in multiple ways.