relax `_include_this_registry` check

johnnychen94 commented 3 years ago

Sometimes one might prefer to use a fork URL for various reasons(e.g., private network, slow connection to GitHub). The current _include_this_registry check fails silently by returning false. This causes some hard to figure error:

# a private URL because connection to GitHub is slow here; one can use any General fork to reproduce this issue
url = "https://gitlab.lflab.cn/julialang/JuliaRegistries/General.git"
RegistryCI.test(registry_deps=[url])

┌ Error: It is not the case that all dependencies exist in the General registry
│   setdiff(depuuids, alluuids) =
│    Set{Base.UUID} with 1 element:
│      UUID("6fe1bfb0-de20-5000-8ca7-80f57d26f881")
└ @ RegistryCI ~/.julia/packages/RegistryCI/Nj1Mv/src/registry_testing.jl:187
BlockMatching: Test Failed at /Users/jc/.julia/packages/RegistryCI/Nj1Mv/src/registry_testing.jl:189
  Expression: depuuids ⊆ alluuids
   Evaluated: Set(Base.UUID[UUID("6fe1bfb0-de20-5000-8ca7-80f57d26f881")]) ⊆ Set(Base.UUID[UUID("3f19e933-33d8-53b3-aaab-bd5110c3b7a0"), UUID("8bf52ea8-c179-5cab-976a-9e18b702a9bc"), UUID("9fa8497b-333b-5362-9e8d-4d0656e87820"), UUID("6462fe0b-24de-5631-8697-dd941f90decc"), UUID("cf7118a7-6976-5b1a-9a39-7adc72f591a4"), UUID("d6f4376e-aef5-505a-96c1-9c027394607a"), UUID("fa267f1f-6049-4f14-aa54-33bafae1ed76"), UUID("05823500-19ac-5b8b-9628-191a04bc5112"), UUID("c8ffd9c3-330d-5841-b78e-0817d7145fa1"), UUID("9a3f8284-a2c9-5f02-9a11-845980a1fd5c")  …  UUID("8bb1440f-4735-579b-a4ab-409b98df4dab"), UUID("2f01184e-e22b-5df5-ae63-d93ebab69eaf"), UUID("a4e569a6-e804-4fa4-b0f3-eef7a1d5b13e"), UUID("4af54fe1-eca0-43a8-85a7-787d91b784e3"), UUID("14a3606d-f60d-562e-9121-12d972cd8159"), UUID("e66e0078-7015-5450-92f7-15fbd957f2ae"), UUID("2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"), UUID("56f22d72-fd6d-98f1-02f0-08ddc0907c33"), UUID("efcefdf7-47ab-520b-bdef-62a2eaa19f15"), UUID("de0858da-6303-5e67-8744-51eddeeeb8d7")])

This change introduces a lowercase(basename(x)) alternative and uses the set intersection operation to simplify the logic.

A debug log for easier review:

┌ Info: Registry match check
│   registry_spec =
│    Set{AbstractString} with 3 elements:
│      "General.git"
│      "General"
│      "general"
│   extra_deps =
│    Set{AbstractString} with 3 elements:
│      "https://gitlab.lflab.cn/julialang/JuliaRegistries/General.git.git"
│      "general.git"
└      "https://gitlab.lflab.cn/julialang/JuliaRegistries/General.git"
┌ Info: Registry match check
│   registry_spec =
│    Set{AbstractString} with 3 elements:
│      "https://github.com/JuliaRegistries/General.git.git"
│      "https://github.com/JuliaRegistries/General.git"
│      "general.git"
│   extra_deps =
│    Set{AbstractString} with 3 elements:
│      "https://gitlab.lflab.cn/julialang/JuliaRegistries/General.git.git"
│      "general.git"
└      "https://gitlab.lflab.cn/julialang/JuliaRegistries/General.git"

DilumAluthge commented 3 years ago

I don't think we should apply this relaxed behavior by default. Perhaps we can provide a keyword argument, and when the user sets this keyword argument to a certain value, then we apply the relaxed behavior, but otherwise we apply the regular behavior.

GunnarFarneback commented 3 years ago

I have no idea what this code is good for. load_registry_dep_uuids creates a temporary depot, installs a couple of registries, then loops over the registries to see if they actually match the list of registries that were just installed? If it were me I would just replace _include_this_registry with true, so I guess I'm missing something vital.

DilumAluthge commented 3 years ago

IIRC, we added _include_this_registry in https://github.com/JuliaRegistries/RegistryCI.jl/pull/367 to fix a bug (https://github.com/JuliaRegistries/RegistryCI.jl/pull/361#issuecomment-798948649).

DilumAluthge commented 3 years ago

Before #367, instead of _include_this_registry, we had this code:

if spec.url ∈ registry_deps_names || spec.name ∈ registry_deps_names

After #367, that code became:

if _include_this_registry(spec, registry_deps_names)

GunnarFarneback commented 3 years ago

Sorry, I still don't understand why there is an if statement there in the first place.

ericphanson commented 3 years ago

Sorry for the late review, but I agree with Gunnar that it looks like we create a fresh depot, then add the registries the user specifies, so it seems like we could just include them all and not have a check. What could the check filter out?

DilumAluthge commented 3 years ago

Hmmmm..... it's been so long that I don't remember why the code is structured this way.

Should we try changing the if conditional to if true and see if anything breaks?

ericphanson commented 3 years ago

Sure

JuliaRegistries / RegistryCI.jl

relax `_include_this_registry` check #426