GunnarFarneback / LocalRegistry.jl

Create and maintain local registries for Julia packages.
Other
223 stars 21 forks source link

Better error messages for non-package projects #61

Closed grahamas closed 1 year ago

grahamas commented 1 year ago

Closes #60.

Checks that the Project.toml defines both version and uuid, and tells the user that it is missing if they are not found.

Also adds tests for these cases.

Currently two problems: 1) There's a failing test on my machine (see below). The failure is independent of my tests, though. 2) Properly, should also check that the Project.toml is associated with a file $(project_dir)/src/$(package_name).jl that defines a module of the correct name. I have a discourse post up to see if checking for being a valid package is already a solved problem (https://discourse.julialang.org/t/canonical-way-to-check-if-a-project-is-a-package/90827).

The error I get during testing (both with and without the changes on this branch)

Initialized empty Git repository in /tmp/LocalRegistryTestsGKMtSM/upstream/
error: src refspec master does not match any
error: failed to push some refs to 'file:///tmp/LocalRegistryTestsGKMtSM/upstream'
Register tests: Error During Test at /home/graham/git/LocalRegistry.jl/test/runtests.jl:35
  Got exception outside of a @test
  LoadError: failed process: Process(`git -C /tmp/LocalRegistryTestsGKMtSM/TestRegistryPush -c user.name=LocalRegistryTests -c user.email=localregistrytests@example.com push -u origin master`, ProcessExited(1)) [1]

  Stacktrace:
    [1] pipeline_error
      @ ./process.jl:565 [inlined]
    [2] run(::Cmd; wait::Bool)
      @ Base ./process.jl:480
    [3] run
      @ ./process.jl:477 [inlined]
    [4] create_registry(name_or_path::String, repo::String; description::Nothing, gitconfig::Dict{String, String}, uuid::Nothing, push::Bool)
      @ LocalRegistry ~/git/LocalRegistry.jl/src/LocalRegistry.jl:81
    [5] (::var"#48#49")(testdir::String)
      @ Main ~/git/LocalRegistry.jl/test/register.jl:228
    [6] with_testdir(f::var"#48#49")
      @ Main ~/git/LocalRegistry.jl/test/utils.jl:160
    [7] top-level scope
      @ ~/git/LocalRegistry.jl/test/register.jl:222
    [8] include(fname::String)
      @ Base.MainInclude ./client.jl:476
    [9] macro expansion
      @ ~/git/LocalRegistry.jl/test/runtests.jl:36 [inlined]
   [10] macro expansion
      @ ~/.local/opt/julia-1.8.3/share/julia/stdlib/v1.8/Test/src/Test.jl:1360 [inlined]
   [11] top-level scope
      @ ~/git/LocalRegistry.jl/test/runtests.jl:36
   [12] include(fname::String)
      @ Base.MainInclude ./client.jl:476
   [13] top-level scope
      @ none:6
   [14] eval
      @ ./boot.jl:368 [inlined]
   [15] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:276
   [16] _start()
      @ Base ./client.jl:522
  in expression starting at /home/graham/git/LocalRegistry.jl/test/register.jl:222
codecov-commenter commented 1 year ago

Codecov Report

Base: 96.57% // Head: 96.66% // Increases project coverage by +0.08% :tada:

Coverage data is based on head (d3692e4) compared to base (09b1008). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #61 +/- ## ========================================== + Coverage 96.57% 96.66% +0.08% ========================================== Files 1 1 Lines 263 270 +7 ========================================== + Hits 254 261 +7 Misses 9 9 ``` | [Impacted Files](https://codecov.io/gh/GunnarFarneback/LocalRegistry.jl/pull/61?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Gunnar+Farneb%C3%A4ck) | Coverage Δ | | |---|---|---| | [src/LocalRegistry.jl](https://codecov.io/gh/GunnarFarneback/LocalRegistry.jl/pull/61/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Gunnar+Farneb%C3%A4ck#diff-c3JjL0xvY2FsUmVnaXN0cnkuamw=) | `96.66% <100.00%> (+0.08%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Gunnar+Farneb%C3%A4ck). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Gunnar+Farneb%C3%A4ck)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

GunnarFarneback commented 1 year ago
  1. Looks like a plausible master/main clash. Cf #58.
  2. Checking the existence of a src/$(package_name).jl file is definitely enough. Any further would be going into linter territory, which is rather out of scope for this package.
grahamas commented 1 year ago

Ok, I think it's all done. It now checks for 1) version number, 2) UUID, and 3) module file at src/pkg_filename.jl. With tests for all three checks. It also tests handling package names like Flux.jl and Flux.exe and Flux.exe.jl correctly (I'm not totally sure the .exe is legal but like you said, not trying to be a linter--- local registries could do something weird).

You were right about the test failure: the tests run fine on my machine when I change defaultBranch to master.

aside: is register supposed to return nothing? why not return do_register?

GunnarFarneback commented 1 year ago

It also tests handling package names like Flux.jl and Flux.exe and Flux.exe.jl correctly

You've been overthinking this. The repository might be named Flux.jl but the package can't possibly be. Flux.jl would be syntax for the jl submodule of the Flux module.

GunnarFarneback commented 1 year ago

is register supposed to return nothing?

Yes, that's intentional. Cf https://github.com/GunnarFarneback/LocalRegistry.jl/pull/22#issuecomment-727644620.

grahamas commented 1 year ago

You've been overthinking this. The repository might be named Flux.jl but the package can't possibly be. Flux.jl would be syntax for the jl submodule of the Flux module.

Oof you're right. I got rid of the code accounting for that "possibility" in this commit.

GunnarFarneback commented 1 year ago

Looks great, thanks