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

ClasspathElementDir review adjustments #851

Conversation

attilapuskas
Copy link

Based on the following comments:
#850 (comment)
#850 (comment)

Attila Puskas added 2 commits April 19, 2024 09:04
The file attributes at this point have already been requested in
order to check whether the subPath is a regular file and so there
is no need for an additional isDir call.
To point is that when we already know from some paths to be just
regular files, but not directories then at the end we do not need
to check if these are directories. But rather than copying the
list we can use iterators to remove the already discovered files.
@lukehutch
Copy link
Member

LGTM. Thanks!

@lukehutch lukehutch merged commit 0bf6d6d into classgraph:latest Apr 19, 2024
31 of 33 checks passed
@lukehutch
Copy link
Member

Released in 4.8.172.

@attilapuskas
Copy link
Author

Thanks!
Do you know by any chance why build (windows-latest, 15 & 18) have failed? They give the results in different order, but I have no idea how the commits could cause such a thing.

@lukehutch
Copy link
Member

I opened a bug about that. There's some nondeterminism in the classpath resolution order which is not supposed to be there (the order is supposed to be deterministic, despite parallel scanning). I don't know what could cause that, and it's going to be a hard bug to get to the bottom of. I don't have the time to look into it right now. But if you look at the failing test, you might be able to figure it out.

The issue occurs when a classpath element adds additional classpath elements to the scan, partway through the scan. Specifically, when a manifest has a Class-Path entry.

@attilapuskas
Copy link
Author

I see, thanks for the detailed explanation!

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.

None yet

2 participants