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

Report dependencies by declaring project #47

Merged
merged 2 commits into from
Jul 10, 2023
Merged

Report dependencies by declaring project #47

merged 2 commits into from
Jul 10, 2023

Conversation

bigdaz
Copy link
Member

@bigdaz bigdaz commented Jul 10, 2023

This change aims to reduce redundant dependency reporting in repository snapshots, with the aim to make the result work better with the GitHub Dependency Graph. Previously, a dependency was reported for every project that referenced it directly or transitively through a project dependency. This resulted in many dependency versions
being reported many times within the snapshot, and these duplicates were then mirrored
in the GitHub Dependency Graph and Dependabot Security Alerts.

With this change, only external dependencies are reported, and only within the context of the project that declares those dependencies. This should help reduce the massive redundancy in the generated snapshot and make these files more useful.

This change aims to reduce redundant dependency reporting in repository snapshots,
with the aim to make the result work better with the GitHub Dependency Graph.
Previously, a dependency was reported for every project that referenced it directly
or transitively through a project dependency. This resulted in many dependency versions
 being reported many times within the snapshot, and these duplicates were then mirrored
in the GitHub Dependency Graph and Dependabot Security Alerts.

With this change, only external dependencies are reported, and only within the context
of the project that declares those dependencies. This should help reduce the massive
redundancy in the generated snapshot and make these files more useful.
@bigdaz bigdaz merged commit 95e4cd7 into main Jul 10, 2023
7 checks passed
@bigdaz bigdaz deleted the dd/project-focus branch July 10, 2023 12:09
@JLLeitschuh
Copy link
Contributor

This doesn't seem like the right thing to do. This seems like a problem in GitHub's handling of the data, not this plugins generation of the data.

@JLLeitschuh
Copy link
Contributor

What happens if you declare a dependency upon a project, but you have a dependency constraint in that project that forces a different version of a transitive to get resolved than would normally be resolved by the project being depended upon?

project(":a") {
    dependencies {
        api("x:x:1.0")
    }
}
project(":b") {
    dependencies {
        api(project("a"))
        constraints {
             api("x:x:2.0")
        }
    }
}

@bigdaz
Copy link
Member Author

bigdaz commented Jul 10, 2023

This doesn't seem like the right thing to do. This seems like a problem in GitHub's handling of the data, not this plugins generation of the data.

Thanks. I've spent a lot of time submitting dependency graphs for real-world projects, and IMHO this gives the best user experience working with GitHub as it stands now. There's no reason we can't change things later if/when GitHub changes things.

@JLLeitschuh
Copy link
Contributor

There's no reason we can't change things later if/when GitHub changes things.

Have you reached out to GitHub to explain the problem?

@bigdaz
Copy link
Member Author

bigdaz commented Jul 10, 2023

What happens if you declare a dependency upon a project, but you have a dependency constraint in that project that forces a different version of a transitive to get resolved than would normally be resolved by the project being depended upon?

When a constraint is declared by a consuming project, then that project will have a direct dependency. I've tested for a transitive dependency in a single project build here, but will add coverage for a multi-project build.

@bigdaz
Copy link
Member Author

bigdaz commented Jul 10, 2023

Have you reached out to GitHub to explain the problem?

No I haven't, but I feel like the current mapping is more in line with the GitHub Dependency Graph model.

Here's my thinking:

  • I consider a manifest as being "a place where dependencies are declared". The GitHub model refers to a "manifest file", so this seems like a logical definition.
  • As such, it makes sense for each resolved dependency to be assigned to the project that declared it. It would be even better if we could attribute the dependency to the actual file that declared it, but we're not there yet (and may not ever be).
  • Assigning a dependency declared by projectA to another project that depends on that project doesn't really make sense, since the fix will/should involve updating projectA (not the consuming project).

@JLLeitschuh
Copy link
Contributor

As such, it makes sense for each resolved dependency to be assigned to the project that declared it. It would be even better if we could attribute the dependency to the actual file that declared it, but we're not there yet (and may not ever be).

I can get behind this, assuming there is some mechanisim within the file format that allows you to directly reference the other resolved graph from the project being depended upon. Currently PURL doesn't support that. However, maybe GitHub's API could support this.

Assigning a dependency declared by projectA to another project that depends on that project doesn't really make sense, since the fix will/should involve updating projectA (not the consuming project).

Assuming that projectA will be published, it has a well-defined GAV coordinate. Can we generate a PURL for the GAV of projectA as if it were going to be published, and use that to refer to the dependencies of projectA?

@JLLeitschuh
Copy link
Contributor

When a constraint is declared by a consuming project, then that project will have a direct dependency. I've tested for a transitive dependency in a single project build here, but will add coverage for a multi-project build.

Much appreciated!

@JLLeitschuh
Copy link
Contributor

Have you reached out to GitHub to explain the problem?
No I haven't, but I feel like the current mapping is more in line with the GitHub Dependency Graph model.

Happy to coordinate such a discussion, but there is also the Gradle - GitHub shared slack channel between the two orgs that I'd suggest leveraging. If that's no longer present, let me know and I'm happy to leverage my contacts to setup a meeting here.

@bigdaz
Copy link
Member Author

bigdaz commented Jul 13, 2023

Happy to coordinate such a discussion, but there is also the Gradle - GitHub shared slack channel between the two orgs that I'd suggest leveraging. If that's no longer present, let me know and I'm happy to leverage my contacts to setup a meeting here.

Thanks. I've started having discussions with the GitHub Dependency Graph team on Slack, and I'm meeting with the PM today. This has been helpful and will drive further improvements to the mapping of the Gradle model to the current GitHub Dependency Graph model.

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.

3 participants