Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Add support for Yarn v2 projects #244

Merged
merged 32 commits into from
Jun 4, 2021
Merged

Add support for Yarn v2 projects #244

merged 32 commits into from
Jun 4, 2021

Conversation

cnr
Copy link
Contributor

@cnr cnr commented Jun 1, 2021

Overview

This PR adds support for the v2 yarn lockfile format. When analyzing yarn projects, we first attempt to analyze yarn.lock as a v1 lockfile, then fall back to analyzing as a v2 lockfile.

Developer documentation for the yarn v2 lockfile is rendered here

Acceptance criteria

  • fossa analyze supports projects containing a yarn v2 lockfile, including all standard dependency types contained therein (see associated devdocs and Strategy.Yarn.V2.Resolvers module for more info)
  • fossa analyze continues to support projects containing a yarn v1 lockfile

Testing plan

  • Automated unit tests, including an end-to-end test on the lockfile in the devdocs
  • Manual sanity check tests on various yarn projects:
    1. Analyze a yarn v1 project in --output mode: fossa analyze --output
    2. Set the yarn version to v2: yarn set version berry
    3. Update the lockfile: yarn install
    4. Analyze the same project in --output mode: fossa analyze --output
    5. The dependency output from steps 1 and 4 should be identical (but with the addition of dependencies marked as "direct". Thanks yarn v2!)

Risks

None

References

Closes https://github.com/fossas/team-analysis/issues/526

Checklist

Remaining:

  • Convert devdocs to markdown

@cnr cnr requested a review from skilly-lily June 1, 2021 04:40
@@ -0,0 +1,186 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is currently formatted as an emacs org-mode document, because it's substantially easier for me to edit that way. I'll make sure to convert to markdown before merging

]
++ if null failureWarnings
Copy link
Contributor Author

@cnr cnr Jun 1, 2021

Choose a reason for hiding this comment

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

This change is unrelated to adding yarn support. It excludes the "Relevant warnings include" addendum to error messages if there are no relevant warnings

}

getDeps :: (Has ReadFS sig m, Has Diagnostics sig m) => YarnProject -> m (G.Graphing Dependency)
getDeps = context "Yarn" . context "Static analysis" . YarnLock.analyze' . yarnLock
getDeps project = context "Yarn" $ getDepsV1 project <||> getDepsV2 project
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function can also be written as:

getDeps :: (Has ReadFS sig m, Has Diagnostics sig m) => YarnProject -> m (G.Graphing Dependency)
getDeps = (<||>) <$> getDepsV1 <*> getDepsV2

.. which is really cute but unmaintainable

Comment on lines +68 to +71
-- Fortunately, aeson provides a mechanism for us to represent this:
-- 'FromJSONKey', which is used in the 'FromJSON' instance for 'Map'. It allows
-- us to arbitrarily decode any type as a Map key (assuming an 'Ord' instance,
-- of course).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's incredible that aeson has this. It made parsing this format SO much easier

-- From yarn's structUtils.tryParseDescriptor/tryParselocator (in strict mode):
--
-- @
-- string.match(/^(?:@([^/]+?)\/)?([^/]+?)(?:@(.+))$/)
Copy link
Contributor Author

@cnr cnr Jun 1, 2021

Choose a reason for hiding this comment

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

I never want to see another regex in my life.

Special shoutout to whatever the heck this is:

range.match(/^([^#:]*:)?((?:(?!::)[^#])*)(?:#((?:(?!::).)*))?(?:::(.*))?$/);

@elldritch
Copy link
Member

I really like the explanations you provided in the developer docs. Can you link to the parts of the Yarn documentation (or other source material e.g. blog posts, GH source permalinks) that you're referencing for some of the content? For example, you talk about the semantics behind specific fields in the lockfile - is there a doc somewhere in Yarn that describes those semantics?

Having these references makes it easier for new developers to see where to look when we inevitably need to make bugfixes or keep up with spec changes (so we have somewhere to go to answer "are we actually doing the semantically correct thing?").

@cnr
Copy link
Contributor Author

cnr commented Jun 2, 2021

is there a doc somewhere in Yarn that describes those semantics?

unfortunately, their source code is the documentation for most of this. I'll add more source code references

Copy link
Contributor

@skilly-lily skilly-lily left a comment

Choose a reason for hiding this comment

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

LGTM, few nits and one new test case for a Text function.

Comment on lines 1 to 2
# This file is generated by running "yarn install" inside your project.
# Manual changes might be lost - proceed with caution!
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]: We should probably remove these comments. They're a little misleading for test purposes. We can use other comments if we're interested in checking the parser.

StrictData
TupleSections
TypeApplications
TypeOperators
TypeSynonymInstances
ImportQualifiedPost
StandaloneKindSignatures
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]: did cabal-fmt put these out-of-order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'm not sure why it's doing that 😕

Comment on lines +16 to +19
gtraverse f = fmap mkAdjacencyMap . traverse (\(a, xs) -> (,) <$> f a <*> traverse f xs) . AM.adjacencyList
where
mkAdjacencyMap :: Ord c => [(c, [c])] -> AM.AdjacencyMap c
mkAdjacencyMap = AM.fromAdjacencySets . fmap (fmap S.fromList)
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional, readability]: I hate the (,) constructor so much, it's really annoying to read. I usually use tuplify, since names are easier to read in prefix position than operators-in-parenthesis.

Suggested change
gtraverse f = fmap mkAdjacencyMap . traverse (\(a, xs) -> (,) <$> f a <*> traverse f xs) . AM.adjacencyList
where
mkAdjacencyMap :: Ord c => [(c, [c])] -> AM.AdjacencyMap c
mkAdjacencyMap = AM.fromAdjacencySets . fmap (fmap S.fromList)
gtraverse f = fmap mkAdjacencyMap . traverse (\(a, xs) -> tuplify <$> f a <*> traverse f xs) . AM.adjacencyList
where
tuplify a b = (a, b)
mkAdjacencyMap :: Ord c => [(c, [c])] -> AM.AdjacencyMap c
mkAdjacencyMap = AM.fromAdjacencySets . fmap (fmap S.fromList)

@@ -46,3 +48,6 @@ underBS f = decodeUtf8 . f . encodeUtf8

showT :: Show a => a -> Text
showT = T.pack . show

dropPrefix :: Text -> Text -> Text
dropPrefix pre txt = fromMaybe txt (T.stripPrefix pre txt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a single test case in test/Extra/TextSpec.hs?

@cnr cnr merged commit c4a6d0a into master Jun 4, 2021
@cnr cnr deleted the yarn-v2 branch June 4, 2021 20:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants