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

Add Conan (C/C++) conan.lock file support #1230

Merged
merged 12 commits into from
Sep 29, 2022
Merged

Conversation

hkwi
Copy link
Contributor

@hkwi hkwi commented Sep 28, 2022

extend #1083 and implement #1222

Signed-off-by: Hiroaki KAWAI <hiroaki.kawai@gmail.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

This is looking pretty good -- just one small suggestion.

It also looks like there's a lint issue that needs a simple fix.

syft/pkg/cataloger/cpp/parse_conanlock.go Outdated Show resolved Hide resolved
@kzantow kzantow changed the title add Conan (C/C++) conan.lock file support Add Conan (C/C++) conan.lock file support Sep 28, 2022
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Copy link

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Language: pkg.CPP,
Type: pkg.ConanPkg,
MetadataType: pkg.ConanaMetadataType,
Metadata: pkg.ConanMetadata{
Copy link
Contributor

Choose a reason for hiding this comment

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

capturing the name and version isn't really necessary. There is one or two other catalogers that do this for implementation-specific limitations, however, in this case it's redundant.

That being said, there is other information in a conan.lock file that could be useful to capture (such as options). It's probably worth taking a look at what other information could be provided and adding that to the metadata.

If we don't want to add any more elements, my recommendation now would be to not have a metadata struct at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wagoodman I can add options path and context as metadata fields and remove name and version here

Then I'll update the conanfile to include nil for the metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

@wagoodman it also looks like name and version are tied to the method for generating a PURL for a given conan package. It might be useful to keep those fields so we can agnostically generate the PURL for either a lockfile or conanfile even if the information is redundant for the package struct:

https://github.com/anchore/syft/blob/main/syft/pkg/conan_metadata.go

Is there another space you would want this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, we're reusing the same metadata for logically a different things. I don't think we should do this. That is, the Metadata is reserved for things that are specific from what this package was parsed from (I didn't see the reuse on the first review pass). We should probably be introducing a new metadata type to capture this information.

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Agree with @wagoodman 's comment about not needing redundant name and version information, but 👍 from me

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
Copy link
Contributor

spiffcs commented Sep 28, 2022

Talked offline with @wagoodman. @kzantow I enhanced the metadata struct with more fields from the lockfile. We'll keep name and version for now to not disturb the PURL generation for cpp packages.

@spiffcs
Copy link
Contributor

spiffcs commented Sep 28, 2022

One more small refactor incoming


type conanLock struct {
GraphLock struct {
Nodes map[string]struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this could be map[string]pkg.ConanMetadata instead of redefining these. That is, if the metadata is meant to raise up the raw packaging metadata, if we remove name/version then there would be no difference between this struct def and pkg.ConanMetadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong about this if options string should really be transformed into options []string (split by newline) or options map[string]string if split by newline and =. (same comment for similar fields)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll update options to be the map suggestion and look to make the other fields updated to match this suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

For those following along here are all possible fields for the conan.lock structure
https://github.com/conan-io/conan/blob/develop/conans/model/graph_lock.py#L101-L276

@@ -8,6 +8,9 @@ import (
type ConanMetadata struct {
Name string `mapstructure:"name" json:"name"`
Version string `mapstructure:"version" json:"version"`
Options string `mapstructure:"options" json:"options"`
Copy link
Contributor

@wagoodman wagoodman Sep 28, 2022

Choose a reason for hiding this comment

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

we'll need to regenerate the json schema due to change (might as well wait on this until we know what the final change is)

- add NameAndVersion method to parse ref from ConanMetadata
- update parseConanfile to use new NameAndVersion method
- update parseConanfile tests to use Ref vs Name/Version
- update parseConanlock to use new NameAndVersion method
- update packageurl to test new NameAndVersion method
- add ConanMetadata test for PURL parsing

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
Copy link
Contributor

spiffcs commented Sep 28, 2022

Quick status on this one

Currently separating out the metadata types per this comment:
#1230 (comment)

Updating the CLI tests and adding more fields to the new metadata object per this comment:
#1230 (comment)

- add conan_lock_metadata to use for conan lock Parser
- revert changes to conan metadata
- update inline struct for conan lock parser to match all fields
- update tests to match new parsing of conan lock options

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
Copy link
Contributor

spiffcs commented Sep 28, 2022

Last change here will be to regenerate the schema, I'm still reviewing and trying to find examples of a conan lock file is all possible fields populated so we're sure the new metadata struct if constructed correctly

- BREAKING: change ConanMetadata to use ref
- ADDITION: add ConanaLockMetadata type

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs
Copy link
Contributor

spiffcs commented Sep 29, 2022

CLI passed on my local @kzantow so this 🤞 should be ready

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
@spiffcs spiffcs enabled auto-merge (squash) September 29, 2022 18:22
@spiffcs
Copy link
Contributor

spiffcs commented Sep 29, 2022

Thanks @hkwi for the contribution! We really appreciate the enhancement here and getting the ball rolling on this. We also appreciate your patience with the PR process.

@spiffcs spiffcs merged commit b9b13d5 into anchore:main Sep 29, 2022
spiffcs added a commit that referenced this pull request Oct 13, 2022
* main: (45 commits)
  feat: add RelationshipsBySourceOwnership to syft json output (#1248)
  fix: reset merged package into map; (#1258)
  refactor: Remove experimental Anchore Enterprise upload functionality (#1257)
  Update syft bootstrap tools to latest versions. (#1254)
  Update Stereoscope to d24c9d626b33fa720210b007a20767801827b532 (#1253)
  Update syft bootstrap tools to latest versions. (#1244)
  fix apkdb checksum representation (#1247)
  feat: add identifiable field to source object (#1243)
  feat: attest support for Singularity images (#1201)
  Update syft bootstrap tools to latest versions. (#1239)
  Update Stereoscope to 1b1b744a919964f38d14e1416fb3f25221b761ce (#1240)
  fix: Follow symlinks when searching for globs in all-layers scope (#1221)
  update requires to use list; remove field (#1234)
  Add Conan (C/C++) conan.lock file support (#1230)
  add sequence diagrams and flesh out TODO notes (#1233)
  Do not fail if unable to parse `.rpm` file (#1232)
  fix: support exclude patterns on Windows (#1228)
  Update syft bootstrap tools to latest versions. (#1225)
  Update Stereoscope to 56552770e555d764ea72b99d3c810326b27ead4a (#1224)
  Update syft bootstrap tools to latest versions. (#1223)
  ...

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
aiwantaozi pushed a commit to aiwantaozi/syft that referenced this pull request Oct 20, 2022
Co-authored-by: Christopher Phillips <christopher.phillips@anchore.com>
spiffcs added a commit that referenced this pull request Oct 21, 2022
Co-authored-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
spiffcs added a commit that referenced this pull request Oct 21, 2022
Co-authored-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
Co-authored-by: Christopher Phillips <christopher.phillips@anchore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants