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

Cache conflict between Maven/Ant and Eclipse/CLI run #3566

Closed
rnveach opened this Issue Nov 19, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Nov 19, 2016

While finishing up #3487, I noticed that running Checkstyle through Maven and CLI kept causing the cache to be erased. When looking into it, the main differences was the hash for the configuration.

Since I am using the exact same configuration, properties, settings, etc on the same workspace, I am expecting the 2 to work together, generate the same hash, and not keep erasing the cache when running the 2.

By default, when CLI runs a check with the check's severity as ignore, the check is still processed but violations are not printed by the DefaultLogger (which is also the same as XMLLogger). This is why they still appear as part of the configuration hash.
maven-checkstyle-plugin currently uses it's own version of the CLI so it still processes ignored checks like our CLI.
Ant run skips ignored checks because of omitIgnoredModules which is the heart of the problem.

Users don't expect checks with a configured severity of ignore on the check itself to be run, so this is how our CLI and Ant should be by default.

In terms of what this issue should do:

  1. We should add the option omitIgnoredModules to the CLI. It should default to on (TRUE) (like Ant), but can be turned off.
  2. All our uses in our ANT configs of omitIgnoredModules should be turned off (FALSE). We want to run every check to be a sort-of no-exception validation.
  3. We should make a request to maven-checkstyle-plugin to add the property omitIgnoredModules so users can specify it, and so we can make sure it stays off (FALSE) by default.
  4. Do similar requests to Eclipse-CS and IntelijIdea plugins.
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 19, 2016

Member

Analysis

When running mvn clean verify -DskipTests -DskipITs -Dpmd.skip=true -Dfindbugs.skip=true -Dcobertura.skip=true the hash is: 7E1C6368EC83470571F22FF289F835154436F6C1
Checkstyle is run by our Ant configuration which is defined in maven through the verify phase, not through the maven-checkstyle-plugin.

When running through the Main class with eclipse the hash is: 530A6B20AEC8B26B322F05E443420C5188CE07F6

Program Arguments: -c ${workspace_loc}\checkstyle\config\checkstyle_checks.xml ${workspace_loc}\checkstyle\src\it\java ${workspace_loc}\checkstyle\src\main\java ${workspace_loc}\checkstyle\src\main\resources ${workspace_loc}\checkstyle\src\test\java ${workspace_loc}\checkstyle\src\xdocs ${workspace_loc}\checkstyle\src\site
VM Arguments: -Dcheckstyle.header.file="${workspace_loc}\checkstyle\config\java.header" -Dcheckstyle.suppressions.file="${workspace_loc}\checkstyle\config\suppressions.xml" -Dcheckstyle.regexp.header.file="${workspace_loc}\checkstyle\config\java_regexp.header" -Dcheckstyle.importcontrol.file="${workspace_loc}\checkstyle\config\import-control.xml" -Dcheckstyle.cache.file="${workspace_loc}\checkstyle\target\cachefile"

Running directly with the CLI gave me the same results as running with Eclipse.

java -Dcheckstyle.header.file="M:\check
styleWorkspace\checkstyle\config\java.header" -Dcheckstyle.suppressions.file="M:
\checkstyleWorkspace\checkstyle\config\suppressions.xml" -Dcheckstyle.regexp.hea
der.file="M:\checkstyleWorkspace\checkstyle\config\java_regexp.header" -Dcheckst
yle.importcontrol.file="M:\checkstyleWorkspace\checkstyle\config\import-control.
xml" -Dcheckstyle.cache.file="M:\checkstyleWorkspace\checkstyle\target\cachefile
" -jar checkstyle-7.3-SNAPSHOT-all.jar -c M:\checkstyleWorkspace\checkstyle\conf
ig\checkstyle_checks.xml M:\checkstyleWorkspace\checkstyle\src\it\java M:\checks
tyleWorkspace\checkstyle\src\main\java M:\checkstyleWorkspace\checkstyle\src\mai
n\resources M:\checkstyleWorkspace\checkstyle\src\test\java M:\checkstyleWorkspa
ce\checkstyle\src\xdocs M:\checkstyleWorkspace\checkstyle\src\site

Looking at the raw output of the config that we hash using PropertyCacheFile.getHashCodeBasedOnObjectContent, it looks like there is an ordering conflict between the some checks and some numbering differences in some variables. This is what is causing the hashes to be different.
(Maven on left, Eclipse on right)
maven-vs-eclipse

Differences:

  • The checks missing from maven's ant are SeverityMatchFilter, MissingCtor, FinalParameters. They are all set to ignore in the configuration.
  • Number differences is 18h vs 19h, and 91h vs 93h. This adds up to 3 and since Eclipse has more, this is equal to the number of missing checks.

It looks like this is an issue with maven/ant dropping off checks that are ignored.


