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

Fixes #522, closes #542 #607

Closed
wants to merge 1 commit into from
Closed

Fixes #522, closes #542 #607

wants to merge 1 commit into from

Conversation

karolyi
Copy link

@karolyi karolyi commented Jul 24, 2020

Take the environmnent passed CXXFLAGS into account, add -fPIC to Makefile.freebsd so it won't have to be specified at each build. For the freebsd package llvm-90, -fPIC is necessary for the successful build.

Take the environmnent passed CXXFLAGS into account, add -fPIC to Makefile.freebsd so it won't have to be specified at each build. For the freebsd package llvm-90, -fPIC is necessary for the successful build.
@esc
Copy link
Member

esc commented Jul 24, 2020

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

@karolyi
Copy link
Author

karolyi commented Jul 24, 2020

Thanks. I need a timely release here, as it's blocking one of my client's projects, and I really don't want to modify my deployment scripts just to compile llvmlite from my patched version.

@karolyi
Copy link
Author

karolyi commented Jul 24, 2020

So, it doesn't seem to me that my changes broke the OSX build, it seems that master itself is broken.

@karolyi
Copy link
Author

karolyi commented Jul 24, 2020

Yep, just as I thought. Now that I fixed the usage of CXXFLAGS, there is an obscure MACOSX_DEPLOYMENT_TARGET that doesn't get passed.

Grepping the source, I have no idea where it comes from. All I see is that some scripts give it default values when it's not passed, but this is not the case now, so the build fails.

@esc
Copy link
Member

esc commented Jul 24, 2020

@karolyi I am so sorry to disappoint you, but we are not in a position to do a timely release at this stage. The next RC is planned for the 5th of August and I can not guarantee that this PR will make it as it requires manual testing on a FreeBSD system. Some expectation management: the release after that will be probably be sometime in early October. I think it will be safest for you to find an alternate route of deployment for your client. Apologies again, that I don't have better news.

@karolyi
Copy link
Author

karolyi commented Jul 24, 2020

Hm, that's bad news. I just talked with my client, hopefully we can ditch numba that depends on llvmlite, if not then I have to use my own forked repo with the fixes until this gets merged and released.

If you need testing on a FreeBSD box, let me know. I have my server always on the latest-greatest update.

@karolyi karolyi closed this Jul 24, 2020
@karolyi karolyi reopened this Jul 24, 2020
@karolyi
Copy link
Author

karolyi commented Jul 24, 2020

Dammit, wrong button.

@stuartarchibald
Copy link
Contributor

It might be possible to work around this for now by writing a small bash script and using the LLVM_CONFIG variable, for example:

$ cat myllvmconfig
#!/bin/bash
function my_llvm_config () {
    out=$(llvm-config "$@")
    if [[ $1 == "--cxxflags" ]]; then
        out="-fPIC $out"
    fi
    echo $out
}

my_llvm_config "$@"

$ export LLVM_CONFIG=$PWD/myllvmconfig

$ python setup.py build
<path>/site-packages/setuptools/dist.py:458: UserWarning: Normalizing '0.34.0dev0' to '0.34.0.dev0'
  warnings.warn(tmpl.format(**locals()))
running build
got version from VCS {'version': '0.34.0dev0', 'full': '118e993ff33ab240679595511e59b891900e972e'}
running build_ext
<path>/bin/python /tmp/llvmlite/ffi/build.py
LLVM version... 9.0.1

