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

Turn on Config Cache File Locally for Developers #3487

Closed
rnveach opened this Issue Oct 1, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Oct 1, 2016

http://checkstyle.sourceforge.net/config.html#Checker property cacheFile.
It speeds up checkstyle's run by not rechecking files that have no issues and have not changed.

I want to turn that on locally, but currently the only way to do that is to modify the config for each branch I make.
I feel this will help developers who have to constantly re-run the checkstyle before sending a PR to the server. Some computers that are slower take a few minutes to run this command, so any performance boost is appreciated.

I assume we can make like a maven property which we can override locally that will set it when we run the checkstyle, that way it doesn't change CI behavior and turn it on there.

When I run locally in Eclipse, I can run it either through maven:

mvn verify -Dmaven.test.skip=true -Dpmd.skip=true -Dfindbugs.skip=true -Dcobertura.skip=true -Dforbiddenapis.skip=true

or directly running Main:

Main class: com.puppycrawl.tools.checkstyle.Main

Program Arguments: -c M:\checkstyleWorkspaceEclipse\checkstyle\config\checkstyle_checks.xml M:\checkstyleWorkspaceEclipse\checkstyle\src\it\java M:\checkstyleWorkspaceEclipse\checkstyle\src\main\java M:\checkstyleWorkspaceEclipse\checkstyle\src\test\java M:\checkstyleWorkspaceEclipse\checkstyle\src\xdocs

VM arguments: -Dcheckstyle.header.file="M:\checkstyleWorkspaceEclipse\checkstyle\config\java.header" -Dcheckstyle.suppressions.file="M:\checkstyleWorkspaceEclipse\checkstyle\config\suppressions.xml" -Dcheckstyle.regexp.header.file="M:\checkstyleWorkspaceEclipse\checkstyle\config\java_regexp.header" -Dcheckstyle.importcontrol.file="M:\checkstyleWorkspaceEclipse\checkstyle\config\import-control.xml"

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 2, 2016

Member

It appears we do set the location for the cache file in the ant-phase-verify, but we don't use it anywhere in the configuration:

<property key="checkstyle.cache.file" file="${mvn.project.build.directory}/cachefile"/>

Seems all we have to do is add it to the configuration file and developers can hook into it, through maven or ant or any other process.

Question: Is it safe to put the cache file into the build directory? Seems like it could accidentally get picked up in the maven clean commands we are required to run in various situations, like cobertura.

Member

rnveach commented Oct 2, 2016

It appears we do set the location for the cache file in the ant-phase-verify, but we don't use it anywhere in the configuration:

<property key="checkstyle.cache.file" file="${mvn.project.build.directory}/cachefile"/>

Seems all we have to do is add it to the configuration file and developers can hook into it, through maven or ant or any other process.

Question: Is it safe to put the cache file into the build directory? Seems like it could accidentally get picked up in the maven clean commands we are required to run in various situations, like cobertura.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 13, 2016

Member

yes we should activate cache in our config -

<property name="fileExtensions" value="java, properties, xml, g, g4"/>
, http://checkstyle.sourceforge.net/config.html#Checker
No changes on Travis is required, as it should do all validation without any cache to avoid any leak of violations due to cache problems.

Please rename file from "cachefile" to "checkstyle.cache" to make it clear that that is not maven file.

Question: Is it safe to put the cache file into the build directory?

yes, when you run "mvn clean ...." you force maven to clean all caches and .... from all plugins and phases. If smb want to reuse he need to not use "clean" phase in his commands.

Member

romani commented Oct 13, 2016

yes we should activate cache in our config -

<property name="fileExtensions" value="java, properties, xml, g, g4"/>
, http://checkstyle.sourceforge.net/config.html#Checker
No changes on Travis is required, as it should do all validation without any cache to avoid any leak of violations due to cache problems.

Please rename file from "cachefile" to "checkstyle.cache" to make it clear that that is not maven file.

Question: Is it safe to put the cache file into the build directory?

yes, when you run "mvn clean ...." you force maven to clean all caches and .... from all plugins and phases. If smb want to reuse he need to not use "clean" phase in his commands.

@romani romani added the approved label Oct 13, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 10, 2016

Member

@romani Because we don't support multi-file validation (TranslationCheck) (just like the remaining issue #3493) and that check is enabled in our configuration currently. Turning cache on with the check will create issues.

To continue with this issue we must disable the check in the default config and have a special CI only config for it. There are already a few checks that will only show up in CI (IntelliJ).

Our only other option is to abandon this issue completely until multi-file validation can be sorted out.

Member

rnveach commented Nov 10, 2016

@romani Because we don't support multi-file validation (TranslationCheck) (just like the remaining issue #3493) and that check is enabled in our configuration currently. Turning cache on with the check will create issues.

To continue with this issue we must disable the check in the default config and have a special CI only config for it. There are already a few checks that will only show up in CI (IntelliJ).

Our only other option is to abandon this issue completely until multi-file validation can be sorted out.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 10, 2016

Member

We should not disable this Check in our config as we have a rule to use all Checks. Yes, we have problems , fortunately it will not be a problem for users, it will not be a problem for CIs, it might be a problem for contributors (but very rare use case, unlikely to happen). Contributors will come to us and we will explain this.

@rnveach , We should enable caching by default to let you do development verification stage quicker.

Member

romani commented Nov 10, 2016

We should not disable this Check in our config as we have a rule to use all Checks. Yes, we have problems , fortunately it will not be a problem for users, it will not be a problem for CIs, it might be a problem for contributors (but very rare use case, unlikely to happen). Contributors will come to us and we will explain this.

@rnveach , We should enable caching by default to let you do development verification stage quicker.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 20, 2016

Member

Fix is merged

Member

romani commented Nov 20, 2016

Fix is merged

@romani romani closed this Nov 20, 2016

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