Tie dynamic linking all together #818
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We're using `<||>` to select the first success, so we need to explicitly fail ones that don't work instead of recovering.
skilly-lily
reviewed
Mar 2, 2022
Comment on lines
55
to
57
| -- When adding new tactics in the future, ensure that they fail (through diagnostics) if they're not the last link in the chain. | ||
| -- <||> selects the first item to succeed without a diagnostic error. | ||
| resolved <- rpmTactic root file <||> debTactic root file <||> apkTactic table file |
There was a problem hiding this comment.
I think they should just always fail. That way, ordering is not critical for success.
Suggested change
| -- When adding new tactics in the future, ensure that they fail (through diagnostics) if they're not the last link in the chain. | |
| -- <||> selects the first item to succeed without a diagnostic error. | |
| resolved <- rpmTactic root file <||> debTactic root file <||> apkTactic table file | |
| -- When adding new tactics in the future, ensure that they fail through diagnostics. | |
| -- <||> selects the first item to succeed without a diagnostic error. | |
| resolved <- rpmTactic root file <||> debTactic root file <||> apkTactic table file |
There was a problem hiding this comment.
Idea for a utility:
dsum [a, b, c] = a <||> b <||> cNaming comes from asum, but we should probably find a better name.
I think it could be implemented as
data DSumStart = DSumStart
instance ToDiagnostic DSumStart where
renderDiagnostic _ = "dsum was called with an empty list"
dsum = foldr (<||>) (fatal DSumStart)
|
@scruffystuffs since you did a drive by I can't use the bot to select anyone else 😬 if you don't have time to review this let me know and I'll post in Slack. |
|
Oops. Yeah I'm pretty swamped atm, you probably need someone else to review this. |
meghfossa
approved these changes
Mar 7, 2022
| additionalSourceUnits = mapMaybe (join . resultToMaybe) [manualSrcUnits, vsiResults, binarySearchResults] | ||
| traverse_ (Diag.flushLogs SevError SevDebug) [vsiResults, binarySearchResults, manualSrcUnits] | ||
| additionalSourceUnits = mapMaybe (join . resultToMaybe) [manualSrcUnits, vsiResults, binarySearchResults, dynamicLinkedResults] | ||
| traverse_ (Diag.flushLogs SevError SevDebug) [vsiResults, binarySearchResults, manualSrcUnits, dynamicLinkedResults] |
There was a problem hiding this comment.
If we want the dynamic deps results to show in scan summary, we would need to update this type, https://github.com/fossas/fossa-cli/blob/master/src/App/Fossa/Analyze/Types.hs#L40. And also few methods in App.Fossa.Analyze.ScanSummary.
If this is not in the AC of the ticket, ignore this.
|
@meghfossa thanks for the comments, it now integrates cleanly with the scan summary: |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Ties dynamic linking together.
Also removes
APKsupport, as we found that to have unacceptable performance when usingsyft.We'll make a ticket to revisit APK support when/if users request it.
Acceptance criteria
Support reporting dynamically linked dependencies.
Testing plan
Manual testing (todo: fill this in)
Testing on Ubuntu
I set up an ubuntu docker container which could build the CLI.
I then ran the following command:
I verified that the results appear as expected. The FOSSA UI shows the following results:
I verified the results by running
lddand looking up each package manually:Testing on Fedora
I set up a Fedora installation inside Parallels Desktop. I then set up the Haskell toolchain and built the CLI.
I then ran the following command:
I verified that the results appear as expected. The FOSSA UI displays the following results:
I verified the results by running
lddand looking up each package manually:Risks
Not very risky. Adds a new additional source unit.
References
Closes https://github.com/fossas/team-analysis/issues/886
Checklist
docs/.Changelog.mdif this change is externally facing. If this PR did not mark a release, I added my changes into an# Unreleasedsection at the top.*schema.jsonif I have made changes for.fossa.yml,fossa-deps.{json, yaml, yml}. You may also need to update these if you have added/removed new dependency (e.g. pip) or analysis target type (e.g. poetry).