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

lua: Build a shared library #6008

Merged
merged 1 commit into from
Nov 1, 2017
Merged

lua: Build a shared library #6008

merged 1 commit into from
Nov 1, 2017

Conversation

michaelkuhn
Copy link
Member

Patch is taken from Arch Linux.

@michaelkuhn michaelkuhn mentioned this pull request Oct 27, 2017
Merged
davydden
davydden previously approved these changes Oct 27, 2017
View reviewed changes
var/spack/repos/builtin/packages/lua/package.py Outdated
def install(self, spec, prefix):
if spec.satisfies("platform=darwin"):
target = 'macosx'
else:
target = 'linux'
make('INSTALL_TOP=%s' % prefix,
'MYCFLAGS=-fPIC',
Copy link
Member

Choose a reason for hiding this comment

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

here and below please use self.compiler.pic_flag

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. :-)

davydden
davydden reviewed Oct 27, 2017
View reviewed changes
var/spack/repos/builtin/packages/lua/package.py Outdated
'MYLDFLAGS=-L%s -L%s' % (
spec['readline'].prefix.lib,
spec['ncurses'].prefix.lib),
'MYLIBS=-lncursesw',
'CC=%s -std=gnu99' % spack_cc,
'TO_LIB=liblua.a liblua.so liblua.so.%s liblua.so.%s' % (
Copy link
Member

Choose a reason for hiding this comment

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

sorry, another issue: this will certainly fail on macOS. Can you please get the list dynamically?

Copy link
Member

@davydden davydden Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ah, I see that your patch won't work on macOS either, maybe don't apply it on darwin and skip this line as well?

Copy link
Member

Choose a reason for hiding this comment

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

on macOS that would be

TO_LIB=liblua.a liblua.dylib

@davydden davydden dismissed their stale review October 27, 2017 18:52

wont work on macOS

@davydden davydden added the WIP label Oct 27, 2017
davydden
davydden reviewed Oct 27, 2017
View reviewed changes
var/spack/repos/builtin/packages/lua/package.py Outdated
@@ -58,24 +58,33 @@ class Lua(Package):
destination="luarocks",
placement='luarocks')

patch('https://git.archlinux.org/svntogit/packages.git/plain/trunk/liblua.so.patch?h=packages/lua&id=70abed4cfa74993348bf7afbbc70151a3d57cc01',
Copy link
Member

@davydden davydden Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

maybe add this patch to Spack (as a file inside lua folder) and also add a variation from Homebrew: https://github.com/Homebrew/homebrew-core/blob/master/Formula/lua.rb (at the bottom)

@davydden
Copy link
Member

if you put some if's in place, i can check this PR locally on macOS and clean that part

@michaelkuhn
Copy link
Member Author

I tried to come up with something a bit more generic. The Linux path still works but you will have to test the macOS path. :-)

@michaelkuhn michaelkuhn force-pushed the lua-shared branch 2 times, most recently from ef71c46 to 3ac5b9c Compare October 27, 2017 21:54
davydden
davydden approved these changes Oct 27, 2017
View reviewed changes
Copy link
Member

@davydden davydden left a comment

Choose a reason for hiding this comment

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

seems to be working 👍

$ ls /Users/davydden/spack/opt/spack/darwin-highsierra-x86_64/clang-9.0.0-apple/lua-5.3.4-orbdggfe4o2c2etu3ztpzv7xntqyg7nv/lib/
liblua.a		liblua.dylib		liblua.dylib.5.3	liblua.dylib.5.3.4	lua

@davydden davydden added ready and removed WIP labels Oct 27, 2017
junghans
junghans requested changes Oct 28, 2017
View reviewed changes
var/spack/repos/builtin/packages/lua/package.py Outdated
@@ -58,24 +58,34 @@ class Lua(Package):
destination="luarocks",
placement='luarocks')

patch('liblua-shared.patch')
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment with a link to Arch linux!

Copy link
Member

Choose a reason for hiding this comment

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

but to be fair, this patch is NOT the same as in arch linux, neither is it the same as in Homebrew

Copy link
Member

Choose a reason for hiding this comment

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

@junghans see the last comment #6008 (comment)

@junghans
Copy link
Contributor

Honestly, I would prefer converting the liblua.a to liblua.so outside of their buildsystem.
Calling gcc -shared -Wl,-soname=liblua.1 -Wl,-z,defs -Wl,--whole-archive liblua.a -Wl,--no-whole-archive -lm -o liblua.so.1 in install() doesn't seem too hard.
(Or OSX it would be gcc -dynamiclib -install_name <prefix>/liblua.1 -Wl,-all_load -Wl,liblua.a -lm -o liblua.so.1)

@tgamblin: having a generic static_to_shared_lib function would be a good idea anyhow...

@michaelkuhn
lua: Build a shared library
1a27df9 
Patch is inspired by Arch Linux and Homebrew.
@michaelkuhn
Copy link
Member Author

I have added a comment with links to the Arch Linux and Homebrew patches. If the conversion function is preferred, though, I could also give that a try in the next few days.

@junghans junghans merged commit 0d624ea into spack:develop Nov 1, 2017
@michaelkuhn michaelkuhn mentioned this pull request Nov 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davydden davydden davydden approved these changes

@junghans junghans junghans requested changes

Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michaelkuhn @davydden @junghans