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

feat: upgrade PMD to 7.5.0 #173

Closed
wants to merge 1 commit into from
Closed

feat: upgrade PMD to 7.5.0 #173

wants to merge 1 commit into from

Conversation

alanjaouen
Copy link

Hey, this is my first PR here so sorry if I missed something and please tell me If I

So, this PR is bassically just a run of npm run update-pmd to upgrade PMD engine to 7.5

Also I'm not sure about what to do with this part of the README

Any pull request submitted with updates to PMD MUST BE "CHECKSUMED"!

Feel free to reach

@ChuckJonas
Copy link
Owner

ChuckJonas commented Sep 16, 2024

Any pull request submitted with updates to PMD MUST BE "CHECKSUMED"!

This is really a reminder for the maintainers. For security reasons, we really shouldn't accept any PR that touches the binaries, as it would be very easy to compile PMD from source with malicious code. So we end up rerunning the upgrade scripts anyways (these should just be automated).

@adangel thoughts on officially not accepting any PR that touches the binaries?

@alanjaouen
Copy link
Author

@ChuckJonas thanks for the reply, I aggreed that I could have incorporated malicious code :/
Would you consider reopening the PR (or update it) with the binaries fetched on your side?

Also I could write a github action to automate this if you're up to

@ChuckJonas
Copy link
Owner

@alanjaouen If you want to take a stab at a github action, that would be great. It's been on the backlog for some time now.

#73

@adangel
Copy link
Collaborator

adangel commented Sep 17, 2024

@alanjaouen Thanks for the PR.
Do you have a specific reason you want to have PMD 7.5.0 shipped with this extension? Do you know, that you can just update PMD yourself by configuring the parameter pmdBinPath?

So we end up rerunning the upgrade scripts anyways (these should just be automated).

Exactly - I usually run it again, and if git doesn't show a diff, then I assume the binaries are not manipulated (to be exact - that I have the same binaries on my machine, e.g. in my local maven repository).

@adangel thoughts on officially not accepting any PR that touches the binaries?

I guess, as long as the repo contains binaries, we will receive PRs that will try to update the binaries. See also #171 ...

It's more a question of what we want - as PMD is released monthly. That would mean, we could release vscode-apex-pmd also monthly. I didn't do this in the past, because there is pmdBinPath, which I hoped, makes this simply unnecessary.

  1. Update vscode-apex-pmd on a monthly basis to always ship the latest PMD version
  2. Change vscode-apex-pmd to not include PMD, but to have it downloaded on the client when needed (not sure, if an extension is allowed to download additional files). This would require a bit more logic in the extension, as e.g. at least have a button "check for new PMD version". Ideally it would check automatically every now and then.
  3. Change vscode-apex-pmd, to download PMD during build-time. Then we only have a version number in the code, but no binaries. The upgrade script would simply be called during build. Then it depends on who is performing the release, whether any malicious binaries end up in the extension. If the release is performed via GitHub Actions, then it depends on GitHub, whether there are no malicious files in any caches.

In terms of automation, I've created #153 a while ago...
If we change the extension according to 3), then the action "update-pmd" is not needed anymore.

@alanjaouen
Copy link
Author

@alanjaouen Thanks for the PR. Do you have a specific reason you want to have PMD 7.5.0 shipped with this extension? Do you know, that you can just update PMD yourself by configuring the parameter pmdBinPath?

Hi @adangel

I'm working on a salesforce project with npm dependencies.
In our ci-cd build, we check codebase with forcedotcom/sfdx-scanner, which has been updated to use pmd 7.5.
pmd 7.4 intruduce (for us) a breaking change with the rule

Changed rules
ClassNamingConventions (Apex Codestyle): Two new properties to configure different patterns for inner classes and interfaces: innerClassPattern and innerInterfacePattern.

Updating our rulset for 7.5 make pmd having error on unexpected parameters with ChuckJonas/vscode-apex-pmd, as the included pmd is older

The forcedotcom/sfdx-code-analyzer-vscode is actually very inefficient, so we locally use ChuckJonas/vscode-apex-pmd to have a near real time linting.

I tried to use the param pmdBinPath to target the dist of forcedotcom/sfdx-code-analyzer-vscode but I only achieve to have pmd lib folder, and not the bin

I looked how to include the bin of pmd as a npm dependency, but I did'nt succeed

I hope my use case is clearer, and if you have any tips for me let me know

@adangel
Copy link
Collaborator

adangel commented Sep 19, 2024

The forcedotcom/sfdx-code-analyzer-vscode is actually very inefficient, so we locally use ChuckJonas/vscode-apex-pmd to have a near real time linting.

I tried to use the param pmdBinPath to target the dist of forcedotcom/sfdx-code-analyzer-vscode but I only achieve to have pmd lib folder, and not the bin

I looked how to include the bin of pmd as a npm dependency, but I did'nt succeed

I hope my use case is clearer, and if you have any tips for me let me know

In order to make this working locally, you need to download PMD from https://github.com/pmd/pmd/releases/latest, extract it and point pmdBinPath to the extracted folder. You can't use the PMD, that is included in sfdx-code-analyzer-vscode, as it is incomplete (as you figured out already).

@adangel
Copy link
Collaborator

adangel commented Sep 27, 2024

Closing in favor of #174 and #176

@adangel adangel closed this Sep 27, 2024
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