SVML detected
# static-libstdc++ avoids runtime dependencies on a
# particular libstdc++ version.
g++  -shared   -fPIC -I<path>/include -std=c++17 -fno-exceptions -fno-rtti -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -fno-rtti -g -DHAVE_SVML -flto assembly.cpp bitcode.cpp core.cpp initfini.cpp module.cpp value.cpp executionengine.cpp transforms.cpp passmanagers.cpp targets.cpp dylib.cpp linker.cpp object_file.cpp -o libllvmlite.so  -L<path>/lib -flto -Wl,--exclude-libs=ALL -lLLVMXRay -lLLVMWindowsManifest -lLLVMTextAPI -lLLVMTableGen -lLLVMSymbolize -lLLVMDebugInfoPDB -lLLVMOrcJIT -lLLVMJITLink -lLLVMObjectYAML -lLLVMMCA -lLLVMLTO -lLLVMPasses -lLLVMObjCARCOpts -lLLVMLineEditor -lLLVMLibDriver -lLLVMInterpreter -lLLVMIntelJITEvents -lLLVMFuzzMutate -lLLVMMCJIT -lLLVMExecutionEngine -lLLVMRuntimeDyld -lLLVMDlltoolDriver -lLLVMOption -lLLVMDebugInfoGSYM -lLLVMCoverage -lLLVMCoroutines -lLLVMX86Disassembler -lLLVMX86AsmParser -lLLVMX86CodeGen -lLLVMX86Desc -lLLVMX86Utils -lLLVMX86Info -lLLVMWebAssemblyDisassembler -lLLVMWebAssemblyCodeGen -lLLVMWebAssemblyDesc -lLLVMWebAssemblyAsmParser -lLLVMWebAssemblyInfo -lLLVMNVPTXCodeGen -lLLVMNVPTXDesc -lLLVMNVPTXInfo -lLLVMAMDGPUDisassembler -lLLVMMCDisassembler -lLLVMAMDGPUCodeGen -lLLVMMIRParser -lLLVMGlobalISel -lLLVMSelectionDAG -lLLVMipo -lLLVMInstrumentation -lLLVMVectorize -lLLVMLinker -lLLVMIRReader -lLLVMAsmParser -lLLVMAsmPrinter -lLLVMDebugInfoDWARF -lLLVMCodeGen -lLLVMTarget -lLLVMScalarOpts -lLLVMInstCombine -lLLVMAggressiveInstCombine -lLLVMTransformUtils -lLLVMBitWriter -lLLVMAnalysis -lLLVMProfileData -lLLVMObject -lLLVMBitReader -lLLVMBitstreamReader -lLLVMAMDGPUAsmParser -lLLVMMCParser -lLLVMAMDGPUDesc -lLLVMAMDGPUUtils -lLLVMMC -lLLVMDebugInfoCodeView -lLLVMDebugInfoMSF -lLLVMCore -lLLVMRemarks -lLLVMBinaryFormat -lLLVMAMDGPUInfo -lLLVMSupport -lLLVMDemangle -lz -lrt -ldl -lpthread -lm
<snip>

@karolyi
Copy link
Author

karolyi commented Jul 24, 2020

@stuartarchibald for the time being I just referred my fork from requirements.txt. It can be done your way too but I have ansible scripts that are able to redeploy the jail I use the project in, and it is much easier just to use my fork than deploy all the workaround scripts for this bug.

Luckily I am able to deploy now with the modified requirements.txt, it seems.

@esc
Copy link
Member

esc commented Jul 24, 2020

@karolyi thanks reporting back and happy to hear you managed to find a solution. We will keep this PR open and merge it when the time comes.

@esc esc added this to the Version 0.35.0 milestone Aug 18, 2020
@stuartarchibald
Copy link
Contributor

Tested this on FreeBSD, works fine.

@esc
Copy link
Member

esc commented Sep 8, 2020

@stuartarchibald excellent, thanks for testing this. While you are in the FreeBSD vicinity, might you be able to check the following also: numba/numba#6037

@esc
Copy link
Member

esc commented Sep 9, 2020

@stuartarchibald is this ready for merge?

@stuartarchibald
Copy link
Contributor

Am looking into #627 which should encompass this and also fix it for other OS.

@karolyi
Copy link
Author

karolyi commented Sep 9, 2020

Nonetheless, this fix for passing CXXFLAGS (which wasn't working before) is still useful.

@esc
Copy link
Member

esc commented Oct 8, 2020

@karolyi #627 has now been merged and will become part of the next RC soon (next week, maybe?). Can you check if this fixes #542 for you on BSD too? If so, please be so kind and close this PR, thank you! And thank you again for bringing this to our attention.

@karolyi
Copy link
Author

karolyi commented Oct 11, 2020

@esc worksforme™

what about #607 (comment) (the actual passing of CXXFLAGS which broke the OSX build) ?

@esc
Copy link
Member

esc commented Oct 11, 2020

@karolyi I am not sure yet. Will they fix something else that is broken? If yes, can you point me to a reproducer?

@karolyi
Copy link
Author

karolyi commented Oct 11, 2020

With this PR I fixed the FreeBSD build that was failing due to not passing through the CXXFLAGS=-fPIC I sent in the environment. When I fixed the passing through, it pointed out that doing so would break the OSX build.

Now that -fPIC is directly set, the CXXFLAGS are still not passed through. Works for now, but this error will show its head up somewhere else when someone will try to use CXXFLAGS and will come to the same conclusions. So this now seems to be more like a fix for one problem but not for another that became apparent.

That said, the whole thing LGTM, I just wanted to make sure you guys won't be surprised when this issue surfaces again. I realize that software in general is a mess, it's just useful to acknowledge that this is an interim solution.

@karolyi karolyi closed this Oct 11, 2020
@esc
Copy link
Member

esc commented Oct 11, 2020

@karolyi awesome, thanks for the heads up and for following up on this! We appreciate it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants