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.

Sign up for GitHub

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

lua5_4: add readline support #107885

Closed
wants to merge 1 commit into from
Closed

lua5_4: add readline support #107885

wants to merge 1 commit into from

Conversation

nagy
Copy link
Member

@nagy nagy commented Dec 29, 2020

Motivation for this change

Introduce readline support for lua5_4. This makes the handling in the REPL nicer.

The problem with this patch is, that it cannot be disabled. It would be better to put this into interpreter.nix so that the readline argument can be overriden/disabled. However, that would also affect previous lua versions, which seem to detect readline already.

Also future Lua version probably will need this as well.

CC @raboof since you introduced lua5_4 initially.

Any thoughts ?

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.

@SuperSandro2000
Copy link
Member

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

2 packages built:
  • lua5_4
  • lua5_4_compat

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 29, 2020 •
edited

Result of nixpkgs-review pr 107885 run on x86_64-darwin 1

2 packages failed to build:
  • lua5_4
  • lua5_4_compat
clang -DLUA_USE_LINUX -O2 -fPIC -DLUA_USE_READLINE=1   -c -o luac.o luac.c                                                                                                                  clang -o luac -lreadline luac.o liblua.a -lm -lreadline                                                                                                                                     clang -shared -ldl -Wl,-soname,liblua.so.5.4 -o liblua.so.5.4.2 lapi.o lcode.o lctype.o ldebug.o ldo.o ldump.o lfunc.o lgc.o llex.o lmem.o lobject.o lopcodes.o lparser.o lstate.o lstring.o ltable.o ltm.o lundump.o lvm.o lzio.o lauxlib.o lbaselib.o lcorolib.o ldblib.o liolib.o lmathlib.o loadlib.o loslib.o lstrlib.o ltablib.o lutf8lib.o linit.o -lm                           ld: unknown option: -soname                                                                                                                                                                 clang-7: error: linker command failed with exit code 1 (use -v to see invocation)                                                                                                           make[2]: *** [Makefile:65: liblua.so] Error 1                                                                                                                                               make[2]: Leaving directory '/private/tmp/nix-build-lua-5.4.2.drv-0/lua-5.4.2/src'                                                                                                           make[1]: *** [Makefile:134: macosx] Error 2                                                                                                                                                 make[1]: Leaving directory '/private/tmp/nix-build-lua-5.4.2.drv-0/lua-5.4.2/src'                                                                                                           make: *** [Makefile:55: macosx] Error 2

@nagy nagy marked this pull request as ready for review December 29, 2020 20:05
raboof
raboof reviewed Dec 31, 2020
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it'd be good to build lua with readline support.

Rather than adding the define 'manually', I wonder if it'd be possible to specify linux-readline as the target platform for 5.4?

pkgs/development/interpreters/lua-5/default.nix Outdated Show resolved Hide resolved
@raboof
Copy link
Member

raboof commented Dec 31, 2020

Result of nixpkgs-review pr 107885 run on x86_64-darwin 1
2 packages failed to build:

This is not a new failure on this PR, though, right?

@nagy
Copy link
Member Author

nagy commented Dec 31, 2020

With @raboof 's help I was able to move the change into interpreter.nix, which is better suited. This way it is possible to disable readline support as well. In addition, all future lua versions will now use this as well and all older version's derivations should not even have changed.

@nagy
Copy link
Member Author

nagy commented Dec 31, 2020

Also, during my testing I had no problems with setting enableParallelBuilding to true. Do you want me to set that as well while we are at it?

raboof
raboof approved these changes Dec 31, 2020
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@raboof
Copy link
Member

raboof commented Dec 31, 2020

Also, during my testing I had no problems with setting enableParallelBuilding to true. Do you want me to set that as well while we are at it?

I'm not opposed to that 👍 (might be good to double-check it doesn't introduce unreproducability with nix-build '<nixpkgs>' -A lua5_4 --check)

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review. 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 107885 run on x86_64-linux 1

2 packages built:
  • lua5_4
  • lua5_4_compat

@nagy
Copy link
Member Author

nagy commented Jan 1, 2021

Also, during my testing I had no problems with setting enableParallelBuilding to true. Do you want me to set that as well while we are at it?

I'm not opposed to that +1 (might be good to double-check it doesn't introduce unreproducability with nix-build '<nixpkgs>' -A lua5_4 --check)

I have pushed another commit, that enables it, after a nix-build -A lua5_4 --check worked out fine. Keep in mind that this causes a larger rebuild of all lua dependant packages.

@raboof
Copy link
Member

raboof commented Jan 2, 2021

Keep in mind that this causes a larger rebuild of all lua dependant packages.

Hmm, indeed - so much that with this change this PR should probably target staging rather than master.

@nagy nagy changed the base branch from master to staging January 2, 2021 13:18
@nagy nagy force-pushed the lua5_4-readline branch 2 times, most recently from f065d65 to 0c097b1 Compare June 28, 2021 21:50
@nagy
Copy link
Member Author

nagy commented Jun 28, 2021

I have resolved the merge conflicts for this

@nagy nagy changed the base branch from staging to master October 6, 2021 09:16
@Artturin
Copy link
Member

Artturin commented Oct 15, 2021 •
edited

made #141785 but did not search for existing PRs

the plat is still linux-readline even with readline ? null. do we even need a option to disable readline?

@Artturin Artturin closed this Oct 16, 2021
@nagy nagy deleted the lua5_4-readline branch October 17, 2021 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raboof raboof raboof approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@peti peti Awaiting requested review from peti

Assignees
No one assigned
Labels
6.topic: lua 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nagy @SuperSandro2000 @raboof @Artturin