This is the culprit, omitIgnoredModules.

It was added with: 585f7c2#diff-78be33d1ef39645130e7853aa1b80a02 and has always had a default of true.

Member

rnveach commented Nov 19, 2016

Analysis

When running mvn clean verify -DskipTests -DskipITs -Dpmd.skip=true -Dfindbugs.skip=true -Dcobertura.skip=true the hash is: 7E1C6368EC83470571F22FF289F835154436F6C1
Checkstyle is run by our Ant configuration which is defined in maven through the verify phase, not through the maven-checkstyle-plugin.

When running through the Main class with eclipse the hash is: 530A6B20AEC8B26B322F05E443420C5188CE07F6

Program Arguments: -c ${workspace_loc}\checkstyle\config\checkstyle_checks.xml ${workspace_loc}\checkstyle\src\it\java ${workspace_loc}\checkstyle\src\main\java ${workspace_loc}\checkstyle\src\main\resources ${workspace_loc}\checkstyle\src\test\java ${workspace_loc}\checkstyle\src\xdocs ${workspace_loc}\checkstyle\src\site
VM Arguments: -Dcheckstyle.header.file="${workspace_loc}\checkstyle\config\java.header" -Dcheckstyle.suppressions.file="${workspace_loc}\checkstyle\config\suppressions.xml" -Dcheckstyle.regexp.header.file="${workspace_loc}\checkstyle\config\java_regexp.header" -Dcheckstyle.importcontrol.file="${workspace_loc}\checkstyle\config\import-control.xml" -Dcheckstyle.cache.file="${workspace_loc}\checkstyle\target\cachefile"

Running directly with the CLI gave me the same results as running with Eclipse.

java -Dcheckstyle.header.file="M:\check
styleWorkspace\checkstyle\config\java.header" -Dcheckstyle.suppressions.file="M:
\checkstyleWorkspace\checkstyle\config\suppressions.xml" -Dcheckstyle.regexp.hea
der.file="M:\checkstyleWorkspace\checkstyle\config\java_regexp.header" -Dcheckst
yle.importcontrol.file="M:\checkstyleWorkspace\checkstyle\config\import-control.
xml" -Dcheckstyle.cache.file="M:\checkstyleWorkspace\checkstyle\target\cachefile
" -jar checkstyle-7.3-SNAPSHOT-all.jar -c M:\checkstyleWorkspace\checkstyle\conf
ig\checkstyle_checks.xml M:\checkstyleWorkspace\checkstyle\src\it\java M:\checks
tyleWorkspace\checkstyle\src\main\java M:\checkstyleWorkspace\checkstyle\src\mai
n\resources M:\checkstyleWorkspace\checkstyle\src\test\java M:\checkstyleWorkspa
ce\checkstyle\src\xdocs M:\checkstyleWorkspace\checkstyle\src\site

Looking at the raw output of the config that we hash using PropertyCacheFile.getHashCodeBasedOnObjectContent, it looks like there is an ordering conflict between the some checks and some numbering differences in some variables. This is what is causing the hashes to be different.
(Maven on left, Eclipse on right)
maven-vs-eclipse

Differences:

  • The checks missing from maven's ant are SeverityMatchFilter, MissingCtor, FinalParameters. They are all set to ignore in the configuration.
  • Number differences is 18h vs 19h, and 91h vs 93h. This adds up to 3 and since Eclipse has more, this is equal to the number of missing checks.

It looks like this is an issue with maven/ant dropping off checks that are ignored.


This is the culprit, omitIgnoredModules.

It was added with: 585f7c2#diff-78be33d1ef39645130e7853aa1b80a02 and has always had a default of true.

@romani romani added the approved label Apr 22, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue May 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue May 25, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 25, 2017

Member

Issue for maven-checkstyle-plugin can be found at: https://issues.apache.org/jira/browse/MCHECKSTYLE-338
Issue for Eclipse-CS can be found at: https://sourceforge.net/p/eclipse-cs/feature-requests/169/

Member

rnveach commented May 25, 2017

Issue for maven-checkstyle-plugin can be found at: https://issues.apache.org/jira/browse/MCHECKSTYLE-338
Issue for Eclipse-CS can be found at: https://sourceforge.net/p/eclipse-cs/feature-requests/169/

rnveach added a commit to rnveach/checkstyle that referenced this issue May 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue May 25, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 1, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 1, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 12, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 12, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 12, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 13, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 13, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 13, 2017

@romani romani added the checkstyle8 label Jun 19, 2017

romani added a commit that referenced this issue Jun 19, 2017

@romani romani added this to the 8.0 milestone Jun 19, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 19, 2017

Member

fix is merged.

Member

romani commented Jun 19, 2017

fix is merged.

@romani romani closed this Jun 19, 2017

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