-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(sca): add conan (C++) ecosystem #320
Conversation
Backwards compatibility summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you follow the correct steps after making the change, it LGTM
@@ -1091,6 +1091,7 @@ type ecosystem <python decorator="dataclass(frozen=True)"> <ocaml attr="deriving | |||
(* Deprecated: Mix is a build system, should use Hex, which is the ecosystem *) | |||
| Mix <json name="mix"> | |||
| Hex <json name="hex"> | |||
| Conan <json name="conan"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is the best approach here. It may be, but it also feels slightly awkward because we don't technically support this ecosystem from a SCA rules standpoint yet (and C++ is weird enough that I personally am not super confident that Conan really fits our idea of an "ecosystem").
For package managers that we don't yet support, I wonder if it would make more sense to add an unknown ecoosystem or, probably better, allow ecosystem to be optional in the places where we convert from lockfile kind to ecosystem. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and C++ is weird enough that I personally am not super confident that Conan really fits our idea of an "ecosystem"
This is something I’ve been grappling with as well, and I agree with this sentiment. C++ doesn’t have a single, universally adopted package manager, and introducing conan as an ecosystem sets the expectation that we’ll add other C++ package managers as separate ecosystems. that doesn’t feel like the right approach.
allow ecosystem to be optional in the places where we convert from lockfile kind to ecosystem.
I think this makes a lot of sense and is probably the direction I’ll take. It gives us the flexibility to handle unsupported or edge-case scenarios like this.
That said, we’ll likely need to support C++ package managers in the future, and when that happens, we’ll still need a way to define an overarching ecosystem. Following the Python approach (Pypi
), we could introduce something like CPlusPlus
to represent the broader ecosystem (assuming ++
isn’t allowed in the type name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, we’ll likely need to support C++ package managers in the future, and when that happens, we’ll still need a way to define an overarching ecosystem. Following the Python approach (Pypi), we could introduce something like CPlusPlus to represent the broader ecosystem (assuming ++ isn’t allowed in the type name).
Yeah, I think this will require some thought when we get there. It feels odd because while there are different package managers for python, AFAIK they all use the same basic registry definition defined by Python, but that's not the case for C++ (there is no such concept of a registry in the language spec afaik). This sounds reasonable for now and for the general case of unsupported subproject types.
closing. need a larger discussion on what a C++ ecosystem should look like. |
This PR adds the Conan C++ package manager to the ecosystem type.
make setup && make
to update the generated code after editing a.atd
file (TODO: have a CI check)For example, the Semgrep backend need to still be able to consume data generated
by Semgrep 1.17.0.
See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades