-
Notifications
You must be signed in to change notification settings - Fork 12
Normalize version range spacing in compress function #107
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
Normalize version range spacing in compress function #107
Conversation
|
This issue is causing more and more JLL updates to get stuck. Can we get this merged & deployed? |
60f8d0b to
3859aff
Compare
|
This issue only appeared some time last week. Another option would be to undo the respective change. ( |
|
Can we get a regression test? My understanding is that this issue is Julia-version specific, which also explains why it popped up only recently in BinaryBuilder (we switched to Julia v1.12 only last week) |
|
Bump |
|
I missed that Dilum had rebased on master after #109 was merged. Since the downstream tests pass I'm confident enough to merge this. However, it doesn't seem to be entirely effective. With there are still entries like where I will add a commit to attempt to solve this and we'll see if that passes the downstream tests. |
|
I still think more tests here would be useful, instead of just relying on downstream ones 🙂 |
|
Absolutely, I wouldn't hesitate accepting a PR that adds tests. |
|
But until some volunteer gets to that, the downstream tests are a lot better than nothing. Now I need to investigate whether the LocalRegistry regression tests found something significant or something harmless. Won't happen until tomorrow though. Ah, RegistryCI also flagged something. Then it's probably a real problem. |
|
I thought you had a test right there: #107 (comment) 🙂 |
|
Not really, that involves far too much visual inspection and may or may not be on point. Frankly I still don't understand why this is causing errors in JLLs where it otherwise just has made the registry files less compact than they could be. |
|
As far as I understand, RegistryTools doesn't realise |
|
Sure, but why do overlapping bounds only happen for JLLs? Registrator has had this issue in production for quite a while now, and the only visible effect has been a loss of conciseness. |
|
Because we switched to Julia v1.12 last week. |
|
That explains the timing but not why only JLLs. The extra spacing was introduced in JuliaLang/Pkg.jl#3580 and has been present since Julia 1.11. Registrator has been running with this since July, which has resulted in a lot of split ranges but never in an overlapping range. I have never seen a bug report of this for LocalRegistry, and I would expect at least some users to be on 1.11 or 1.12. Are the JLLs using RegistryTools differently somehow? |
|
All usage of RegistryTools is in this file: https://github.com/JuliaPackaging/BinaryBuilder.jl/blob/ee05c44e940128fca1fa9980c6cc00c78a21e88e/src/AutoBuild.jl. We don't have any test about registration. Last time I attempted to understand RegistryTools code I wanted to scream in the void: #94 |
|
Presumably https://github.com/JuliaPackaging/BinaryBuilder.jl/blob/ee05c44e940128fca1fa9980c6cc00c78a21e88e/src/AutoBuild.jl#L634-L643 is what matters. That's a function I don't call from LocalRegistry but Registrator should be using it. With knowledge of the argument values and using a registry fork on local disk, it may be possible to extract a stand-alone reproducer. |
|
I know what is wrong with my latest change and will fix it tonight. In the absence of useful tests, would it be possible to run this PR in production to see if it empirically solves the problem? |
|
The special thing that happened here might be that we tried to register a new version that differs only in its build number ( I assume what happened was approximately the following:
In other words, we first registered dependencies for |
|
That sounds plausible, I think all cases where we've seen the issue were in non |
|
Thanks. That's a good hint and maybe something I can verify with LocalRegistry. |
|
Indeed I can. With Julia 1.12 and RegistryTools 2.4.1 in the manifest, this script generates the original diff in JuliaRegistries/General#143840 and with this PR instead, it generates the diff after the corrections. That's good enough verification for me. |
Fix an issue where version ranges with different spacing (e.g., "4.3.0-4" vs. "4.3.0 - 4")
are treated as different keys in the compressed dictionary, causing duplicate entries
instead of merging them properly.
This change strips all spaces from version range strings before using them as dictionary
keys, ensuring consistent treatment regardless of formatting variations.
This is a temporary work-around that will become unnecessary when "all this is rewirtten to use VersionNumbers", as suggested in the beginning of this file.