Add serde to Public and Private #466
Conversation
|
I think it would be a good idea to make serde support as an optional feature. |
Serde is already hard-required, so why make it optional? |
|
Oh, you are right. That was unfortunate. Then it will not matter for this PR but rather something we should look into doing. Because it is nice in my opinion to make it possible for the user to opt out of non essential functionality. |
|
Agreed with @Superhepper, serde should be optional but it's not an issue with this PR. |
|
Looks like I stepped on a land-mine adding these tests. Following the pattern of the marshall_unmarshall test I added these with a reflexive test using serde_json. serde_json was added as a dev-dependency. This caused more than 1500 compiler errors - it turns out that once serde_json exists, this confuses the compiler about the partialEq trait in the macros "test_valid_conversion" used throughout the test suite. Because of this, assert_eq! fails to work because there are multiple possible satisfying types for the ::into calls. An example is: I think this is beyond my ability to solve this, and probably needs some bigger work to clean up. |
|
In light of the above issue, what do you think I should do here @ionut-arm ? |
|
Have you tried something trivial like: if !(u16::from(*left_val) == u16::from(*right_val)) {And see if it helps? |
There are hundreds of locations that need this update. I'm not doing it in this PR. |
|
I can help you fix some of them. |
- Many of tests uses macro to create the test code and in this test code it is common practice to use ```into``` or ```try_into``` conversions. But this can cause ambiguities later on if new types introduced implements traits that previously only was implemented by one type. So in order to avoid this kind of ambiguity some of the conversions have been changed. This only a first step of changes that probably needs to be done. This fixes the errors that occurs if one tries to add ```serde_json``` to the ```cargo.yaml``` which is probably needed in order to be able to write tests for parallaxsecond#466. Signed-off-by: Jesper Brynolf <[email protected]>
Firstyear
force-pushed
the
20231103-add-serde-feature
branch
from
8b4797d to
fae4a03
Compare
|
Thanks to @Superhepper I was now able to add the serde tests :) |
|
Hey all, what else is needed here? We really want this merged so that we can use it for TPM support for Azure AD and other linux pam authentication subsystems. |
|
Not anything I think. I would just want CI to work again so this can without getting some unrelated errors. |
|
Try to rebase this on main now. The CI should be fine. |
Firstyear
force-pushed
the
20231103-add-serde-feature
branch
2 times, most recently
from
aa0b092 to
43b91f4
Compare
This allows serialisation and deserialisation of the Public and Private structures, which is required for storing these in many database or serialised forms. Signed-off-by: William Brown <[email protected]>
Firstyear
force-pushed
the
20231103-add-serde-feature
branch
from
43b91f4 to
2f3cd59
Compare
|
All fixed :) |
|
Thank you all so much! |
- Many of tests uses macro to create the test code and in this test code it is common practice to use ```into``` or ```try_into``` conversions. But this can cause ambiguities later on if new types introduced implements traits that previously only was implemented by one type. So in order to avoid this kind of ambiguity some of the conversions have been changed. This only a first step of changes that probably needs to be done. This fixes the errors that occurs if one tries to add ```serde_json``` to the ```cargo.yaml``` which is probably needed in order to be able to write tests for parallaxsecond#466. Signed-off-by: Jesper Brynolf <[email protected]>
Add support for serde to Public and Private, going through the current Marshall and Unmarshall pathways. This pattern is quite "simple" so it could be possible to duplicate it to other structures, or macro them for types that implement Marshall and Unmarshall.