Feat/152 typesafe test infrastructure

[tziemek]

resolves #152
resolves #174

[tziemek]

@tschmidtb51 We added another crate representing the parsed schema. Could you add this csaf-schema as another package on crates.io? And probably npm as well.

[oxisto]

@tschmidtb51 We added another crate representing the parsed schema. Could you add this csaf-schema as another package on crates.io? And probably npm as well.

What is the reasoning behind this? While I am not a complete expert on the rust ecosystem, my feeling is that splitting functionality up between crates without good reasoning is not preferred. Each crate needs its own publishing task (which could theoretically fail) and its own publishing config on crates.io. This can create a mess if one of them fails.

It is also harder for the user since they need to maintain versions on two crates, albeit this can be done with automatic tools. But you have to keep in mind that Rust will not "harmonize" dependency versions, i.e., if you use csaf-rs in version 0.2.0 (which depends on csaf-schema version 0.2.0) and you accidentally bump only csaf-schema in your dependencies to 0.2.1, then you will pull in both 0.2.1 and 0.2.0, but the types of both crates will be different.

I would propose simply switching the paths from csaf::csaf2_0::schema to csaf::schema::2_0 to keep everything organized. Then schema would be on the same level as validation.

[ctron]

I think it is ok to split libraries. There are tools like cargo release which can help in publishing multiple crates at the same time. Using a workspace, it is possible to keep dependencies aligned between crates. But this is all there.

I'm just not sure I understand the split and responsibility of each crate. I think it makes sense to have one crate which focuses on validation. That maybe a lot of code, that only certain users might want to use. Compared to the actual types, which will be used by more people. Splitting up the loader from the types (which is what seems to be case with this PR) doesn't seem like a reasonable idea.

I'd rather see the "test" stuff go into a dedicated module or crate. And maybe enable it through a feature flag. To remove it from the "normal" code.

So you could have:

• csaf-rs: the main types and serde stuff
  • Feature test to enable test stuff used outside of this crate
• csaf-validation: the validation, based on csaf-rs

I also noted that there are files which are claimed to be generated by build.rs. I don't think such files should be committed or in the src folder. To my understanding, such files should be in the target folder somewhere and regenerated. Which also means that code for generating must be supported on all platforms. Ideally written in Rust.

An alternative could be to manually generate this and commit the code. Performing the generation step during CI and failing the CI if there is a difference.

[tziemek]

Thank you for your feedback.

What is the reasoning behind this? While I am not a complete expert on the rust ecosystem, my feeling is that splitting functionality up between crates without good reasoning is not preferred. Each crate needs its own publishing task (which could theoretically fail) and its own publishing config on crates.io. This can create a mess if one of them fails.

It doesn't need its own publishing task, only the crate configuration in crates.io. You can see this already in the current build workflow, where there is only one call to cargo publish which publishes the whole workspace.

It is also harder for the user since they need to maintain versions on two crates, albeit this can be done with automatic tools. But you have to keep in mind that Rust will not "harmonize" dependency versions, i.e., if you use csaf-rs in version 0.2.0 (which depends on csaf-schema version 0.2.0) and you accidentally bump only csaf-schema in your dependencies to 0.2.1, then you will pull in both 0.2.1 and 0.2.0, but the types of both crates will be different.

No they don't, or rather only if consumers use explicit versions with =0.2.0. Cargo refers to SemVer when considering versions, so it doesn't matter if someone references 0.2.0 or 0.2.1 or what is referenced in another crate, they will get the latest version according to SemVer. So there is no version conflict, unless they use an explicit version with =0.2.0 and if someone does this, they know what they are doing. This behavior is consistent with other languages and their packaging systems.

I would propose simply switching the paths from csaf::csaf2_0::schema to csaf::schema::2_0 to keep everything organized. Then schema would be on the same level as validation.

The main reason behind this extraction was to make the code more maintainable and not blow up the build file, and additionally to have the benefit of having the types as a separate package which others can use.

I think it is ok to split libraries. There are tools like cargo release which can help in publishing multiple crates at the same time. Using a workspace, it is possible to keep dependencies aligned between crates. But this is all there.

I don't think cargo release is necessary, as the publish command already handles a lot of stuff regarding correct publishing. It may help in regards to keeping the versions clean, but we don't have a problem there at the moment.

I'm just not sure I understand the split and responsibility of each crate. I think it makes sense to have one crate which focuses on validation. That maybe a lot of code, that only certain users might want to use. Compared to the actual types, which will be used by more people. Splitting up the loader from the types (which is what seems to be case with this PR) doesn't seem like a reasonable idea.

I'd rather see the "test" stuff go into a dedicated module or crate. And maybe enable it through a feature flag. To remove it from the "normal" code.

So you could have:

csaf-rs: the main types and serde stuff
Feature test to enable test stuff used outside of this crate
csaf-validation: the validation, based on csaf-rs

I see your point here. What exactly do you mean by loader? The current proposal would exactly do that, split the types into csaf-schema and leave the all the validation in the current crate. I guess we could also move the csaf trait into the schema crate, which would then represent the concrete types for the individual versions and the trait for the abstraction. Here we could opt into the specific functionality for validation via a feature.

I also noted that there are files which are claimed to be generated by build.rs. I don't think such files should be committed or in the src folder. To my understanding, such files should be in the target folder somewhere and regenerated. Which also means that code for generating must be supported on all platforms. Ideally written in Rust.

An alternative could be to manually generate this and commit the code. Performing the generation step during CI and failing the CI if there is a difference.

As there is no standard in rust to refer to, we can agree to disagree :) I see the point in having generated files in the target folder, but I loose a bit of control over what is there. With committed files a clearly see what changed right away and not only with some errors in other parts of the code. But I like the approach of not doing this in the build.rs file, but having some small tooling with manual interaction, it was for the moment just easier to put it there as the generation was already there. As these schemata aren't expected to change often, I would go with that approach.

[tziemek]

I'll pulled out the test infrastructure to a separate PR #221 . This is then only for eventually pulling out the schema generation etc. I'll refactor into a local tool to generate the files/

[peinjoh]

I think it would be helpful to print the expected errors here.