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

[core] Non-XML output breaks XML-based CLI integrations #1036

Closed
SCWells72 opened this Issue Apr 11, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@SCWells72
Copy link

SCWells72 commented Apr 11, 2018

Affects PMD Version:
6.2.0

Rule:
All

Description:
It looks like in recent builds when you specify -f xml, some non-XML information is written to stdout causing XML-based integrations such as my IDE (Illuminated Cloud) to break. For example, here's he result of an otherwise successful execution:

This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/pmd-6.2.0/pmd_userdocs_getting_started.html#incremental-analysis …<?xml version="1.0" encoding="UTF-8"?><pmd ...>...</pmd>

When the CLI is run for structured output, nothing but the structured output should be emitted to stdout/stderr. Otherwise parsing of the contents of the streams will fail unless some form of scrubbing/pre-processing is performed by every integrating client.

Running PMD through:

While found through the integration with Illuminated Cloud, this is easily reproducible via the CLI.

@SCWells72

This comment has been minimized.

Copy link

SCWells72 commented Apr 11, 2018

Looping in @rsoesemann for context.

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Apr 11, 2018

@SCWells72 thanks for your report and your concern. However, I do not agree with your position.

Messages such as the one you cite are written to stderr (as many other messages may), the output of the analysis is written to stdout unless redirected to a file. These are two separate and distinct streams any integration should be able to differentiate easily regardless of the operating system. As long as we don't mix both I believe our usage is correct and we should never silence a warning just because no output file has been chosen.

Why is it impossible to differentiate streams in your case?

If you believe we are mixing streams please let me know. That would be a huge bug.

Unrelated, but also of note, an IDE integration is a prime use-case for the analysis cache, you should consider supporting it.

@adangel

This comment has been minimized.

Copy link
Member

adangel commented Apr 12, 2018

@SCWells72 When running PMD from command line, there is also the -r / -reportfile option. This directly writes the report into a separate (file) stream and avoids the possible mixes with warnings/errors, if they are accidentally written to stdout.

I agree, that nothing else than the report should go to stdout. If there is such output, then it's a bug. But stderr is still useful to report errors/warnings and can easily separated via redirections (e.g. ... > report.xml 2> error.log).

Related to this issue: #897

@SCWells72

This comment has been minimized.

Copy link

SCWells72 commented Apr 12, 2018

I am separating stdout from stderr already. The issue is that the message is included in both streams.

Regarding stdout vs. stderr and expectations, if the programmatic contract with the CLI is expressed via both stdout and stderr, then both should provide structured responses. If the contract is always communicated via stdout, only it should provide a structured response and stderr can be free-form.

I integrate other CLIs with the IDE, notably the Salesforce DX CLI, and when you run it with a --json argument, the exit code determines which stream you should consume, both of which are written in JSON format.

If PMD is only intended to write a structured XML response to stdout when run with the -f xml argument, I agree that it doesn't matter how stderr is formatted. The contract just needs to be communicated/documented enough for integrators.

@SCWells72

This comment has been minimized.

Copy link

SCWells72 commented Apr 12, 2018

Here you go. I just ran this:

pmd.bat -dir /path/to/source -language apex -R /path/to/apex-ruleset.xml -f xml 2> pmd.err > pmd.out

and here's the first few lines of pmd.out:

This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/pmd-6.2.0/pmd_userdocs_getting_started.html#incremental-analysis
<?xml version="1.0" encoding="UTF-8"?>
<pmd xmlns="http://pmd.sourceforge.net/report/2.0.0"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://pmd.sourceforge.net/report/2.0.0 http://pmd.sourceforge.net/report_2_0_0.xsd"
    version="6.2.0" timestamp="2018-04-12T10:59:24.989">

and here's pmd.err:

Apr 12, 2018 10:59:24 AM net.sourceforge.pmd.PMD processFiles
WARNING: This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/pmd-6.2.0/pmd_userdocs_getting_started.html#incremental-analysis

That's the real issue.

@jsotuyod jsotuyod added the a:bug label Apr 12, 2018

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Apr 12, 2018

@SCWells72 awesome, I'll look into this

@jsotuyod jsotuyod added this to the 6.3.0 milestone Apr 12, 2018

@jsotuyod jsotuyod changed the title [apex] Non-XML output breaks XML-based CLI integrations [core] Non-XML output breaks XML-based CLI integrations Apr 12, 2018

@rsoesemann

This comment has been minimized.

Copy link
Contributor

rsoesemann commented Apr 17, 2018

@jsotuyod couldn't @SCWells72 turn off this Incremental Analysis in the PMDConfiguration.class? At least as a workaround?

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Apr 17, 2018

@SCWells72 @rsoesemann in 6.2.0 it's possible to avoid that warning by explicitly disabling the incremental analysis (see changelog)

Non the less, an IDE is a prime use-case for the cache, as you analyze, edit only a few files, analyze again, and so on. I'd rather recommend making use of the feature.

@SCWells72

This comment has been minimized.

Copy link

SCWells72 commented Apr 17, 2018

@jsotuyod Thanks for the tip on a workaround. I'll include it in my next build. And I'll definitely look at incremental analysis as an enhancement going forward for sure.

@jsotuyod jsotuyod self-assigned this Apr 22, 2018

@jsotuyod jsotuyod added the has:pr label Apr 23, 2018

adangel added a commit that referenced this issue Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment