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

[ANE-610] Adds PDM Support #1214

Merged
merged 9 commits into from
Jun 13, 2023
Merged

[ANE-610] Adds PDM Support #1214

merged 9 commits into from
Jun 13, 2023

Conversation

meghfossa
Copy link
Contributor

@meghfossa meghfossa commented Jun 3, 2023

Overview

This PR, adds initial PDM support.

It does not support,

  • local dependency (these are path dependencies)
  • non-git version control sources

Acceptance criteria

  • As a user, I can analyze PDM project when I have pyproject.toml and pdm.lock
  • As a user, I can analyze PDM project when I only have pyproject.toml

Testing plan

Prereq

git checkout master && git pull origin && git checkout feat/adds-pdm && make install-dev

PDM (with lockfile)

  1. Create pdm project
  2. Perform pdm lock (to create lockfile)
  3. Perform fossa-dev analyze

Or clone example pdm project: https://github.com/pdm-project/pdm

# .fossa.yml
version: 3
paths:
  exclude:
    - ./tests/

PDM (without lockfile)

  1. Create pdm project (ensure there is no lockfile)
  2. Perform fossa-dev analyze

Or clone example pdm project: https://github.com/pdm-project/pdm, and rm pdm.lock.

# .fossa.yml
version: 3
paths:
  exclude:
    - ./tests/

Risks

N/A

Metrics

N/A

References

https://fossa.atlassian.net/browse/ANE-610

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).

@meghfossa meghfossa marked this pull request as ready for review June 3, 2023 23:15
@meghfossa meghfossa requested a review from a team as a code owner June 3, 2023 23:15
@meghfossa meghfossa requested a review from csasarak June 3, 2023 23:15
@meghfossa meghfossa changed the title Adds PDM Support [ANE-610] Adds PDM Support Jun 3, 2023
Copy link
Contributor

@csasarak csasarak left a comment

Choose a reason for hiding this comment

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

I'm only requesting changes to get a bit more clarity on how the strategies are organized here. Otherwise, this looks good to me.

docs/references/strategies/languages/python/pdm.md Outdated Show resolved Hide resolved
@@ -155,6 +158,7 @@ depTypeToFetcher = \case
UserType -> "user"
PubType -> "pub"
SwiftType -> "swift"
PathType -> "url"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the distinction here between URLType and PathType? I ask mainly because we'll do some work with path deps in the near-ish future and some of them probably won't map to a URL fetcher. I don't think this would be a problem because it's easy to add new types but I thought I'd ask.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - I changed to path now. These should be filtered out regardless.

I'm adding something like enrich :: (...) => Graphing Dependency -> m (Graphing Dependency) in upcoming conan pr, which would handle path dependencies. So I left it here for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested in hearing your thoughts about this as we have upcoming tickets (ANE-383/ANE-893) which are both focused on Go path deps. We were hoping to use them as a model for how to do it for other path deps. In particular a challenge we have to solve is how to present path deps in the UI in a way that isn't confusing or misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware re ANE-383/ANE-893. I will bring this up in our slack channel. If ANE-383 is already slotted, I will defer some of the conan work for later, so we can have consistent model.

src/Strategy/Python/PDM/Pdm.hs Show resolved Hide resolved
src/Strategy/Python/PDM/Pdm.hs Show resolved Hide resolved
@@ -155,6 +158,7 @@ depTypeToFetcher = \case
UserType -> "user"
PubType -> "pub"
SwiftType -> "swift"
PathType -> "url"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested in hearing your thoughts about this as we have upcoming tickets (ANE-383/ANE-893) which are both focused on Go path deps. We were hoping to use them as a model for how to do it for other path deps. In particular a challenge we have to solve is how to present path deps in the UI in a way that isn't confusing or misleading.

@meghfossa meghfossa merged commit bef2a5d into master Jun 13, 2023
17 checks passed
@meghfossa meghfossa deleted the feat/adds-pdm branch June 13, 2023 15:25
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