Skip to content

add ExplicitImports to CI check #4326

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

KristofferC
Copy link
Member

Had this in #4288

@github-project-automation github-project-automation bot moved this to New in Pkg.jl Jul 16, 2025
@IanButterworth IanButterworth moved this from New to In progress in Pkg.jl Jul 16, 2025
@ericphanson
Copy link
Contributor

we do have https://github.com/JuliaTesting/ExplicitImports.jl/blob/main/.pre-commit-hooks.yaml in case you want to use that, fyi. https://github.com/JuliaTesting/ExplicitImports.jl/tree/main?tab=readme-ov-file#pre-commit-hooks

@IanButterworth
Copy link
Member

@ericphanson I'm struggling to get the recommended script to work because it expects ExplicitImports to be a dep of the main Pkg Project.toml and I don't think exposes a way to specify a different Project.toml and retain the pwd that files should be evaluated from?

@ericphanson
Copy link
Contributor

Cc @abelsiqueira

@abelsiqueira
Copy link

In the linting workflow, we explicitly add ExplicitImports and JuliaFormatter (https://github.com/JuliaBesties/BestieTemplate.jl/blob/main/.github/workflows/Lint.yml#L32).
For the local development, we install these packages globally.

The language: julia for pre-commit did not exist when we created the pre-commit hook for explicit imports, so it doesn't support additional_dependencies (https://pre-commit.com/#julia).
@ericphanson, we could update the hook to use the Julia language, like https://github.com/fredrikekre/runic-pre-commit/blob/master/.pre-commit-hooks.yaml. What do you think?

@ericphanson
Copy link
Contributor

I got it working with #4329 but it needs a pre-commit change to support in-repo hooks for julia. So if we want it soon we'd have to move the hooks out I think. Or maybe the best course is to revert to 2dcbe44 for now since it seems like that was working.

@KristofferC KristofferC requested a review from a team as a code owner July 29, 2025 10:14
@KristofferC KristofferC changed the title add ExplicitImports pre-commit code check add ExplicitImports to CI check Jul 29, 2025
@KristofferC
Copy link
Member Author

I changed this to just run on CI. @ericphanson, are these bugs in ExplicitImports:

  using UUIDs: UUIDs # used at /Users/kc/JuliaPkgs/Pkg.jl/src/REPLMode/REPLMode.jl:8:7

  using Dates: @dateformat_str # used at /Users/kc/JuliaPkgs/Pkg.jl/src/Registry/Registry.jl:668:57

The first one seems wrong, at that code location the source code is

using UUIDs: UUID

The second one has an explicit import for that symbol:

using Dates: @dateformat_str, Dates, DateTime, Millisecond, Second, now

@KristofferC KristofferC force-pushed the kc/explicit_check branch 4 times, most recently from 14befd3 to 4f1417e Compare July 29, 2025 15:37
@ericphanson
Copy link
Contributor

I changed this to just run on CI. @ericphanson, are these bugs in ExplicitImports:

  using UUIDs: UUIDs # used at /Users/kc/JuliaPkgs/Pkg.jl/src/REPLMode/REPLMode.jl:8:7

  using Dates: @dateformat_str # used at /Users/kc/JuliaPkgs/Pkg.jl/src/Registry/Registry.jl:668:57

The first one seems wrong, at that code location the source code is

using UUIDs: UUID

The second one has an explicit import for that symbol:

using Dates: @dateformat_str, Dates, DateTime, Millisecond, Second, now

probably... IIRC there is an issue where if I removed these it would fail to recognize that using UUIDs: UUIDs was needed at all and would call it stale. So I think they are buggy but kind of a workaround.

@KristofferC
Copy link
Member Author

I don't really understand this:

artifacts.jl: Error During Test at /Users/runner/work/Pkg.jl/Pkg.jl/test/runtests.jl:83
Got exception outside of a @test
LoadError: ArgumentError: Package Artifacts not found in current path.

  • Run import Pkg; Pkg.add("Artifacts") to install the Artifacts package.

Artifacts is a dependency... Maybe it collides with Pkg.Artifacts somehow.

@IanButterworth
Copy link
Member

Or PkgArtifacts? #4230

@KristofferC
Copy link
Member Author

aqua.jl: Error During Test at /home/runner/work/Pkg.jl/Pkg.jl/test/runtests.jl:83
  Got exception outside of a @test
  LoadError: ArgumentError: Package Aqua [4c88cf16-eb10-579e-8560-4a9242c79595] is required but does not seem to be installed:
   - Run `Pkg.instantiate()` to install all reco

 Error During Test at /home/runner/work/Pkg.jl/Pkg.jl/test/runtests.jl:83
  Got exception outside of a @test
  LoadError: ArgumentError: Package HistoricalStdlibVersions [6df8b67a-e8a0-4029-b4b7-ac196fe72102] is required but does not seem to be installed:
   - Run `Pkg.instantiate()` to install all recorded dependencies.

what is going on here... why cant it load any packages

Comment on lines -1441 to +1440
for depot in Pkg.depots()
for depot in Pkg.Base.DEPOT_PATH
Copy link
Member

Choose a reason for hiding this comment

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

This seems odd?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I removed depots() and replaced it with Base.DEPOT_PATH. This is weird (should just be Base.DEPOT_PATH) but I don't think it should change anything..

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

Successfully merging this pull request may close these issues.

4 participants