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

Go: Fix buggy FindAllFilesWithName implementation #17900

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mbg
Copy link
Member

@mbg mbg commented Nov 4, 2024

Summary

It seems that our implementation of FindAllFilesWithName contained bugged logic where dirsToSkip was typically given a list of directory names (such as vendor) and then compared these to the relative path of directories discovered by filepath.WalkDir (such as ./vendor). This resulted in go.mod and go.work files in vendor directories not getting skipped correctly, which was reported in #17893.

Approach

@smowton implemented a handy isGolangVendorDirectory predicate as part of #17227, which already implements the correct check. To not duplicate this logic, I decided to move isGolangVendorDirectory from that package to the shared util one. I then modified FindAllFilesWithName to accept such predicate functions as arguments instead of an array of strings. I changed calls to FindAllFilesWithName to use a (pre-defined) array containing the isGolangVendorDirectory predicate instead of the "vendor" string.

I then added a problematic vendor directory to the mixed-layout integration test, containing a go.work file.

Alternatives

Instead of modifying FindAllFilesWithName to accept a list of predicates, we could just check that each path discovered by filepath.WalkDir ends in one of the excluded directory names.

Open questions

@mbg mbg self-assigned this Nov 4, 2024
@mbg mbg requested a review from a team as a code owner November 4, 2024 12:57
@github-actions github-actions bot added the Go label Nov 4, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

There are failing integration tests that need to be fixed.

It seems broadly sensible. It seems a bit odd to have the functionality to specify arbitrary directories to skip when we only ever use it for vendor directories. It might be simpler to either always skip vendor directories or have a boolean for skipping them. I'm unsure of the right thing to do when CODEQL_EXTRACTOR_GO_EXTRACT_VENDOR_DIRS is true - I guess we should look for go.work and go.mod files in vendor directories in that case, but I defer to @smowton when he is back.

@smowton
Copy link
Contributor

smowton commented Nov 11, 2024

I would suppose yes we should look in the vendor dir in that case, though I can't say I'm 100% confident that will work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants