Skip to content

Comments

Feature: Add java runtime (for PMD) [PLUTO-1420]#114

Merged
andrzej-janczak merged 18 commits intomainfrom
feature/linux-pmd
May 15, 2025
Merged

Feature: Add java runtime (for PMD) [PLUTO-1420]#114
andrzej-janczak merged 18 commits intomainfrom
feature/linux-pmd

Conversation

@andrzej-janczak
Copy link
Contributor

@andrzej-janczak andrzej-janczak commented May 13, 2025

feature: add Java runtime support with PMD tool integration

  • Add Java runtime plugin configuration for PMD analysis
  • Implement Java-specific version handling
  • Add Java binary paths and download templates
  • Configure PMD tool dependencies and execution paths

@andrzej-janczak andrzej-janczak changed the title Feature: linux pmd support [PLUTO-1420] Feature: Add java runtime (for PMD) [PLUTO-1420] May 14, 2025
@codacy-production
Copy link

codacy-production bot commented May 14, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.46% 20.42%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (796ade5) 4261 1328 31.17%
Head commit (1041ce4) 4481 (+220) 1376 (+48) 30.71% (-0.46%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#114) 289 59 20.42%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Copy link
Contributor

@machadoit machadoit left a comment

Choose a reason for hiding this comment

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

I see a problem. I think this is breaking. Which we can align if we want. Breaking on the sense, that if we merge this, people that had their PMD analysis working, will stop to work because it will fail to find the Java runtime.

Unless it is falling back to the Java that the client might have already on the system?

if len(pluginConfig.Environment) > 0 {
// Get runtime install directory if needed
var runtimeInstallDir string
if info.Runtime != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this block of code be inside the len(pluginConfig.Environment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think so, this should only run when we have Environment declared in yaml

configFile, exists := ConfigFileExists(config, "ruleset.xml")
if !exists {
// If no ruleset exists, copy the default ruleset
defaultRuleset := filepath.Join("tools", "pmd", "default-ruleset.xml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we making this changes on the scope of this PR? Seems off to me. And also, I think it is the responsibility of the step that generates the config files, to put the default config on the correct folder for the tools. I don't think it fits on each runner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now it's chicken and egg problem
Consider this scenario:

  1. You start from zero without having PMD
  2. Do codcacy-cli init
  3. You add PMD to yaml
  4. You do install - it works
  5. You do analyze - it's not working :(

If we plan installing tools inside analyze, so I think we definitely should fix missing tool conf file.
Also this is way is robust for PMD - which need PMD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway I am gonna to delete, but I would like to discuss this

defer os.Remove(tempResultFile)

config := *config.NewConfigType(testDirectory, repositoryCache, globalCache)
// Create a test configuration with PMD tool info
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can even delete this tests. I think the tests that validate if things work already exist on the new structure. For instance the https://github.com/codacy/codacy-cli-v2/tree/main/plugins/tools/pmd/test already has the test that will be run. So I think that test needs the update on adding the runtime on its own codacy.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Yasmin gonna remove those in her new PR
but I can also delete 👍

- node@22.2.0
- python@3.11.11
- dart@3.7.2
- java@17.0.10
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need remove this one from here, and add it only on the specific tool test. Does it make sense @zhamborova with the new strucuture? Then, if you don't have it here, the integration-test will fail if you don't put the java there

Copy link
Contributor Author

@andrzej-janczak andrzej-janczak May 14, 2025

Choose a reason for hiding this comment

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

yeah, we should stop using this for developing maybe
But do we need to start in my PR 😄

logger.Warn("Java binary not found. Continuing with the default Java runtime", logrus.Fields{
"expectedPath": javaBinary,
"error": err,
})
Copy link
Contributor Author

@andrzej-janczak andrzej-janczak May 14, 2025

Choose a reason for hiding this comment

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

@machadoit the "fallback" ^^^

… extension configuration

- Removed WindowsPath field from ToolBinary struct and related logic in ProcessTools function.
- Updated ExtensionConfig to eliminate Windows-specific extension handling.
- Adjusted Java and PMD plugin configurations to reflect these changes, removing Windows paths where applicable.
…mport cycle

The change resolves a circular dependency between utils and logger packages by:
- Create new constants package for shared permission constants
- Move DefaultFilePerms (0644) and DefaultDirPerms (0755) to constants
- Update utils and logger packages to use constants package
- Replace direct logrus usage with custom logger in extract.go

This improves the code organization by:
1. Eliminating import cycle between utils and logger
2. Centralizing permission constants in a dedicated package
3. Ensuring consistent logging through custom logger implementation

The change maintains all existing functionality while following Go best practices
for package organization.
…andling

This update enhances error logging in the ExtractTarGz and ExtractZip functions by replacing direct logrus calls with a custom logger. The changes ensure consistent logging practices and improve code readability while maintaining existing functionality. All error messages related to directory and file operations have been updated accordingly.
…nctions

This update simplifies the error handling in the ExtractTarGz and ExtractZip functions by removing redundant comments and consolidating error logging. The changes enhance code readability and maintain existing functionality while ensuring consistent error reporting. Unused symlink handling code has also been removed to clean up the implementation.
This commit deletes the cli-v2-linux binary file, streamlining the project by removing unnecessary artifacts. This change helps maintain a cleaner repository structure.
…gging

This update improves the InstallRuntime function by adding checks to determine if a runtime is already installed before proceeding with the installation. It also includes logging for both successful installations and errors related to missing binaries after extraction. These changes enhance the user experience by providing clearer feedback during the installation process.
…proved code clarity

This update introduces a new function, getMajorVersion, to encapsulate the logic for extracting the major version from a version string. This change reduces code duplication in the getFileName and getDownloadURL functions, enhancing maintainability and readability. Additionally, a new rule has been added to the cursor.mdc file to encourage avoiding code duplication.
@andrzej-janczak andrzej-janczak enabled auto-merge (squash) May 15, 2025 07:04
@andrzej-janczak andrzej-janczak merged commit a7a158c into main May 15, 2025
9 of 10 checks passed
@alerizzo alerizzo deleted the feature/linux-pmd branch June 3, 2025 09:45
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.

2 participants