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

ldc shouldn't install LLVMgold.so #1897

Closed
kalev opened this issue Nov 27, 2016 · 19 comments · Fixed by #1898
Closed

ldc shouldn't install LLVMgold.so #1897

kalev opened this issue Nov 27, 2016 · 19 comments · Fixed by #1898

Comments

@kalev
Copy link
Contributor

kalev commented Nov 27, 2016

I was building ldc 1.1.0 beta4 for Fedora and noticed that it installs a /usr/lib/LLVMgold.so file which it clearly shouldn't as it's part of llvm. This only seems to be happening on 64 bit architectures for some reason -- llvm's LLVMgold.so is already in /usr/lib64/ and then ldc tries to install it to /usr/lib/ (note the lib64 vs lib).

https://kojipkgs.fedoraproject.org//work/tasks/7209/16647209/build.log has the full build log (short lived link)

@kinke
Copy link
Member

kinke commented Nov 27, 2016

@JohanEngelen
Copy link
Member

Ah, I completely overlooked that LDC is not installed as a package in some arbitrary dir...
Will adjust the script to rename the lib to LLVMgold-ldc.so.
Thanks very much for noticing and reporting.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

Why is it even needed externally? And should/must it be part of the redistributable package?

@JohanEngelen
Copy link
Member

Because otherwise you cannot use LTO without installing LLVM.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

More technical please ;) - can't we link it in statically?

@JohanEngelen
Copy link
Member

It is a plugin library for the linker.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

Alright (and somehow found automatically if in the right directory). What does that mean for the package? I guess the users have to install LLVM then if they wanna play with LTO.

@JohanEngelen
Copy link
Member

I will just rename the file, easy :)
It's not found automatically: LDC directs the linker to it.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

Ah perfect. :)

@kinke
Copy link
Member

kinke commented Nov 27, 2016

I've just had a look at the relevant code. So there's 3 hardcoded default locations %%ldcbinarypath%%/../lib, /usr/local/lib/ and /usr/lib/bfd-plugins/. I'd suggest expecting it in the bin dir directly, where all other binaries reside (e.g., libconfig.so). And install the DLL into the bin dir instead of using ${CMAKE_INSTALL_PREFIX}/lib. The reasons being that the package's lib dir then only includes .a and no .so and working around the problem that a 64-bit DLL may end up in the 32-bit system lib directory. [The Windows multilib package doesn't have any lib dir at all, just lib32 and lib64.]

Edit: Maybe not install()ing LLVMgold.so makes the most sense (and instead adding %%ldcbinarypath%% and %%ldcbinarypath%%/../lib64 to the default search paths). After all it's people who build from source who 'install' LDC this way, and they need an installed LLVM anyway. It should be copied over to the bin dir for the packages though so that it's automatically available for those who don't build from source.

@JohanEngelen
Copy link
Member

Note: cmake "install()" is used to create a standalone LDC package, it doesn't necessarily mean "install on my system"; install() is shorthand for "copy a file". For example, we use make install to create the binary packages for releases. Also, you don't have to install LLVM on your system to build LDC. LLVM can sit anywhere.
The reason the script copies the plugin to two locations is to make it work for non-installed builds (make), and for "installed" builds (make install).

I don't mind about the lib or bin dir. I thought lib is where these things usually go?
Happy to modify the PR for it, but first I'll wait for CI testing to go green.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

cmake "install()" is used to create a standalone LDC package, it doesn't necessarily mean "install on my system"

The way I see it, it is primarily used to install it locally from source, but secondarily also used as base for the redistributable packages. We already copy DLLs over specifically for the packages (libcurl on Windows, libconfig, both in bin) but don't install() them.

Also, you don't have to install LLVM on your system to build LDC. LLVM can sit anywhere.

It can sit anywhere, with its libraries, incl. LLVMgold.so. We should just make sure we find the default locations (incl. lib64 and LDC's bin dir for the packages); otherwise the guy building from source with an exotic LLVM location will be able to figure out the -flto-binary switch.

@JohanEngelen
Copy link
Member

Note that the plugin is version locked to LDC... The only reason I copy it from LLVM is that I don't have to put in the build logic to build it ourselves.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

Copying instead of building manually is fine, and assuming a self-installed LDC was built using the currently installed LLVM (and so the currently installed LLVMgold.so) seems fine by me too. I'd really just extend the default search paths by these 2 entries and edit the packaging script to copy over the LLVMgold.so to the bin dir. [I can also prepare a PR for that.]

@JohanEngelen
Copy link
Member

@kinke Please let's stop debating this. Your idea will not work, and I am getting angry over having to debate this. In the only two use cases of LTO at the moment that I know of: there is no "currently installed" LLVM, the CMake script must install LLVMgold.so because there is no packing script run, etc, etc.

@kinke
Copy link
Member

kinke commented Nov 27, 2016

So for the OP, you rather prefer getting angry over thinking about how to avoid littering his 32-bit lib dir with the 64-bit LLVMgold-ldc.so. Right. So please install() it in the bin dir so that all binaries of the LDC packages end up there, and add the corresponding search path(s).

@dnadlinger
Copy link
Member

Isn't the obvious choice the lib directory (with optional suffix) that we also install druntime/Phobos to?

@kinke
Copy link
Member

kinke commented Nov 27, 2016

That suffix is unknown at compile-time, and I think we all agree that parsing the config file and checking for the default lib path at runtime or adding a hardcoded #define for it isn't worth it.

@dnadlinger
Copy link
Member

There is already a config switch for the library path; we could just set that from the config file.

This was referenced Nov 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants