-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: Add Julia language analyzer support #5635
feat: Add Julia language analyzer support #5635
Conversation
ccf889f
to
1024114
Compare
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.
Hello @Octogonapus
Thanks for your work!
I left few comments.
Can you also add tests for SPDX, CycloneDX to see how we use UUID for these formats?
1024114
to
a65056c
Compare
Yes I would like to do this. Can you help me by pointing out where there is an example of how to do this? I looked around the code for a while but I can't figure out how to export an SBOM in code (except of course by invoking the |
take a look on Integration tests for SBOM - https://github.com/aquasecurity/trivy/blob/main/integration/sbom_test.go |
I pushed an SPDX test with Julia, though it currently does not pass because the SPDX marshaller is doing some sort of deduplication based on the package name. i.e. if I changed one of the |
Do you mean that the packages are in random order? |
No, I mean that in the SPDX marshal test I added, there are two packages named |
Now i understand you. trivy/pkg/sbom/spdx/marshal_test.go Lines 968 to 988 in b5874e3
Some packages (like trivy/pkg/sbom/spdx/marshal_test.go Line 974 in b5874e3
Perphaps we can just add Indirect field.wdyt? |
Thanks, good idea, that worked. |
I pushed a CycloneDX test as well. There are a few problems with it that will require changing its marshaller:
I guess if we really needed to, we could also change the Julia PURL spec as package-url/purl-spec#237 has not been accepted yet. The PURLs could become |
This is a strange case. Why does Julia create duplicate dependencies? About PURL and BomRef - i think we can use next logic:
What if we update BomRef logic? if we use pkgID, we will not get a conflict, because BomRef will be unique for each julia library.
I don't think it's convenient. You need to read the lock file to find out the dependency name. But we can use
If I remember correctly, we added this because SPDX uses different hashes for each run(take a look this test - trivy/integration/repo_test.go Lines 360 to 368 in aedbd85
|
Julia allows you to add two dependencies with the same name but different UUIDs. This is an edge case, but given that it is allowed, I think it is important to test. Your suggestions for the PURL sounds good to me. After reviewing the spec, I can simply add a qualifier for the |
64b30b1
to
cfd4711
Compare
I added two integration tests for Julia SBOMs (SPDX and CycloneDX). I can't manage to run the tests, though:
or
So the tests might be wrong. If admin could trigger CI, that would be helpful so I can fix the tests. |
You need to run ➜ ls -dhl */
drwxr-xr-x 11 work staff 352B Jul 31 09:25 brand/
drwxr-xr-x 4 work staff 128B Aug 30 08:51 ci/
drwxr-xr-x 3 work staff 96B Jul 31 09:25 cmd/
drwxr-xr-x 10 work staff 320B Oct 31 12:24 contrib/
drwxr-xr-x 11 work staff 352B Dec 7 10:47 docs/
drwxr-xr-x 4 work staff 128B Jul 31 09:25 examples/
drwxr-xr-x 3 work staff 96B Jul 31 09:25 helm/
drwxr-xr-x 15 work staff 480B Dec 7 11:40 integration/
drwxr-xr-x 3 work staff 96B Nov 30 10:08 internal/
drwxr-xr-x 5 work staff 160B Dec 7 12:07 magefiles/
drwxr-xr-x 5 work staff 160B Oct 31 12:24 misc/
drwxr-xr-x 43 work staff 1.3K Nov 30 10:08 pkg/
drwxr-xr-x 5 work staff 160B Jul 31 09:25 rpc/
➜ mage test:integration
? github.com/aquasecurity/trivy/pkg/fanal/test/integration/docker [no test files]
...
Let us know about this work. It may make sense to wait for these changes before merging this PR. |
That is what I did.
I've updated the spec PR. Luckily there are no changes required in |
hm.. this is strange. Also you can copy command from mage file (e.g. Lines 239 to 243 in 01edbda
and use it in bash. Anyway i ran tests in CI/CD. |
a731135
to
30b6bbc
Compare
I reinstalled mage from source and it is working now 🤷 I fixed the integration tests. Everything is looking good to me, now that the PURL includes the UUID. |
// Removes dependencies not specified as direct dependencies in the Project.toml file. | ||
// This is not strictly necessary, but given that test dependencies are in flux right now, this is future-proofing. | ||
// https://pkgdocs.julialang.org/v1/creating-packages/#target-based-test-specific-dependencies | ||
func (a juliaAnalyzer) removeExtraDependencies(fsys fs.FS, dir string, app *types.Application) error { |
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.
We recently added Dev
flag to Package
(Dev dependencies are excluded from the report by default, but users can use --include-dev-deps
to include them).
But I'm not sure that Julia's users need this feature.
What do you think? Do users need dev dependencies in reports?
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.
Yes I think this will be helpful! I have tried to implement this from looking at other code and I've updated the test with some dev deps. LMK if it looks correct.
30b6bbc
to
22dee4f
Compare
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.
LGTM.
Left small comments. Take a look, please
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.
LGTM now!
@Octogonapus Thanks for your work!
@knqyf263 i approved this PR. Take a look, when you have time, please.
@Octogonapus |
Will be fixed by aquasecurity/go-dep-parser#292 |
90e9f1b
to
60df344
Compare
60df344
to
7cd5eeb
Compare
f7be231
to
6587286
Compare
This PR is stale because it has been labeled with inactivity. |
6587286
to
acd34a8
Compare
01e1987
to
fbc517f
Compare
I'm still trying to keep this PR up to date... though it can be hard with how many refactors happen on main |
fbc517f
to
10258d5
Compare
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.
Thanks for your patient. LGTM. I left a small comment.
integration/repo_test.go
Outdated
{ | ||
name: "julia generating CycloneDX SBOM", | ||
args: args{ | ||
command: "rootfs", | ||
format: "cyclonedx", | ||
input: "testdata/fixtures/repo/julia", | ||
}, | ||
golden: "testdata/julia-cyclonedx.json.golden", | ||
}, |
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.
We introduced the intermediate representation for SBOM. We don't need tests for SPDX and CycloneDX unless Julia needs any specific handling for SBOM. Either of them is fine.
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.
No specific handling is needed. I have removed the CycloneDX test.
20c833b
to
fe064b6
Compare
Description
This PR adds Julia language support for generating SBOMs.
Related issues
Checklist