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

lua: support non-Darwin non-Linux OSes; clean up #119860

Merged
merged 5 commits into from Apr 19, 2021

Conversation

alyssais
Copy link
Member

Motivation for this change

See commit messages.

With these changes I can now build pkgsCross.x86_64-netbsd.lua.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • B ilt 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.

@alyssais alyssais added 6.topic: bsd Running or building packages on BSD 6.topic: exotic Exotic hardware or software platform 6.topic: lua labels Apr 19, 2021
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Apr 19, 2021
@alyssais alyssais changed the title lua: support non-Darwin non-Linux OSes lua: support non-Darwin non-Linux OSes; clean up Apr 19, 2021
@github-actions github-actions bot removed the 6.topic: bsd Running or building packages on BSD label Apr 19, 2021
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 119860 at 9c2b5350 run on x86_64-linux 1

60 packages marked as broken and skipped:
  • bareos
  • clickshare-csc1
  • dockbarx
  • gnunet-gtk
  • grass
  • hydrus
  • libsForQt512.discover
  • libsForQt512.elisa
  • libsForQt512.phonon-backend-vlc
  • libsForQt514.discover
  • ...
248 packages failed to build:
550 packages skipped due to time constraints:
  • EBTKS
  • _90secondportraits
  • adapta-gtk-theme
  • aegisub
  • alephone
  • alephone-durandal
  • alephone-eternal
  • alephone-evil
  • alephone-infinity
  • alephone-marathon
  • ...
27 packages built successfully:
  • dell-530cdn
  • gutenprintBin
  • highlight
  • imapfilter
  • kakounePlugins.quickscope-kak
  • lite
  • lsyncd
  • lua (lua5 ,lua5_2_compat)
  • lua5_1 (lua51Packages.lua)
  • lua51Packages.luxio
  • lua51Packages.vicious
  • lua51Packages.wrapLua
  • lua5_2 (lua52Packages.lua ,luaPackages.lua)
  • luaPackages.luxio (lua52Packages.luxio)
  • luaPackages.vicious (lua52Packages.vicious)
  • luaPackages.wrapLua (lua52Packages.wrapLua)
  • lua5_3 (lua53Packages.lua)
  • lua53Packages.luxio
  • lua53Packages.vicious
  • lua53Packages.wrapLua
  • lua5_3_compat
  • lua5_4
  • lua5_4_compat
  • nice-dcv-client
  • perl532Packages.RPM2
  • rpm (python38Packages.rpm)
  • toluapp
1 suggestion:
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/development/interpreters/lua-5/interpreter.nix:112:3:

        |
    112 |   meta = {
        |   ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

@alyssais
Copy link
Member Author

Looks like LUA_USE_LINUX was doing something after all. Investigating...

@alyssais
Copy link
Member Author

@rmcgibbo could you try that again please? :)

This is supposed to be automatically set by Lua's Makefile if PLAT is
set appropriately, but it was being overridden by us overridding
CFLAGS.  Setting it manually was a hack.  The correct thing to do was
to make sure SYSCFLAGS (where Lua's Makefile puts LUA_USE_LINUX) was
still included in our custom CFLAGS.
@alyssais alyssais added the 6.topic: bsd Running or building packages on BSD label Apr 19, 2021
Copy link
Member

@7c6f434c 7c6f434c left a comment

Choose a reason for hiding this comment

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

Diff looks fine

] else [])
;
"PLAT=${plat}"
"CC=${stdenv.cc.targetPrefix}gcc"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"CC=${stdenv.cc.targetPrefix}gcc"
"CC=${stdenv.cc.targetPrefix}cc"

not sure how this works on darwin without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes I think you're right. Do we have any way of testing Darwin at the moment, since it doesn't seem to be working on OfBorg?

Copy link
Member

Choose a reason for hiding this comment

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

I can test it, but it's gonna take a bit.

@SuperSandro2000
Copy link
Member

@rmcgibbo could you try that again please? :)

Thats on the todo and AFAIK curently not possible.

Comment on lines 41 to 46
postPatch = lib.optionalString (!stdenv.isDarwin) ''
sed -e '1s/^/LUA_SO = liblua.so/' \
-e 's/ALL_T *= */&$(LUA_SO) /' \
-i src/Makefile
cat ${./lua-dso.make} >> src/Makefile
Copy link
Member

Choose a reason for hiding this comment

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

A comment here wouldn't hurt.

We do this same patch in three different ways for four different Lua
versions, even though the structure of the Makefile barely changes
between releases.  We can easily consolidate this by just modifying
the Makefile ourselves instead of using patches (Makefiles are very
amenable to this).
On other OSes, like NetBSD, these are part of libc.  Fortunately, the
Lua Makefile already knows about this, and has a SYSLIBS variable we
can use for this.
This evaluates to an empty string when not cross compiling, so by
using this instead of stdenv.hostPlatform.config we can eliminate a
conditional.
@github-actions github-actions bot removed the 6.topic: bsd Running or building packages on BSD label Apr 19, 2021
@alyssais alyssais added the 6.topic: bsd Running or building packages on BSD label Apr 19, 2021
@Ericson2314
Copy link
Member

Tested on darwin.

@Ericson2314 Ericson2314 merged commit 96fc9cc into NixOS:staging Apr 19, 2021
@alyssais alyssais deleted the lua-bsd branch April 19, 2021 17:36
vcunat added a commit that referenced this pull request May 14, 2021
Commit 57832e6 (PR #119860) switched to using SYSCFLAGS,
but lua 5.1 calls it MYCFLAGS.  We noticed due to broken luarocks build:
https://hydra.nixos.org/build/142663274
@vcunat
Copy link
Member

vcunat commented May 15, 2021

Still a problem. All liblua.so are under-linked (on standard x86_64-linux at least), missing -lm and in 5.1 case also -ld. Adding libs to lua.pc instead seems like a poor man's workaround.

EDIT: test by ldd -r result/lib/*.so

@vcunat
Copy link
Member

vcunat commented May 15, 2021

After 539ad4f it looks good, but if you have a better idea...
pkgsCross.x86_64-netbsd.lua still seems OK to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants