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

fix: moduleArtifacts is sometimes empty #292

Merged
merged 8 commits into from
Aug 2, 2021

Conversation

elldritch
Copy link
Member

@elldritch elldritch commented Jul 20, 2021

Overview

This PR fixes an issue where moduleArtifacts could be empty, causing a silent NoSuchElementException that would cause certain Gradle configurations to not resolve dependencies.

Please pay particular attention to the documentation and comment changes. Do these help improve understandability? I would like to use the documentation I wrote here as the model for docs for all future analyzers.

Acceptance criteria

Gradle projects relying on dependencies with no module artifacts (e.g. jackson-bom or junit-bom) should successfully resolve.

Testing plan

Put this build.gradle.kts file in a new empty directory to use as a test project:

/*
 * This file was generated by the Gradle 'init' task.
 *
 * This generated file contains a sample Java library project to get you started.
 * For more details take a look at the 'Building Java & JVM projects' chapter in the Gradle
 * User Manual available at https://docs.gradle.org/7.1.1/userguide/building_java_projects.html
 */

plugins {
    // Apply the java-library plugin for API and implementation separation.
    `java-library`
}

repositories {
    // Use Maven Central for resolving dependencies.
    mavenCentral()
}

dependencies {
    testImplementation("org.junit.jupiter:junit-jupiter:5.7.1")
    testImplementation("org.mockito:mockito-junit-jupiter:3.11.2")
    testImplementation("commons-io:commons-io:2.10.0")

    implementation("com.google.guava:guava:30.1-jre")
    implementation("org.apache.calcite:calcite-core:1.27.0")
    implementation("org.projectlombok:lombok:1.18.8")
    implementation("com.github.vertical-blank:sql-formatter:1.0")
    implementation("org.apache.commons:commons-collections4:4.2")
    implementation("org.apache.avro:avro:1.8.2")
    implementation("org.apache.commons:commons-lang3:3.12.0")
    implementation("com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.12.3")
    annotationProcessor("org.projectlombok:lombok:1.18.8")
}

tasks.test {
    // Use JUnit Platform for unit tests.
    useJUnitPlatform()
}

(Run gradle :dependencies in the directory to see that this is a valid project, and to make sure your gradle is working.)

Run fossa analyze --output using a build from master on the directory containing this file. Notice that no dependencies are found.

Then, switch to this PR, and try again. You should now get working dependencies.

Risks

I'm not sure how to actually unit test this change. @cnr, any ideas on unit testing this script?

If not, I think I'll add this to our integration tests once they merge back into Spectrometer (i.e. once https://github.com/fossas/team-analysis/issues/676 lands).

References

Resolves support conversation in Slack here.

Checklist

  • I added tests for this PR's change (or confirmed tests are not viable).
  • I linted and formatted (via haskell-language-server) any haskell files I touched in this PR.
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • I updated Changelog.md if this change is externally facing. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • I linked this PR to any referenced GitHub issues, if they exist.

@elldritch elldritch force-pushed the fix/gradle-no-such-element branch 3 times, most recently from d10d977 to 85fd648 Compare July 27, 2021 19:13
@elldritch elldritch marked this pull request as ready for review July 28, 2021 08:06
@elldritch elldritch requested a review from cnr July 28, 2021 08:06
Copy link
Contributor

@cnr cnr left a comment

Choose a reason for hiding this comment

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

Looks good generally; left a lot of mostly-minor comments


any ideas on unit testing this script

No, short of breaking the script out into a separate repo (which I'd certainly like to do).

Please pay particular attention to the documentation and comment changes. Do these help improve understandability?

Yes, these docs are great and seem approachable.

It might be helpful to occasionally link to key terms in a glossary or other relevant documentation -- especially for the warnings like "This strategy requires dynamic analysis", which probably wouldn't mean anything to me as a user (both what "dynamic analysis" means, and the impact of requiring it). I also likely wouldn't understand the ramifications of "This tactic provides a graph for subproject dependencies" without a link to additional documentation.

Additionally, linking to relevant documentation the first time we use our own defined terms like "tactic" or "strategy" or "analysis target" might be helpful, as those terms likely won't mean anything to a user directly navigating to the gradle docs page

docs/strategies/gradle.md Outdated Show resolved Hide resolved
docs/strategies/gradle.md Outdated Show resolved Hide resolved
docs/userguide.md Show resolved Hide resolved
scripts/jsondeps.gradle Outdated Show resolved Hide resolved
docs/strategies/gradle.md Outdated Show resolved Hide resolved
docs/strategies/gradle.md Outdated Show resolved Hide resolved
scripts/jsondeps.gradle Show resolved Hide resolved
scripts/jsondeps.gradle Outdated Show resolved Hide resolved
src/Strategy/Gradle.hs Outdated Show resolved Hide resolved
docs/strategies/gradle.md Outdated Show resolved Hide resolved
@elldritch elldritch force-pushed the fix/gradle-no-such-element branch from b122716 to 8ec3cca Compare August 2, 2021 09:05
@elldritch
Copy link
Member Author

Thanks for the thorough docs read!

@elldritch elldritch enabled auto-merge (squash) August 2, 2021 09:09
@elldritch elldritch merged commit d60d528 into master Aug 2, 2021
@elldritch elldritch deleted the fix/gradle-no-such-element branch August 2, 2021 09:28
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.

2 participants