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

[MDEP-799] tree: add optional output type json #391

Merged
merged 24 commits into from
May 23, 2024

Conversation

LogFlames
Copy link
Contributor

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.

Same as #325, will squash and update commit in the end, once everything is fixed.

  • Format the pull request title like [MDEP-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MDEP-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.

  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.

  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.

  • You have run the integration tests successfully (mvn -Prun-its clean verify).

  • I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004

  • In any other case, please file an Apache Individual Contributor License Agreement.

This is a continuation of #325, which adds JSON output type to the maven-depdenceny-plugin with the goal tree.

The tests were disabled in the previous fork, to enable the tests (in a working state) this is rebased to the current master (2024-05-16), if the history looks weird (have some 'double' commits, unsure how to solve it). Two tests exists.

Discussion: IOException The JsonDependencyNodeVisitor now uses the PrintWriter which swallows IOExceptions, as the other Node Visitors do. Could not get it to throw an IOException due to the function being defined in the interface without throwing an Exception, unsure if this can be solved without a larger rework?

Discussion: Circular Dependency I have added a test for circular dependency, as well as a visited set to stop infinite recursion. This currently exists silently, but I feel there should be some sort of Exception or warning. Due to the same reason as above (interface not throwing exception) I could not find a way to do this nicely. Guessing RuntimeException("Detected Circular Dependency") is not an option? Happy to receive help with ideas/suggestions/opinions!

I think all other requested changes from #325 are addressed.

Sorry, something went wrong.

@LogFlames
Copy link
Contributor Author

CLA is now registered.

@LogFlames
Copy link
Contributor Author

The workflow must be approved by a maintainer. @elharo can you help?

@elharo
Copy link
Contributor

elharo commented May 20, 2024

Apparently not. I'm not sure who has permissions to do that.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

This could use a test that parses the output JSON (to verify it's correct in total) and then verifies the individual components. Use javax.json for this to avoid third party dependencies that have long histories of security bugs, abandonment, or both.

@elharo
Copy link
Contributor

elharo commented May 20, 2024

Error: Errors:
Error: TestTreeMojo.testTreeJsonCircularDependency:179 » FileNotFound /Users/runner/work/maven-dependency-plugin/maven-dependency-plugin/target/unit-tests/treetree2.json (No such file or directory)

@slawekjaranowski
Copy link
Member

Apparently not. I'm not sure who has permissions to do that.

@elharo you can simply click Approve and run

@slawekjaranowski
Copy link
Member

  • PR should be rebased
  • a unit test or integration test will be appreciated

@LogFlames
Copy link
Contributor Author

I'll add a test parsing the JSON using javax.json as suggested.
Having some trouble with the workflow on MacOS, I'll try to debug it using a local workflow run, will write once it's resolved.

@LogFlames
Copy link
Contributor Author

The verify checks should now pass (verified in my fork), MacOS as well.

A new test has been added that parses and verifies Json output.

@LogFlames
Copy link
Contributor Author

LogFlames commented May 22, 2024

I apolagize for the ugly history, had some trouble with my previous merge when trying to rebase.
The CI/testcases currently fail, I will update once CI runs again on my fork. :)

@elharo
Copy link
Contributor

elharo commented May 22, 2024

don't worry about history; it's all just squashed anyway

/*
* Test parsing of Json output and verify all key-value pairs
*/
public void testTreeJsonParsing() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you declare a more specific exception?

Copy link
Contributor Author

@LogFlames LogFlames May 22, 2024

Choose a reason for hiding this comment

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

This uses the prebuilt function runTreeMojo that is also used by most other tests in the file, which throws an Exception. I don't think this can be more specific without major changes to runTreeMojo.
Within runTreeMojo there are function that throws the following exceptions:
lookupMojo: Exception
setVariableValueToObject: IllegalAccessException
this.stubFactory.createArtifact: IOException
mojo.execute(): MojoFailureException and MojoExecutionException

@elharo elharo merged commit 09775b3 into apache:master May 23, 2024
19 checks passed
@monperrus
Copy link

This feature is important.

Thanks @LogFlames and @elharo

Any ETA for the corresponding release?

@slawekjaranowski
Copy link
Member

Any ETA for the corresponding release?

There is on my list ... I hope do next release 3.7.0 it at next week or two

There is issues list https://issues.apache.org/jira/issues/?jql=project%20%3D%20MDEP%20AND%20fixVersion%20%3D%203.7.0

@monperrus
Copy link

There is on my list ... I hope do next release 3.7.0 it at next week or two

excellent, thanks!

@pombredanne
Copy link

Everyone thank you ++ and @LogFlames 🙇 ❤️

You have rendered obsolete about 22K files on GitHub that try to parse the output of tree!
See https://github.com/search?q=mvn+"dependency%3Atree"&type=code

@LogFlames @monperrus I guess you plan to use it in https://github.com/chains-project/maven-lockfile ?

FWIW, on my side this is going to be used in a front end to the https://github.com/nexB/scancode.io/ code scanner and matcher:

@slawekjaranowski
Copy link
Member

Thanks again everybody for contribution ... but we are lack of documentation, so I created issue:
https://issues.apache.org/jira/browse/MDEP-933

@LogFlames @elharo or someone else can you look.

@pombredanne
Copy link

@slawekjaranowski re: doc, I was about to post a comment here too!

mvn dependency:tree -DoutputFile=/path/to/file
... thank for catching this.

@cstamas
Copy link
Member

cstamas commented May 24, 2024

I wonder why various (somewhat related) topics are mentioned here, while Resolver expected checksums is not. Demo for it is here https://github.com/cstamas/tc-demo

@slawekjaranowski
Copy link
Member

@slawekjaranowski re: doc, I was about to post a comment here too!

mvn dependency:tree -DoutputFile=/path/to/file

... thank for catching this.

We can add in mentioned place a new format, but I think that we should publish structure of generated json somewhere a new page will be the best for it in my opinion

@monperrus
Copy link

@LogFlames @monperrus I guess you plan to use it in chains-project/maven-lockfile ?

@pombredanne yes and in other Chains experiments

Demo for it is here cstamas/tc-demo

@cstamas interesting, this is related to how we store checksums in lockfiles in https://github.com/chains-project/maven-lockfile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants