refactor & restructure LicenseEvidenceFetcher #1

jkowalleck · 2024-11-08T10:24:40Z

this copies the existing art from https://github.com/CycloneDX/cyclonedx-webpack-plugin/blob/master/src/extractor.ts

I'd rather take it as is - with all possible flaws - and improve it in a later issue/PR, than mixing too many things at a time.

Signed-off-by: Jan Kowalleck <[email protected]>

src/_helpers.ts

jkowalleck · 2024-11-08T10:28:08Z

src/builders.ts

+    }
+    const LICENSE_FILENAME_PATTERN = this.#LICENSE_FILENAME_PATTERN
+    const logger = this.console
+    return async function * (pkg: Package): AsyncGenerator<License> {


coppied from webpack implementation with the needed adjustments for yarn

tests/_data/snapshots/license-evidence-dev_dev-dependencies.json.bin

tests/_data/snapshots/license-evidence-dev_juice-shop.json.bin

jkowalleck · 2024-11-08T11:24:32Z

@AugustusKling reverted most of your work and dump copied over the existing art.

it showes some minor flaws, that i would love to tackle in an independent issue/PR

src/_helpers.ts

AugustusKling · 2024-11-13T16:50:20Z

src/builders.ts


 interface BomBuilderOptions {
  omitDevDependencies?: BomBuilder['omitDevDependencies']
  metaComponentType?: BomBuilder['metaComponentType']
  reproducible?: BomBuilder['reproducible']
  shortPURLs?: BomBuilder['shortPURLs']
-  gatherLicenseTexts: BomBuilder['gatherLicenseTexts']
+  gatherLicenseTexts?: BomBuilder['gatherLicenseTexts']


There is no need to have this optionally. The parameter definition of Clipanion has a default value of false set for the gather-license-texts parameter. Therefore the boolean is always present, either true or false.

understood.
I went with the code "style" the existing interface had.
I think it should be changed/improved in a dedicated pullrequest.

Changing everything at once makes review hard; keeping up the changes and intended outcome gets harder when modifying out-of-scope code.

AugustusKling · 2024-11-13T17:02:11Z

src/builders.ts


    this.console = console_
  }

  async buildFromWorkspace (workspace: Workspace): Promise<Bom> {
    // @TODO make switch to disable load from fs
    const fetchManifest: ManifestFetcher = await this.makeManifestFetcher(workspace.project)
+    const fetchLicenseEvidences: LicenseEvidenceFetcher = await this.makeLicenseEvidenceFetcher(workspace.project)


I get your argument from CycloneDX#193 (review) but 2 separate fetchers means that all packages have to be extracted twice. This is a waste resource-wise.

We should therefore have only 1 fetcher but not call it manifest fetcher nor license evidence fetcher. The fetcher is something general that makes package contents available. Depending on the linker used it goes to a real file system or a virtual one like the content of ZIP files.

What do you think about letting the fetcher take multiple extractors? Then the fetcher could open the packages (using fetcher.fetch(...)), call all extractors and close the packages (using releaseFs()). In between one extractor could get the package.json while the other could search for license evidence files.

I totally understand. During development of this patch, I've made attempts of extracting/refactoring the fetcher. I rolled back these results, as I thought they were premature optimizations.

I would like to have the unoptimized most-basic solution in place first, so any optimization is measurable easily, and can be introduced in an isolated pullrequest.
A process for this would be:

introduce a new feature as a minimum viable product (MVP)- merge it

test/benchmark this product and baseline your outcome

identify what can/must be improved - and write a very brief ticket/report for it

pullrequest a scoped fix for the issues

Changing all at once might be appealing, but it makes reviews harder and collaboration processes slower.
This is why I encourage small/scoped increments. :-D

AugustusKling · 2024-11-13T17:09:12Z

src/builders.ts

+          // option `withFileTypes:true` is not supported and casues crashes
+          files = packageFs.readdirSync(prefixPath)
+        } catch (e) {
+          logger?.warn('collecting license evidence in', prefixPath, 'failed:', e)


Whilst I prefer to simply call console.warn(...) like here, it does not follow the style of cyclonedx-node-yarn which uses console.warn('WARN | ...) in the other code.

thanks for spotting this.
fixed it via 2973e59

AugustusKling · 2024-11-13T17:13:13Z

src/builders.ts

+        try {
+          // option `withFileTypes:true` is not supported and casues crashes
+          files = packageFs.readdirSync(prefixPath)
+        } catch (e) {


This catch should not be here as it hides errors. The warning message is not enough.

I'd assume users of cyclonedx-node-yarn would call it as part of a build pipeline and expect a non zero exit code if anything fails that affects the produced SBOM. It's better to explode than to log a human-friendly message.

I copied this over from webpack implementation, without thinking too much.
With webpack, the SBOM is a side-artifact that is generated during build time, and it is designed to not break build processes.

the yarn plugin is called explicitly, and - like you said - it is expected to explode on errors.
I will change this accordingly.
PS: done via 643f95e

AugustusKling · 2024-11-13T17:14:45Z

src/builders.ts

+
+          const contentType = getMimeForTextFile(file)
+          if (contentType === undefined) {
+            continue


Do not silently skip over files matching LICENSE_FILENAME_PATTERN.

current implementation is a copy/past of existing art.
lets discuss improvements in a different issue/pullrequest (see #1 (comment))

AugustusKling · 2024-11-13T17:18:41Z

src/builders.ts

+    manifest.name = pkg.scope ? `@${pkg.scope}/${pkg.name}` : pkg.name
+    manifest.version = pkg.version
+    const component = this.makeComponent(pkg, manifest, type)
+    if (component instanceof Component) {


I don't see how makeComponent could conditionally return an instance. I'd guess something is wrong if we do not get an Component instance here and thus the code should abort execution.

lets discuss the out-of-scope improvements in a different issue/pullrequest (see #1 (comment))

tests/_data/snapshots/license-evidence-dev_juice-shop.json.bin

tests/_data/snapshots/plain_alternative-package-registry.json.bin

Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck · 2024-12-03T14:09:48Z

exising art got some fixes: CycloneDX/cyclonedx-webpack-plugin#1339

will update this very PR accordingly, soon

…x-webpack-plugin#1339 Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck · 2024-12-04T12:35:09Z

updated to match latest existing art - via 1dad58e

jkowalleck · 2024-12-04T12:52:21Z

@AugustusKling ,

how do you see the current state of this PR? Would you be okay with merging it and have it published and tested downstream?

I mean, this software is not meant to be final, it is intended to be improved step-by-step over time, right?

AugustusKling · 2024-12-04T16:45:22Z

tests/_data/snapshots/license-evidence-dev_juice-shop.json.bin

@@ -35997,20 +35993,12 @@
      ],
      "evidence": {
        "licenses": [
-          {
-            "license": {
-              "name": "file: LICENSE.APACHE2",


This should be found. The library allows using it with BSD-2-Clause OR MIT OR Apache-2.0

AugustusKling · 2024-12-04T16:57:54Z

@AugustusKling ,

how do you see the current state of this PR? Would you be okay with merging it and have it published and tested downstream?

I mean, this software is not meant to be final, it is intended to be improved step-by-step over time, right?

Firstly, sorry for leaving this for many days. I would like to spend more time to improve this tool but time is sadly limited.

We can merge this in my opinion as it is. When I compare with my PR to your repo https://github.com/CycloneDX/cyclonedx-node-yarn/pull/193/files then I think these things should be addressed soon (before or after merge):

Add --gather-license-texts to /README.md file, maybe with a warning that it's not reliable yet
Take over changes from /tests/README.md file

These improvement could be addressed in follow-up PRs:

Improve file name pattern for license files
Make sure to abort with a non-zero status code on errors, that is address things like mentioned in https://github.com/AugustusKling/cyclonedx-node-yarn/pull/1/files#r1840859282
Improve efficiency if it turns out opening the cached packages multiple times is notable

Just make sure to call the license extraction experimental until we can trust it does not silently skip over errors.

jkowalleck · 2024-12-04T18:18:37Z

[...] sorry for leaving this for many days.

No need to apologize, I totally understand that everybody has their own time/budget for working on OSS. You are doing great.

We can merge this in my opinion as it is. [...] I think these things should be addressed soon (before or after merge):

Feel free to merge my changes to your own branch, and apply the mentioned changes as needed.
We then might continue discussions in CycloneDX#193 - to identify follow-up tasks and changes for improvement.

merging this PR is expected to automatically update the existing CycloneDX#193

These improvement could be addressed in follow-up PRs. [...]

Fully agree.

Just make sure to call the license extraction experimental until we can trust it does not silently skip over errors.

Thank you for calling this out.
Stating the experimental nature in the README and in the CLI help page is probably the best we can do.

refactor & restructure LicenseEvidenceFetcher

a8a71da

Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck mentioned this pull request Nov 8, 2024

Extraction of license text from files. CycloneDX/cyclonedx-node-yarn#193

Merged

jkowalleck marked this pull request as draft November 8, 2024 10:26

jkowalleck changed the title ~~refactor & restructure LicenseEvidenceFetcher~~ [WIP] refactor & restructure LicenseEvidenceFetcher Nov 8, 2024

refactor & restructure LicenseEvidenceFetcher

0c96c60

Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck commented Nov 8, 2024

View reviewed changes

jkowalleck marked this pull request as ready for review November 8, 2024 11:23

jkowalleck changed the title ~~[WIP] refactor & restructure LicenseEvidenceFetcher~~ refactor & restructure LicenseEvidenceFetcher Nov 8, 2024

AugustusKling reviewed Nov 13, 2024

View reviewed changes

jkowalleck added 4 commits November 14, 2024 10:38

fix: warning prefixed

2973e59

Signed-off-by: Jan Kowalleck <[email protected]>

dont catch/warn - throw instead

643f95e

Signed-off-by: Jan Kowalleck <[email protected]>

dont gather licenses unintended

1d80836

Signed-off-by: Jan Kowalleck <[email protected]>

tests

f4070f4

Signed-off-by: Jan Kowalleck <[email protected]>

update according to existing art\n\ncopy over from CycloneDX/cycloned…

1dad58e

…x-webpack-plugin#1339 Signed-off-by: Jan Kowalleck <[email protected]>

AugustusKling reviewed Dec 4, 2024

View reviewed changes

AugustusKling merged commit 7696ebf into AugustusKling:gather-license-texts Dec 4, 2024

jkowalleck deleted the jk_gather-license-texts_changes branch December 4, 2024 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor & restructure LicenseEvidenceFetcher #1

refactor & restructure LicenseEvidenceFetcher #1

jkowalleck commented Nov 8, 2024 •

edited

Loading

jkowalleck Nov 8, 2024

jkowalleck commented Nov 8, 2024

AugustusKling Nov 13, 2024

jkowalleck Nov 14, 2024

AugustusKling Nov 13, 2024

jkowalleck Nov 14, 2024 •

edited

Loading

AugustusKling Nov 13, 2024

jkowalleck Nov 14, 2024 •

edited

Loading

AugustusKling Nov 13, 2024

jkowalleck Nov 14, 2024 •

edited

Loading

AugustusKling Nov 13, 2024

jkowalleck Nov 14, 2024

AugustusKling Nov 13, 2024

jkowalleck Nov 14, 2024 •

edited

Loading

jkowalleck commented Dec 3, 2024

jkowalleck commented Dec 4, 2024

jkowalleck commented Dec 4, 2024

AugustusKling Dec 4, 2024

AugustusKling commented Dec 4, 2024

jkowalleck commented Dec 4, 2024

refactor & restructure LicenseEvidenceFetcher #1

refactor & restructure LicenseEvidenceFetcher #1

Conversation

jkowalleck commented Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

jkowalleck commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkowalleck Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

jkowalleck commented Dec 3, 2024

jkowalleck commented Dec 4, 2024

jkowalleck commented Dec 4, 2024

Choose a reason for hiding this comment

AugustusKling commented Dec 4, 2024

jkowalleck commented Dec 4, 2024

jkowalleck commented Nov 8, 2024 •

edited

Loading

jkowalleck Nov 14, 2024 •

edited

Loading

jkowalleck Nov 14, 2024 •

edited

Loading

jkowalleck Nov 14, 2024 •

edited

Loading

jkowalleck Nov 14, 2024 •

edited

Loading