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

relax _include_this_registry check #426

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnnychen94
Copy link
Contributor

@johnnychen94 johnnychen94 commented Jun 12, 2021

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

IIRC, we added _include_this_registry in #367 to fix a bug (#361 (comment)).

@DilumAluthge
Copy link
Member

DilumAluthge commented Jun 12, 2021

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
Copy link
Contributor

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

@ericphanson
Copy link
Member

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?

@ericphanson ericphanson removed their request for review September 17, 2021 10:25
@DilumAluthge
Copy link
Member

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
Copy link
Member

Sure

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

4 participants