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

Jump to bottom

readline80 -> readline81 #108721

Merged
merged 6 commits into from Apr 24, 2021
Merged

readline80 -> readline81 #108721

merged 6 commits into from Apr 24, 2021

Conversation

lsix
Copy link
Member

@lsix lsix commented Jan 7, 2021

Motivation for this change

This PR updates readline-8.0 to readline-8.1. readline81 should be used with bash-5.1, which was forgotten when updating bash_5 from 5.0 to 5.1

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from lovek323, peti and dtzWill January 7, 2021 21:30
@SuperSandro2000
Copy link
Member

/rebase-staging

@github-actions github-actions bot changed the base branch from master to staging January 8, 2021 08:28
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 108721 run on x86_64-linux 1

1 package built:
  • readline81

@ajs124
Copy link
Member

ajs124 commented Feb 12, 2021

This needs a rebase.

@FRidh FRidh added this to the 21.05 milestone Feb 20, 2021
@FRidh FRidh added this to WIP in Staging via automation Feb 20, 2021
@FRidh
Copy link
Member

FRidh commented Mar 8, 2021

@lsix could you rebase this

@lsix
Copy link
Member Author

lsix commented Mar 9, 2021

@FRidh Done!

I have just rebuilt gdb after rebase, and everything looks ok.

@andreykaipov
Copy link

Hi - what needs to be done to get this merged in? I'd really like to try out that new bracketed-paste highlighting in Readline 8.1. 👀

@lsix
Copy link
Member Author

lsix commented Apr 14, 2021

As far as know, it should be OK.

I’ll rebase it to current staging shortly, since it as been some time since my last build on this PR.

@lsix
readline81: init at 8.1p0
ea3bdfc 
See https://lists.gnu.org/archive/html/info-gnu/2020-12/msg00002.html
for release information
@lsix
bash_5: upgrade readline dependency to 8.1
090b180 
Readline 8.1 is required for bash-5.1 to work properly.

From bash-5.1 release message[1]:

> Bash can be linked against an already-installed Readline library
> rather than the private version in lib/readline if desired.  Only
> readline-8.1 and later versions are able to provide all of the symbols
> that bash-5.1 requires; earlier versions of the Readline library will
> not work correctly.

[1] https://lists.gnu.org/archive/html/info-gnu/2020-12/msg00003.html
@lsix
gdb: upgrade readline dependency to 8.1
72c9089
@lsix
Copy link
Member Author

lsix commented Apr 14, 2021

Actually, some new packages rely on readline80 since I wrote those patches. I migrate them to readline81 and push updated patches.

@ajs124 ajs124 moved this from WIP to Needs review in Staging Apr 14, 2021
@lsix
Copy link
Member Author

lsix commented Apr 15, 2021

OK, I rebuilt the few derivations that still used readline80 with readline81, everything looks 👍

Sorry for not noticing those conflicts earlier.

Staging automation moved this from Needs review to Ready Apr 15, 2021
@SuperSandro2000
Copy link
Member

@lsix do we want to add an alias for readline80? Otherwise I would merge it.

@lsix
Copy link
Member Author

lsix commented Apr 22, 2021

@SuperSandro2000 I am not sure it is necessary to introduce alias for this, but I can if you wish.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Apr 22, 2021 •
edited

@SuperSandro2000 I am not sure it is necessary to introduce alias for this, but I can if you wish.

If anyone uses readline80 still then they would get an eval error instead of a nice message to use readline81. I think we should add one.

@lsix
readline80: delete
34a136c 
readline-8.0 is not used anywhere in the tree (since introduction of
readline-8.1); drop it.
@lsix
Copy link
Member Author

lsix commented Apr 22, 2021

Done.

@SuperSandro2000 SuperSandro2000 merged commit 9c2920e into NixOS:staging Apr 24, 2021
Staging automation moved this from Ready to Done Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@lovek323 lovek323 Awaiting requested review from lovek323

@peti peti Awaiting requested review from peti

@dtzWill dtzWill Awaiting requested review from dtzWill

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
Staging
  
Done
Milestone
21.05
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lsix @SuperSandro2000 @ajs124 @FRidh @andreykaipov @r-burns