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

Update to syn-2 #858

Merged
merged 4 commits into from
Mar 26, 2023
Merged

Update to syn-2 #858

merged 4 commits into from
Mar 26, 2023

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Mar 23, 2023

This won't test cleanly or go in yet, as it depends on synstructure updating to support syn-2 (this is trivial, but hasn't landed yet).

Uploading the code for review in case you have feedback before it's landable.

@tarcieri
Copy link
Member

tarcieri commented Mar 23, 2023

It might also be worth dropping synstructure. zeroize_derive isn't a particularly complicated proc macro.

Never mind, synstructure has been updated.

@maurer
Copy link
Contributor Author

maurer commented Mar 25, 2023

I missed the edit and wrote a commit removing the synstructure dep. Do you want me to drop that commit or leave it in?

@tarcieri
Copy link
Member

Oh, if you’ve already done it one fewer dependency is great, thank you!

@tarcieri
Copy link
Member

Hmm, seems like the zeroize tests are failing on fields that use #[zeroize(skip)] because they aren't being accessed.

I think you could go ahead and add #[allow(unused_variables)] to those fields?

@maurer
Copy link
Contributor Author

maurer commented Mar 25, 2023

Added the #[allow(unused_variables)], and put a #![deny(warnings)] into the zeroize derive test that was breaking - it had scrolled past clean locally because the warnings got pushed off the screen, but you must be overriding that in CI somewhere. That should make it fail locally for others.

@tarcieri
Copy link
Member

We deny warnings in CI but allow them locally to make refactoring easier. I'd suggest removing the #![deny(warnings)].

@tarcieri
Copy link
Member

Hmm, seems this will entail an MSRV bump for the derive feature. That's fine.

tarcieri added a commit that referenced this pull request Mar 25, 2023
Since #858 needs MSRV 1.56 anyway, this goes ahead and bumps the edition
for `zeroize` and `zeroize_derive` to 2021.

This also lets us remove a number of hacks in CI, including
commented-out tests.
@tarcieri
Copy link
Member

I opened #869 which does a 2021 edition upgrade for zeroize and zeroize_derive

tarcieri added a commit that referenced this pull request Mar 25, 2023
Since #858 needs MSRV 1.56 anyway, this goes ahead and bumps the edition
for `zeroize` and `zeroize_derive` to 2021.

This also lets us remove a number of hacks in CI, including
commented-out tests.
@tarcieri
Copy link
Member

@maurer if you rebase it should take care of the MSRV-related issues

zeroize/derive/src/lib.rs Outdated Show resolved Hide resolved
fiat-constify/src/main.rs Outdated Show resolved Hide resolved
fiat-constify/src/main.rs Outdated Show resolved Hide resolved
maurer and others added 2 commits March 26, 2023 08:53
For some reason this isn't working in CI, though it works locally.

Co-authored-by: Tony Arcieri <bascule@gmail.com>
`Attribute` already carries the delimiter tokens separately, they shouldn't be additionally provided to the tokens field.

Co-authored-by: Tony Arcieri <bascule@gmail.com>
@tarcieri
Copy link
Member

Awesome, thanks!

@tarcieri tarcieri merged commit 30b2c55 into RustCrypto:master Mar 26, 2023
@maurer maurer deleted the syn-update branch March 26, 2023 18:51
@maurer maurer restored the syn-update branch March 26, 2023 18:51
@maurer maurer deleted the syn-update branch March 26, 2023 18:51
@maurer maurer restored the syn-update branch March 26, 2023 18:51
@maurer maurer deleted the syn-update branch March 26, 2023 18:51
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