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

Always build with -fPIC on Linux and BSD #627

Merged
merged 7 commits into from
Oct 8, 2020
Merged

Always build with -fPIC on Linux and BSD #627

merged 7 commits into from
Oct 8, 2020

Conversation

gmarkall
Copy link
Member

@gmarkall gmarkall commented Aug 21, 2020 •
edited
Loading

EDIT: Title changed to reflect the fact that this PR now always builds with -fPIC on Linux and BSD, instead of requiring the user to specify it as an option. Original PR comment follows:

On systems that do not build PIC by default, linking libllvmlite.so fails (see e.g. the error reported in #553). This PR adds an option to the build_ext command in setup.py (--pic) to compile with -fPIC.

Related issues: #553, #542, #522 - this PR may address some of these issues.

In my case, I need to use this option when building on Ubuntu 18.04 with the system compiler, but from the above issues it seems that there are other systems that require the flag as well.

@gmarkall
Provide option to build with -fPIC
8957275 
On systems that do not build PIC by default, linking libllvmlite.so
fails. This commit adds an option to the `build_ext` command in setup.py
(`--pic`) to compile with `-fPIC`.

Related issues: numba#553, numba#542, numba#522 - this commit may address some of these
issues.
@gmarkall
Always build with -fPIC on Linux and BSD
96c93e4
@gmarkall
Copy link
Member Author

Following some out-of-band discussion with @stuartarchibald, it seems like it makes more sense to just always build with -fPIC on Linux and BSD, so I've adjusted the PR to do that instead.

@gmarkall gmarkall changed the title Provide option to build with -fPIC Always build with -fPIC on Linux and BSD Aug 21, 2020
@gmarkall
Copy link
Member Author

Also @stuartarchibald pointed out to me that -fPIC is used by the Anaconda compilers on OS X, so I've added that to the list of OSes for which the build will use -fPIC.

@esc esc added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 3 - Ready for Review labels Aug 24, 2020
@esc
Copy link
Member

esc commented Aug 24, 2020

@gmarkall thanks for submitting this, I have added it to the queue for review.

@stuartarchibald stuartarchibald mentioned this pull request Sep 9, 2020
Closed
@gmarkall
Fix Issue numba#632 - obj file section contents truncated
0865730 
Section contents were truncated at null bytes because `c_char_p` implies
a null-terminated string. Swapping the return type of
`LLVMPY_GetSectionContents` for a `POINTER(c_char)` no longer implies
this.

Test case added based on the reproducer in numba#632.
@gmarkall
Fix passing of CXXFLAGS and LDFLAGS on FreeBSD
6d0c591
@gmarkall
Revert "Fix Issue numba#632 - obj file section contents truncated"
5fb0a2c 
This reverts commit 0865730, which was
accidentally included on this branch.
stuartarchibald
stuartarchibald reviewed Oct 6, 2020
View reviewed changes
ffi/Makefile.freebsd Outdated
Comment on lines 4 to 5
LDFLAGS := $(LDFLAGS) $(LLVM_LDFLAGS) $(LD_FLTO_FLAGS)
LDFLAGS = $(LLVM_LDFLAGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest removing this as its overwriting the line above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove.

stuartarchibald
stuartarchibald reviewed Oct 6, 2020
View reviewed changes
ffi/Makefile.freebsd
Comment on lines +3 to +4
CXXFLAGS := $(CPPFLAGS) $(CXXFLAGS) $(LLVM_CXXFLAGS) $(CXX_FLTO_FLAGS)
LDFLAGS := $(LDFLAGS) $(LLVM_LDFLAGS) $(LD_FLTO_FLAGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth adding

llvmlite/ffi/Makefile.linux

Lines 5 to 6 in 4acef2d

CXX_FLTO_FLAGS ?= -flto
LD_FLTO_FLAGS ?= -flto -Wl,--exclude-libs=ALL
above too.

@stuartarchibald stuartarchibald mentioned this pull request Oct 6, 2020
Merged
stuartarchibald
stuartarchibald approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. Patch looks good, also tests out ok on OpenBSD.

@stuartarchibald
Copy link
Contributor

@esc any chance you could trigger a test wheel build of this please?

@stuartarchibald stuartarchibald added this to the Version 0.35.0 milestone Oct 6, 2020
@esc
Copy link
Member

esc commented Oct 8, 2020

BFID: llvmlite_wheel_13

@esc
Copy link
Member

esc commented Oct 8, 2020

So, the wheel-build was fine for all architectures, i.e. build artifacts were produced. We currently don't have any automation however to actually run tests on the wheels.

@esc
Copy link
Member

esc commented Oct 8, 2020

I spot checked OSX and Linux x86_64 and both are fine.

@stuartarchibald
Copy link
Contributor

I spot checked OSX and Linux x86_64 and both are fine.

Thanks for building and testing @esc, think this is good to go.

@stuartarchibald stuartarchibald added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 3 - Ready for Review labels Oct 8, 2020
@esc
Copy link
Member

esc commented Oct 8, 2020

Agreed!

@esc esc mentioned this pull request Oct 8, 2020
Closed
24 tasks
@esc esc merged commit 468c23d into numba:master Oct 8, 2020
@gmarkall gmarkall mentioned this pull request Nov 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartarchibald stuartarchibald stuartarchibald approved these changes

Assignees
No one assigned
Labels
5 - Ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed
Projects
None yet
Milestone
Version 0.35.0rc1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gmarkall @esc @stuartarchibald