Skip to content

Issue #980: Replace AntBuilder.copy() with Files.walkFileTree()#1006

Merged
romani merged 1 commit into
checkstyle:masterfrom
vivek-0509:issue-980-refactorAntBuilderCopy
Dec 27, 2025
Merged

Issue #980: Replace AntBuilder.copy() with Files.walkFileTree()#1006
romani merged 1 commit into
checkstyle:masterfrom
vivek-0509:issue-980-refactorAntBuilderCopy

Conversation

@vivek-0509

@vivek-0509 vivek-0509 commented Dec 26, 2025

Copy link
Copy Markdown
Member

Issue #980:

What is fixed:

  • Changed targetDir.startsWith(sourceDir)dir.startsWith(destinationDir) since some of the valid directories were getting skipped which caused ci errors.

Why earlier code failed:

  • Previous check skipped directories when dest was inside source
  • Caused missing files and CI build failures

Improvements:

  • NOT Added VCS directory excludes (.git, .svn, .hg, CVS, etc.)
  • NOT Added VCS file excludes (.gitignore, .gitattributes, etc.)
  • NOT Matches AntBuilder's defaultexcludes behavior

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question

Comment thread checkstyle-tester/diff.groovy Outdated
FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {

if (vcsDirectories.contains(dir.fileName?.toString())) {
return FileVisitResult.SKIP_SUBTREE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we copy all files as is ?
Copy code should be simple copy without any context.

@vivek-0509 vivek-0509 Dec 26, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added the VCS exclusions (.git, .svn, etc.) to match the original AntBuilder.copy() behavior, which applies Ant's "defaultexcludes" by default. Without these exclusions, we would copy .git/ directories and other VCS artifacts like .gitignore, .gitattributes, which is unnecessary since these files are not needed for Checkstyle analysis. However, if you prefer a simpler copy without context, I can remove these exclusions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you share link to any implementation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not see that need some filtering out, we just copy folders, extra hidden folders to copy should be ok

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Official Ant Documentation: https://ant.apache.org/manual/dirtasks.html#defaultexcludes

Ant Source Code (DirectoryScanner.java):
https://github.com/apache/ant/blob/f862774a9e2d2afe3d25d7116f0672eeb4f3e9bf/src/main/org/apache/tools/ant/DirectoryScanner.java#L170

When Copy.java needs to copy files, it calls FileSet.getDirectoryScanner() to get the list of files. Internally, FileSet creates a DirectoryScanner which automatically calls addDefaultExcludes() to add VCS patterns to its exclude list. The scanner then returns only non-excluded files to Copy, so VCS directories are never copied.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do not see that need some filtering out, we just copy folders, extra hidden folders to copy should be ok

Will update accordingly..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@vivek-0509 vivek-0509 force-pushed the issue-980-refactorAntBuilderCopy branch from 41f387f to 3349663 Compare December 26, 2025 21:50
@romani romani merged commit 4a316ae into checkstyle:master Dec 27, 2025
9 checks passed
@romani

romani commented Dec 27, 2025

Copy link
Copy Markdown
Member

lets try one more time ....

@romani

romani commented Dec 27, 2025

Copy link
Copy Markdown
Member

triggered checkstyle/checkstyle#18312 (comment)

@romani

romani commented Dec 27, 2025

Copy link
Copy Markdown
Member

triggered checkstyle/checkstyle#17575 (comment)

@romani

romani commented Dec 27, 2025

Copy link
Copy Markdown
Member

CI is tested at checkstyle/checkstyle#18184 (review)

Works well.

@romani

romani commented Dec 27, 2025

Copy link
Copy Markdown
Member

This time all works.
Thanks a lot!!!

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