maintainers/scripts/remove-old-aliases.py: convert aliases to warnings before throwing or removing; aliases: cleanup#427017
Conversation
|
Of course I decided to look and see if there were other PRs after I started, and found #414664. I'll incorporate that into this. |
03917b2 to
b067834
Compare
|
You might be interested in my work at #442066. This would also affect these scripts big-time. |
|
That PR looks great! This served as a good reminder for me to cherry-pick the fixes to aliases.nix I made here (missing dates + an extra "throw" in the exception string). Those would be helpful for your solution too, I think. They are in #442683 now. |
b067834 to
d5c6c69
Compare
While I have been progressing big time behind the scenes working with @emilazy on structured aliases, by-name etc. - this work won't make it into the 25.11 release. Thus, I think it would make a lot of sense to pick up this PR here again and clean up all the aliases we currently have. Removing old aliases, turning others into warnings - this will allow us to remove a lot more of them in the next cycle, because we already had the deprecation phase. @mdaniels5757 what's the current state of the PR? Do you need to put some work into it first, or do you need more review first? It would be great to get this done before the release. |
|
A few thoughts:
|
There was a problem hiding this comment.
Thanks for this. I’d like to land this for sure, as it’s a clear incremental improvement, even if I do hope we can obsolete it by 26.05!
In terms of how to handle the (hopefully) one‐off run, I think we should drop everything that was already a throw in 25.05, convert everything else that was in 25.05 to a throw, and add warnings to aliases added in 25.11. For better or worse we already in practice turn many things into throws without warnings every release cycle, and there’s no concern about the overlapping support window if we restrict ourselves to making aliases present in 25.05 throws. This will help avoid migrating many old aliases to by-name.
Unfortunately it’s kind of annoying to determine this cut‐off reliably, since our date‐based system is not actually tied to when a merge happened, let alone the release cycle. I did some research:
- The latest addition date of a simple currently‐not‐throwing alias present in the 25.05 release tag is 2025‐05‐19.
- The earliest addition date of a simple currently‐not‐throwing alias that is not the 25.05 release tag is… nominally 2021‐08‐16 because something went wrong with the PR adding
linuxPackages_6_13_hardened,linux_6_13_hardened,linuxPackages_6_14_hardened, andlinux_6_14_hardened, but actually 2025‐05‐13. All exceptfollow,lima-bin, andzintwere added in June or later.zintis the only one of those missing alib.warnOnInstantiate. - The latest addition date of a currently‐throwing simple alias that was already a
throwin the 25.05 release tag isrke2_testingfrom 2025‐06‐02; the latest before that was 2025-05-18. - The earliest addition date of a currently‐throwing simple alias that was not already a
throwin the 25.05 release tag is 2024‐03‐24. The ones before 2025‐05‐19 areafpfs-ng,ats,axmldec,buildXenPackage,clamsmtp,cloudlogoffline,dbench,ggobi,insync-emblem-icons,lesstif,libgadu,libmp3splt,libpseudo,libviper,mp3splt,nix-plugin-pijul,polipo,presage,rke2_1_29,setserial,siproxd,sloccount,subberthehut,suitesparse_4_2,suitesparse_4_4,syncthing-tray,tkgate,unicap,vwm,xmlroff.
Therefore:
- We can safely add warnings to every alias with a date of 2025‐05‐20 or later.
- We should manually add
lib.warnOnInstantiatetozint. - We can drop every
throwwith a date of 2025‐05‐18 or earlier, except for my list of exceptions. - We can convert every non‐throwing, non‐preserved alias with a date of 2025‐05‐19 or earlier into a
throw, except for my list of exceptions.
So the script should be a big help here, in combination with a little extra work.
Note that my methodology is messy and ignores multi‐line aliases and stuff. It also doesn’t catch aliases to things that indirectly throw. We probably have to manually check everything that isn’t a simple one‐line alias for this, but there should be much fewer of those than what’s covered by this. I could have always made mistakes here.
I derived these with let diff = a: b: removeAttrs a (builtins.attrNames b); aliases = ref: let nixpkgs = import (builtins.getFlake "nixpkgs/${ref}"); in diff (nixpkgs { }) (nixpkgs { config.allowAliases = false; }); in builtins.attrNames … and a lot of gross rg pipelines.
| converted = ( | ||
| f"{indent}{alias} = lib.warnOnInstantiate \"'{alias_unquoted}' has been" | ||
| f" renamed to/replaced by '{replacement}'\" {replacement};" | ||
| f" # Converted to warning {datetime.today().strftime('%Y-%m-%d')}" | ||
| ) |
There was a problem hiding this comment.
Unfortunately, some aliases point to non‐derivations, and lib.warnOnInstantiate will silently produce weird results for those. This can’t be detected without eval. I think that with the timeline we’re hoping for this isn’t a blocker, though – we can manually run a check to ensure that everything being passed to lib.warnOnInstantiate is a derivation after conversion.
d5c6c69 to
a59a88a
Compare
|
diff --git a/maintainers/scripts/remove-old-aliases.py b/maintainers/scripts/remove-old-aliases.py
index 5db821768f..9d224229be 100755
--- a/maintainers/scripts/remove-old-aliases.py
+++ b/maintainers/scripts/remove-old-aliases.py
@@ -154,14 +154,14 @@
f" renamed to/replaced by '{replacement}'\";"
f" # Converted to throw {datetime.today().strftime('%Y-%m-%d')}"
)
- converted_lines.append((line, converted))
+ converted_lines[line] = converted
elif convert_to == "warnings":
converted = (
f"{indent}{alias} = lib.warnOnInstantiate \"'{alias_unquoted}' has been"
f" renamed to/replaced by '{replacement}'\" {replacement};"
f" # Converted to warning {datetime.today().strftime('%Y-%m-%d')}"
)
- converted_lines.append((line, converted))
+ converted_lines[line] = converted
else:
raise ValueError("'convert_to' must be either 'throws' or 'warnings'")
@@ -170,23 +170,18 @@
def generate_text_to_write(
txt: list[str],
- date_older_list: list[str],
- converted_to_throw: list[tuple[str, str]],
- date_older_throw_list: list[str],
- converted_to_warning: list[tuple[str, str]],
- date_older_warning_list: list[str],
+ converted_lines: dict[str, str],
) -> list[str]:
"""generate a list of text to be written to the aliasfile"""
text_to_write: list[str] = []
- combined_lists = date_older_list + date_older_throw_list + date_older_warning_list
for line in txt:
text_to_append: str = ""
- for tupl in (converted_to_throw + converted_to_warning):
- if line == tupl[0]:
- text_to_append = f"{tupl[1]}\n"
- break
- if line not in combined_lists:
- text_to_append = f"{line}\n"
+ try:
+ new_line = converted_lines[line]
+ if new_line is not None:
+ text_to_write.append(f"{newline}\n")
+ except KeyError:
+ text_to_write.append(f"{line}\n")
if text_to_append:
text_to_write.append(text_to_append)
@@ -246,16 +241,16 @@
date_older_warning_list,
) = get_date_lists(txt, cutoffdate)
- converted_to_warning: list[tuple[str, str]] = []
- converted_to_throw: list[tuple[str, str]] = []
+ converted_lines: dict[str, str] = {}
+
if date_older_list and "aliases" in args.operate_on:
- converted_to_warning = convert(date_older_list, "warnings")
+ converted_lines.update(convert(date_older_list, "warnings"))
print(" Will be converted to warnings. ".center(100, "-"))
for l_n in date_older_list:
print(l_n)
if date_older_warning_list and "warnings" in args.operate_on:
- converted_to_throw = convert(date_older_list, "throws")
+ converted_lines.update(convert(date_older_list, "throws"))
print(" Will be converted to throws. ".center(100, "-"))
for l_n in date_older_warning_list:
print(l_n)
@@ -263,6 +258,7 @@
if date_older_throw_list and "throws" in args.operate_on:
print(" Will be removed. ".center(100, "-"))
for l_n in date_older_throw_list:
+ converted_lines[l_n] = None
print(l_n)
if date_too_complex_list:
@@ -276,14 +272,7 @@
print(l_n)
if not args.dry_run:
- text_to_write = generate_text_to_write(
- txt,
- date_older_list,
- converted_to_throw,
- date_older_throw_list,
- converted_to_warning,
- date_older_warning_list,
- )
+ text_to_write = generate_text_to_write(txt, converted_lines)
write_file(aliasfile, text_to_write)
|
|
(Uh, not quite, a type issue remains! Should have tested something other than |
|
Further fixes: diff --git a/maintainers/scripts/remove-old-aliases.py b/maintainers/scripts/remove-old-aliases.py
index 9d224229be..e2124e4306 100755
--- a/maintainers/scripts/remove-old-aliases.py
+++ b/maintainers/scripts/remove-old-aliases.py
@@ -116,7 +116,7 @@
def convert(lines: list[str], convert_to: str) -> list[tuple[str, str]]:
"""convert a list of lines to either "throws" or "warnings"."""
- converted_lines = []
+ converted_lines = {}
for line in lines.copy():
indent: str = " " * (len(line) - len(line.lstrip()))
@@ -179,7 +179,7 @@
try:
new_line = converted_lines[line]
if new_line is not None:
- text_to_write.append(f"{newline}\n")
+ text_to_write.append(f"{new_line}\n")
except KeyError:
text_to_write.append(f"{line}\n")
if text_to_append:
@@ -250,7 +250,7 @@
print(l_n)
if date_older_warning_list and "warnings" in args.operate_on:
- converted_lines.update(convert(date_older_list, "throws"))
+ converted_lines.update(convert(date_older_warning_list, "throws"))
print(" Will be converted to throws. ".center(100, "-"))
for l_n in date_older_warning_list:
print(l_n)With this it seems to work pretty well, but it will produce e.g. brasero-original = throw "'brasero-original' has been renamed to/replaced by 'lib.warnOnInstantiate'"; # Converted to throw 2025-10-26when turning warnings into |
|
Both bugs should be fixed now! I ended up going in a different direction than you proposed, but thanks for the patches! |
Man this script is not very nice
99d7cab to
65cc1d1
Compare
I would keep it: this script can also be used for e.g. python-aliases, which has inherit.
I guess it's ready now! Should actually running the script on the aliases file be a separate PR, or done in this one? I had envisioned doing it separately, I think. |
|
I just got through doing some more fixes and running it with the necessary manual work I talked about :) Will report back soon. |
…ings Also revert the addition of the "elif '"' in line:" condition: lines with quotes are generally OK. Co-authored-by: Emily <vcs@emily.moe>
The script tries to avoid complex aliases, which includes aliases with if expressions. But it is not smart enough to recognize if it is in a string or not! Fortunately, it's case sensitive.
Co-authored-by: Emily <vcs@emily.moe>
65cc1d1 to
b9114ba
Compare
emilazy
left a comment
There was a problem hiding this comment.
I put up my work on doing the clean‐up in #456065, and I’ve pushed my script changes to the branch – hope you don’t mind! I had a few more fixes and edge cases handled locally already, and the change to use a dictionary should at least be a performance win if nothing else (not that it matters much). Let me know what you think, but I think this should be good to merge now.
|
I did not look at the script changes in-depth, but I'll trust Emily's extensive testing by actually running this thing to produce the other PR. Nothing will break, if it's not 100%, yet - we can still fix things, if we find more stuff during review of the other PR. |
This is a big one, but it's broken down into many smaller commits.
Things done
foobar = foo;). (This was, and would still be, manually done, and the user would still need to write the date the alias was added.)--only-throwsoption in favor of--only, which lets users pass it multiple times to specify exactly which changes they want to make.MANUAL(case-sensitive), which makes the script skip the line it is on.ifin lowercase in quotes. (The script skips those.)MANUALwhere needed.Add a 👍 reaction to pull requests you find important.