Migrate inventory code #861
Conversation
Copy all files currently living in https://github.com/Malax/inventory/tree/main/inventory/src without any changes. Co-Authored-By: Manuel Fuchs <[email protected]> Co-Authored-By: Josh W Lewis <[email protected]>
runesoerensen
force-pushed
the
migrate-inventory-code
branch
from
0c039db
to
1e47371
Compare
runesoerensen
force-pushed
the
migrate-inventory-code
branch
from
1e47371
to
8c99bc5
Compare
We may want to consider another approach here (e.g. naming the feature `inventory-sha2` and/or pulling in the inventory dependency ["inventory", "dep:sha2"]). While the crate will be pulled in if a user enables the `sha2`, the inventory-specific sha2 code won't be compiled unless the `inventory` feature is also enabled.
runesoerensen
force-pushed
the
migrate-inventory-code
branch
from
8c99bc5
to
99d7d3a
Compare
These re-exports appear to be unnecessary, and doesn't have any impact on code that rely on the `inventory` module (and have the `semver` and/or `sha2` features enabled). Also see related prior discussion here Malax/inventory#2 (comment). The `semver` and `sha2` modules only contain implementations of public traits, so I don't think we need to re-export those (unlike bringing for instance a function or struct in to scope). If I understand how trait implementations work correctly, it doesn't matter where an implementation lives (only the visibility of the trait and the type it's implemented for is relevant) - in other words, the implementation will be available to any code that can access both the trait and the type, even across crate binaries.
runesoerensen
force-pushed
the
migrate-inventory-code
branch
from
e41dc7a
to
369a70f
Compare
| self.artifacts.push(artifact); | ||
| } | ||
|
|
||
| pub fn resolve<R>(&self, os: Os, arch: Arch, requirement: &R) -> Option<&Artifact<V, D, M>> |
There was a problem hiding this comment.
Given this is one of the primary functions of this API, it could probably use some documentation and maybe an example.
There was a problem hiding this comment.
Docs are added and the module docs have an example that calls this function. I think that's good enough for now.
| .max_by_key(|artifact| &artifact.version) | ||
| } | ||
|
|
||
| pub fn partial_resolve<R>( |
There was a problem hiding this comment.
I think this is just a different implementation of resolve? Is one better than the other or is the result different? My feeling is that we don't need both.
If we need both, we should document and test this one too.
There was a problem hiding this comment.
Note the trait bound on V. One is Ord the other is PartialOrd. Some version implementations are only partially ordered. One example could be f32 which is not totally ordered. As a more practical example: OpenJDK versions are only partially ordered as JEP 322 versions can only be compared to a legacy version when no version coordinates beyond the major version are specified.
There was a problem hiding this comment.
I think we can resolve this. Also. Thanks for the answer. I used it to help write documentation.
| @@ -0,0 +1,11 @@ | |||
| use crate::inventory::checksum::Digest; | |||
|
|
|||
| impl Digest for () { | |||
There was a problem hiding this comment.
As far as I can tell, this is unused. Should we remove it?
If we want to keep it - maybe we should give it a name? pub struct NoChecksum, UnknownChecksum, AnyDigest, or something.
There was a problem hiding this comment.
This is useful in tests where you might not want to have an actual checksum or when you want to read an inventory file where the reading code doesn't have an implementation of the used digest. This will happen when we have a generic CLI tool to work with inventory files.
Using ()/unit seems fine to me (unsurprisingly, since I wrote the code in question). There is no need to introduce a custom type to convey "no information", it's actually the formal definition of unit.
Quoting Wikipedia:
In the area of mathematical logic and computer science known as type theory, a unit type is a type that allows only one value (and thus can hold no information).
Note: There is a BogusDigest used in tests for the inventory. It's different from this as we there actually test that digests are validated and only match if the digest implementation matches as well.
I would say lets keep it around as-is for now, maybe create an issue to re-evaluate later. I'm pretty sure we'll need it again when the CLI is re-introduced (it intentionally wasn't ported with this PR). It's sitting in its own dedicated module, isn't explicitly interwoven with code outside of the module (since it's just a trait implementation). The complexity cost on the code-base is pretty much zero.
|
There's a few comments on docs. I took a stab at it in a stacked PR #864. Feedback/review is welcome over there or it's free to be merged in here and iterated on. The only known pending thing is i put the feature names in the docs as TODO. |
* Add module docs and example * Document resolve and partial_resolve * Document more inventory methods * Document Artifact * Document ArtifactRequirement I also suggest we change the name of `inventory/version` to `inventory/artifact_requirement.rs` or `requirement.rs`. * Update example to compare Checksum instead of string * Apply suggestions from code review Co-authored-by: Rune Soerensen <[email protected]> * Update feature names * Rewrite example usage * Show how to display checksum in example --------- Co-authored-by: Rune Soerensen <[email protected]>
|
@joshwlewis I've merged Richard's documentation PR - I think should address your documentation comments? |
This PR imports and migrates the inventory code here to the
libherokubuildpackcrate. The code was copied as-is and added in the first commit, and I've tried to keep changes to integrate it to a minimum.I'm not sure what the best approach is to adapt the
sha2andsemverfeature flags from the source inventory crate - perhaps it's fine as-is, or perhaps it'd make sense to use different feature names given the new context/crate for the code. Relevant commit with comment.I've removed a couple re-exports that appear to be unnecessary - see explanation in this commit.