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

Fallback on 'cdx' or 'bom' JSON files for Java BOM #1127

Merged
merged 2 commits into from
May 30, 2024

Conversation

nekhtan
Copy link
Contributor

@nekhtan nekhtan commented May 29, 2024

Updated the fallback lookup of BOM files to find JSON files that either contain "bom" or "cdx" in their filename.

Linked to #1126

Signed-off-by: nekhtan <129828933+nekhtan@users.noreply.github.com>
@prabhu
Copy link
Contributor

prabhu commented May 29, 2024

Thank you so much! Did the fix work as per your needs?

@nekhtan
Copy link
Contributor Author

nekhtan commented May 29, 2024

Thank you so much! Did the fix work as per your needs?

Actually, no, I opened the PR too soon :(

The filter seems a bit too wide: I have SBOM test files that match the new pattern and they are taken into account. The resulting SBOM is way too large and does not represent the reality (aggregates the project's BOM and the BOMs from tests).

@prabhu
Copy link
Contributor

prabhu commented May 29, 2024

@nekhtan, may be use --exclude to exclude those test directories? Or how about

bomJsonFiles = getAllFiles(path, "target/**/*{cdx,bom}*.json", options);

@nekhtan
Copy link
Contributor Author

nekhtan commented May 29, 2024

Alright I struggled a bit with the exclude but it finally does the job: --exclude=./**/src/test/**/*.json

Note that:

  • ./**/src/test/** is not enough
  • **/src/test/**/*.json has a weird behavior as it adds my CLI executable to the pattern and it's resolved as ["**C:/dev/PortableGit/src/test/**/*.json"] 😮

@prabhu
Copy link
Contributor

prabhu commented May 29, 2024

Could you test the exclude a bit more and contribute to the docs as well in a separate PR?

@nekhtan
Copy link
Contributor Author

nekhtan commented May 29, 2024

I went all the way up to this line and it's still broken...
BUT if I use the absolute path of the node executable it works as expected, so I really don't get it 🙄

Broken when using node directly:

> node /c/<path>/cdxgen/bin/cdxgen.js --no-recurse --exclude="**/src/test/**/*.json" 

process.argv [
  ...,
  '--exclude=**C:/dev/PortableGit/src/test/**/*.json'
]

Working when using node from the result of which node:

> C:/<path>/bin/node /c/<path>/cdxgen/bin/cdxgen.js --no-recurse --exclude="**/src/test/**/*.json"

process.argv [
  ...,
  '--exclude=**/src/test/**/*.json'
]

@prabhu
Copy link
Contributor

prabhu commented May 29, 2024

ok, let's look into the exclude issue separately. Do we need the target prefix as suggested in the earlier comment?

@nekhtan
Copy link
Contributor Author

nekhtan commented May 29, 2024

For my own need, it seems to work just fine with the exclusion, so as far as I'm concerned the PR is ok.

Regarding the general behaviour, I dont think it would be wise to replace the current pattern with a completely different one: it might break for other people, don't you think? The one you suggested doesn't break the current behavior, it just extends it, right?

@prabhu
Copy link
Contributor

prabhu commented May 29, 2024

It's a good point, although we can never be sure with opensource, since every change could break something somewhere. I think we can add the target prefix, since that code shouldn't pick up any other sbom file. Could you kindly make this change?

Signed-off-by: nekhtan <129828933+nekhtan@users.noreply.github.com>
Copy link
Contributor

@prabhu prabhu left a comment

Choose a reason for hiding this comment

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

Thank you!

@prabhu prabhu merged commit df888c3 into CycloneDX:master May 30, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants