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

[cli] Filenames printed as absolute paths in the report despite parameter --short-names #4026

Closed
hassanalamibmx opened this issue Jun 28, 2022 · 6 comments · Fixed by #4214
Closed
Assignees
Labels
a:bug PMD crashes or fails to analyse a file.
Milestone

Comments

@hassanalamibmx
Copy link

hassanalamibmx commented Jun 28, 2022

Affects PMD Version:

PMD 6.47.0

Description:

The PMD CLI option --short-names is expected to print shortned filenames, however the filenames are printed in absolute format.
This feature was working fine 6.46.0

Exception Stacktrace:

AVERTISSEMENT: This analysis could be faster, please consider using Incremental Analysis: https://pmd.github.io/pmd-6.47.0/pmd_userdocs_incremental_analysis.html

Code Sample demonstrating the issue:

pmd --rulesets ruleset.xml --file-list diff.txt --format xml --report-file pmd-report.xml --short-names

Steps to reproduce:

Please provide detailed steps for how we can reproduce the bug.

  1. Install pmd version 6.47.0 (latest)
  2. Create a diff.txt file with list of files to be scanned
  3. Create a ruleset file
  4. Run analysis using the following command:
    pmd --rulesets ruleset.xml --file-list diff.txt --format xml --report-file pmd-report.xml --short-names
  5. Filenames in the pmd-report.xml are printed as absolute paths

Running PMD through: [CLI]

@hassanalamibmx hassanalamibmx added the a:bug PMD crashes or fails to analyse a file. label Jun 28, 2022
@hassanalamibmx hassanalamibmx changed the title [cli] Filenames printed as absolute paths in the report despite parameter --short-names [cli] Filenames printed as absolute paths in the report despite parameter --short-names Jun 28, 2022
@oowekyala
Copy link
Member

oowekyala commented Jun 28, 2022

Thanks for the report. I remember commenting somewhere about this, the paths are only shortened if you input a relative path in the first place. If your file list contains absolute paths they will remain absolute. I couldn't find in which PR this was merged, and I don't think it was for PMD 6.47... That's maybe something #4000 uncovered. I'll take a harder look later. Edit: found it, it's only for PMD 7 #3893 (comment)

That change still makes sense to me so I don't know if it's really a bug, though the behavior is surprising in this case...

@hassanalamibmx
Copy link
Author

hassanalamibmx commented Jun 28, 2022

Hello @oowekyala, thanks for your response, but my file list contains relative paths

@oowekyala oowekyala added this to the 6.48.0 milestone Jul 10, 2022
@oowekyala oowekyala self-assigned this Jul 25, 2022
@oowekyala
Copy link
Member

So I could observe an inconsistent behavior, namely that the --file-list option does not behave as -d and essentially ignores --short-names. But I could reproduce that with PMD 6.44 and 6.43 as well so this does not appear to be a recent regression as indicated in the report...

@hassanalamibmx could you check again whether this is a regression in PMD 6.47, and if so, provide a reproducible MWE?


Here's what I observed with PMD 6.43, 6.44 and 6.47:

              | --short-names | default
---------------------------------------
rel file list | abs           | abs
abs file list | abs           | abs
-d relpath    | rel           | abs
-d abspath    | rel           | abs

Ideally -d and --file-list would behave the same... OTOH, there is a technical problem here. The path shortening procedure works by removing from the path any prefix that has been mentioned on the command line with -d. When you use a file list without -d, nothing is shortened, as we don't know of the common prefix the user wants to remove in this case.

Maybe a solution would be to replace the --short-names option with a --relativize-paths-with <a directory>, since that would work both for file lists and -d. I think such an option would be better than our current --short-names, and I don't see a way to adapt --short-names to work with file lists consistently with -d otherwise.

@adangel
Copy link
Member

adangel commented Jul 29, 2022

I can see, that this feature appears to be working with 6.46.0 - but that is only accidentally: With fixing #3999 the behavior of 6.45.0 was restored. It only appears to be working in 6.46.0, because we used accidentally the current working directory for -d and then used this as a base path for --short-name. But: 6.46.0 analyzed all files twice, so you got duplicated violations.

Maybe the following would be a way to make this magically work without configuring an explicit base path (which gives more flexibility, because you can control it): If we get the file-list, we try to find the longest common path prefix of all files and use this for shortening the paths. I guess, if the filelist only contains relative paths, then we would need to use the current working directory as the base path. If it contains mixed paths, then we would report only absolute paths.

This could be a way to make it work for 90% of the use cases and without adding additional CLI parameters. The short-names-option belongs to reporting options like --property, --report-file, --show-suppressed and of course --format. If we add a new parameter, maybe we should name it like --report-relativize-paths-with, to make it clear it is a reporting option.

@oowekyala
Copy link
Member

Maybe the following would be a way to make this magically work without configuring an explicit base path (which gives more flexibility, because you can control it): If we get the file-list, we try to find the longest common path prefix of all files and use this for shortening the paths. I guess, if the filelist only contains relative paths, then we would need to use the current working directory as the base path. If it contains mixed paths, then we would report only absolute paths.

I think the problem with this strategy is that eg if we use a filelist with the files in pmd-java/src/main/java, we would report paths relative to pmd-java/src/main/java/net/sourceforge/pmd because all the java files have n.s.pmd as common package... In a more extreme case, if you want to run PMD on only changed files (like OP is apparently doing), and a couple of files are changed in the same package, then you'd get just the filenames and no directories. This can make the behavior of --short-names enormously different with just a change to the insides of the file list, and no change to your command line args. I don't think this is intuitive, and it makes the file list option less useful as you can't opt out...

Alternatively, we could say that we always relativize relative to the current working dir. It's less flexible and is not compatible with the current behavior of -d + --short-names. I would argue this is more intuitive than the current behavior, as changing -d can change how paths are rendered in the report which might be unintended, and again, you can't opt out. Also I never understood what --short-names did exactly until I used a debugger to look into it... I think in an ideal world we would use the current working dir by default AND have a --relativize-with option for flexibility.

--report-relativize-paths-with

I don't really like that... It's not English... Maybe --render-paths-relative-to? Alternatively, it could be a property declared on all renderers and be set with -P?

@adangel adangel modified the milestones: 6.48.0, 6.49.0 Jul 30, 2022
@oowekyala oowekyala modified the milestones: 6.49.0, 6.50.0 Aug 30, 2022
@oowekyala
Copy link
Member

Proposed fix for next version:

  • introduce a new parameter, eg --relativize-paths-with <dir> or --shorten-names-with <dir> or so
  • deprecate --short-names without changing its behavior

@adangel adangel modified the milestones: 6.50.0, 6.51.0 Sep 30, 2022
@adangel adangel modified the milestones: 6.51.0, 6.52.0 Oct 25, 2022
@adangel adangel modified the milestones: 6.52.0, 6.53.0 Nov 22, 2022
@adangel adangel modified the milestones: 6.53.0, 6.54.0 Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:bug PMD crashes or fails to analyse a file.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants