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

Make rkyv feature imply size_32 #1383

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Jan 24, 2024

Should fix the build failure on docs.rs, see #1368 (comment) and #1380.

Fixes #1380.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (bf70419) 91.69% compared to head (09a8da0) 91.72%.
Report is 1 commits behind head on 0.4.x.

Files Patch % Lines
src/datetime/mod.rs 0.00% 1 Missing ⚠️
src/offset/utc.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            0.4.x    #1383      +/-   ##
==========================================
+ Coverage   91.69%   91.72%   +0.03%     
==========================================
  Files          38       38              
  Lines       17608    17267     -341     
==========================================
- Hits        16145    15839     -306     
+ Misses       1463     1428      -35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc
Copy link
Member

djc commented Jan 24, 2024

Could we instead change our feature forwarding so that rkyv implies size_32 (the upstream default)? That would unbreak other users.

@pitdicker
Copy link
Collaborator Author

I don't think we can because the feature name rkyv is the same as the name op the optional dependency. But you maybe know better what cargo supports when it comes to specifying dependencies than I do.

@djc
Copy link
Member

djc commented Jan 24, 2024

We can write rkyv = ['dep:rkyv', 'rkyv/size_32'] if our MSRV is new enough (which I think it is?).

@pitdicker
Copy link
Collaborator Author

I'll give it a try.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@pitdicker
Copy link
Collaborator Author

pitdicker commented Jan 24, 2024

Now that features rkyv-validation, rkyv-16, etc. no longer imply the rkyv feature it fails to build. Let me give it one more round 😄.

@pitdicker
Copy link
Collaborator Author

Do we want to bump to 0.4.33 in this PR?

@pitdicker pitdicker changed the title Change docs.rs feature to use rkyv-64 Make rkyv feature implie size_32 Jan 24, 2024
@pitdicker pitdicker changed the title Make rkyv feature implie size_32 Make rkyv feature imply size_32 Jan 24, 2024
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to bump to 0.4.33 in this PR?

Yes, please!

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated
rkyv-16 = ["dep:rkyv", "rkyv?/size_16"]
rkyv-32 = ["dep:rkyv", "rkyv?/size_32"]
rkyv-64 = ["dep:rkyv", "rkyv?/size_64"]
rkyv-validation = ["dep:rkyv", "rkyv?/validation"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this one is now odd because rkyv-validation doesn't pick a size? I'd propose that we leave dep:rkyv out of rkyv-validation and only leave rkyv?/validation in there so that it will only activate the downstream feature if any of the other features have been used to enable rkyv (and pick a size).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried both of your suggestions, but then it doesn't work out for all combinations:

cargo check --features=rkyv-64,rkyv-validation
    Checking rkyv v0.7.43
error: "size_32" and "size_64" are mutually-exclusive features. You may need to set `default-features = false` or compile with `--no-default-features`.
   --> C:\Users\paul\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rkyv-0.7.43\src\macros.rs:127:1
    |
127 | / core::compile_error!(
128 | |     "\"size_32\" and \"size_64\" are mutually-exclusive features. You may need to set \
129 | |     `default-features = false` or compile with `--no-default-features`."
130 | | );
    | |_^

error: could not compile `rkyv` (lib) due to previous error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make do without the validation -> dep:rkyv edge? It seems to work for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Done

src/month.rs Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

Set the date to today. Do we want to get #1384 in first?

@djc djc merged commit 7c419a3 into chronotope:0.4.x Jan 25, 2024
36 of 37 checks passed
@djc
Copy link
Member

djc commented Jan 25, 2024

Published to crates.io, thanks!

@pitdicker pitdicker deleted the docsrs_rkyv_feature branch January 25, 2024 12:39
@pitdicker
Copy link
Collaborator Author

That is quick. You too!

@pitdicker
Copy link
Collaborator Author

And we have docs again https://docs.rs/chrono/0.4.33/chrono/

@pitdicker pitdicker mentioned this pull request Jan 25, 2024
montekki referenced this pull request in matter-labs/zksync-withdrawal-finalizer Jan 25, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [chrono](https://github.com/chronotope/chrono) |
workspace.dependencies | patch | `0.4.32` -> `0.4.33` |

---

### Release Notes

<details>
<summary>chronotope/chrono (chrono)</summary>

###
[`v0.4.33`](https://github.com/chronotope/chrono/releases/tag/v0.4.33):
0.4.33

[Compare
Source](https://github.com/chronotope/chrono/compare/v0.4.32...v0.4.33)

#### What's Changed

- Fixed typo in Duration::hours() exception by
[@&#8203;danwilliams](https://github.com/danwilliams) in
[https://github.com/chronotope/chrono/pull/1384](https://github.com/chronotope/chrono/pull/1384)
- Make `rkyv` feature imply `size_32` by
[@&#8203;pitdicker](https://github.com/pitdicker) in
[https://github.com/chronotope/chrono/pull/1383](https://github.com/chronotope/chrono/pull/1383)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/matter-labs/zksync-withdrawal-finalizer).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMzUuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEzNS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot referenced this pull request in smartive/zitadel-rust Jan 25, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [chrono](https://github.com/chronotope/chrono) | dev-dependencies |
patch | `0.4.32` -> `0.4.33` |

---

### Release Notes

<details>
<summary>chronotope/chrono (chrono)</summary>

###
[`v0.4.33`](https://github.com/chronotope/chrono/releases/tag/v0.4.33):
0.4.33

[Compare
Source](https://github.com/chronotope/chrono/compare/v0.4.32...v0.4.33)

#### What's Changed

- Fixed typo in Duration::hours() exception by
[@&#8203;danwilliams](https://github.com/danwilliams) in
[https://github.com/chronotope/chrono/pull/1384](https://github.com/chronotope/chrono/pull/1384)
- Make `rkyv` feature imply `size_32` by
[@&#8203;pitdicker](https://github.com/pitdicker) in
[https://github.com/chronotope/chrono/pull/1383](https://github.com/chronotope/chrono/pull/1383)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 9pm,before 6am" in timezone
Europe/Zurich, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/smartive/zitadel-rust).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMzUuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEzNS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

2 participants