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

Add recipe for the LLVM core libraries #3375

Merged
merged 37 commits into from
May 24, 2021

Conversation

newlawrence
Copy link
Contributor

@newlawrence newlawrence commented Nov 1, 2020

Specify library name and version: llvm-core/11.0.1

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented Nov 1, 2020

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@Croydon
Copy link
Contributor

Croydon commented Nov 1, 2020

We also have #1450 (just as a reference)

@conan-center-bot

This comment has been minimized.

@newlawrence
Copy link
Contributor Author

The package maps each static library as a conan component (i.e. llvm-core::demangle, llvm-core::support, llvm-core::interpreter...), so they can be linked in the same way as by using the llvm-config tool; all of the dependencies information is automatically extracted from the cmake configuration files. This way, other packages for the rest of the llvm suite elements can be created relying on these ones.

As for shared builds, it uses the single library distribution (check out for LLVM_BUILD_LLVM_DYLIB in llvm documentation), so conan components are not used but the regular usage instead.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Comment on lines 183 to 188
self.run('cmake --graphviz=graph/llvm.dot .')
with tools.chdir('graph'):
dot_text = tools.load('llvm.dot').replace(' ', '')

dep_regex = re.compile(r'//(.+)->(.+)$', re.MULTILINE)
deps = re.findall(dep_regex, dot_text)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this @intelligide? Instead of parsing LLVMExports.cmake, I use cmake to output the dependency graph as a graphviz's dot file which is way easier to parse.

It still uses a regular expression, but it is a simple one. The alternative would be using external tools like graphviz plus networkx that will do the parsing for you, but then you'll introduce a dependency on those tools that may not be installed along conan. I think this solution is easy and maintainable enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better (not perfect yet but better than nothing).

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

recipes/llvm-core/all/conanfile.py Outdated Show resolved Hide resolved
Co-authored-by: Yoann Potinet <intelligide@hotmail.fr>
As it is not part of the install target.
This is a kind of problematic library, see:
https://reviews.llvm.org/D74588
@newlawrence
Copy link
Contributor Author

newlawrence commented Apr 13, 2021

Failure in build 42 (194cf21f70c5a74064d65d6b07ec8f49adb17293):

  • llvm-core/11.1.0@:
    CI failed to create some packages (All logs)

Definitely, the issue with stalled builds does only occur when building shared libraries on Linux. Here, in this commit, I screwed up those builds and it can be seen how the rest of the configurations pass without problems.

Why does this happen? Well, the shared library is a bundle of 70+ (depending on the configured targets) libraries, so high memory consumption during the linking step is expected (see).

@jgsogo, @danimtb do you think there's a way to pass LLVM_PARALLEL_LINK_JOBS=1and CMAKE_EXE_LINKER_FLAGS="-Wl,--reduce-memory-overheads -Wl,--hash-size=1021" to cmake for the building step from outside? I wouldn't like to hardcode those in the conanfile as they're only valid when linking using ld (and also not to impose those on the users building the package on their own).

An alternative I can think of is just change the defaults for the targets and compile just the X86 backends (most of the people wouldn't use most of the other ones) hoping that this will be enough to fix the issue.

@newlawrence
Copy link
Contributor Author

newlawrence commented Apr 13, 2021

Just checked locally. I forgot to mention that for the shared build I had no other choice but to enable the building of the tools which add extra burden to the linker and increments the build time dramatically. In my local tests, with 2GB of ram assigned, even with the options mentioned, the build swapped a lot and lasted several hours upon completion.

I think I've exhausted my options so far, the shared builds need more memory to get built successfully.

@stale
Copy link

stale bot commented May 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 14, 2021
@blockspacer
Copy link

blockspacer commented May 14, 2021

In my local tests, with 2GB of ram assigned, even with the options mentioned, the build swapped a lot and lasted several hours upon completion.

Sounds like LLVM recipe needs better hardware for CI.

Is it just free tier problem?

LLVM just too heavy and requires paid CI plan.

LLVM support may be beneficial for conan. A lot of people may want to use conan with sanitized libc++, libtooling, clang++, clang-tidy, scan-build, ccc-analyzer, c++-analyzer, clang-format. All that requires LLVM recipe...

@newlawrence
Note that current recipe lacks sanitizer options, see "LLVM_USE_SANITIZER" and "COMPILER_RT_BUILD_SANITIZERS" as example from https://github.com/blockspacer/llvm_tools/blob/master/conanfile.py

Note that some options may speedup builds and allow to use less RAM:

        cmake.definitions["LLVM_INCLUDE_TESTS"]="OFF"
        cmake.definitions["LLVM_BUILD_TESTS"]="OFF"
        cmake.definitions["LLVM_BUILD_EXAMPLES"]="OFF"
        cmake.definitions["LLVM_INCLUDE_EXAMPLES"]="OFF"
        cmake.definitions["LLVM_BUILD_BENCHMARKS"]="OFF"
        cmake.definitions["LLVM_INCLUDE_BENCHMARKS"]="OFF"
        cmake.definitions["LLVM_ENABLE_DOXYGEN"]="OFF"
        cmake.definitions["LLVM_ENABLE_OCAMLDOC"]="OFF"
        cmake.definitions["LLVM_ENABLE_SPHINX"]="OFF"
        # when making a debug or asserts build speed it up by building a release tablegen
        cmake.definitions["LLVM_OPTIMIZED_TABLEGEN"]="ON"

@Croydon
Is conan (jfrog?) interested in LLVM support or that pull request will be marked stale forever?

@stale stale bot removed the stale label May 14, 2021
@danimtb
Copy link
Member

danimtb commented May 17, 2021

@newlawrence thanks for trying it locally. Build time is not a concern for us in C3I and since we are not able to provide more resources at the moment, I would like to try to make the recipe build at least. We can constrain the configurations (drop linux shared builds) or even hardcode build flags in the recipe ATM. Do you think it is possible to make it build here? I think it is better to have this PR merged and then work on improvements over this situation. Sorry for the bad news and thanks a lot for moving this forward!

@newlawrence
Copy link
Contributor Author

newlawrence commented May 21, 2021

@danimtb we could disable all shared builds for the moment and see if we finally get green, given that in Windows they are not even available, I don't think it is such a loss.

@blockspacer I think I will give a try to enable builds with the sanitizers enabled. Thank you for your comment!

@conan-center-bot

This comment has been minimized.

recipes/llvm-core/all/conandata.yml Outdated Show resolved Hide resolved
Co-authored-by: Yoann Potinet <intelligide@hotmail.fr>
if self.settings.os == 'Windows' and self.options.shared:
message = 'Shared build not supported on Windows'
if self.options.shared: # Shared builds disabled just due to the CI
message = 'Shared builds not currently supported'
raise ConanInvalidConfiguration(message)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't remove the code to build the package as a shared library but to raise an invalid configuration exception in all cases instead. In case someone wants to tweak the package to recover the lost functionality or, for us in the future, it is as easy as to remove this two lines and uncomment the following ones.

@newlawrence
Copy link
Contributor Author

All green in build 45 (e5b2dee54c8da8cf3c2ab4bb43ec789fbba6135f):

  • llvm-core/11.1.0@:
    All packages built successfully! (All logs)

Ok, so I think we're good to go @danimtb 😁. I'm waiting for your comments @Croydon, @intelligide, @uilianries.

@conan-center-bot
Copy link
Collaborator

All green in build 46 (f6a023ba475bfb5819f70ad70268cfa4a4b02cc7):

  • llvm-core/11.1.0@:
    All packages built successfully! (All logs)

def source(self):
tools.get(**self.conan_data['sources'][self.version])
os.rename('llvm-{}.src'.format(self.version), self._source_subfolder)
self._patch_sources()
Copy link
Contributor

Choose a reason for hiding this comment

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

Patches should be applied in build method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even in this case where we are setting no_copy_source to True?

The patches applied here work for all the different build configurations unconditionally, therefore they can be applied only once. If we were to apply the patches in the build method we'd have no other choice but to copy the sources, which I wouldn't recommend given their size.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is always better to copy the sources. However, given the size of the package and the artifacts, and the limitation of the build machines, an exception is necessary.

@conan-center-bot conan-center-bot merged commit bde927b into conan-io:master May 24, 2021
@blockspacer
Copy link

blockspacer commented May 25, 2021

@danimtb @Croydon, @intelligide, @uilianries, @newlawrence

Hi.

Current recipe is awesome, thank you very much.

Note that LLVM recipe may need some changes for wider adoptation:

  • LLVM docs say about multi-stage builds i.e. first stage builds statically linked clang to re-build all LLVM code. That step can be disabled with env. var, see proof-of-concept below.
  • To use sanitizers: First, need to build cxx and cxxabi with LLVM_USE_SANITIZER in separate folder. Second, build whole LLVM code WITHOUT LLVM_USE_SANITIZER, but with COMPILER_RT_BUILD_SANITIZERS. During package step copy cxx and cxxabi with LLVM_USE_SANITIZER enabled (make sure other build stages does not override cxx and cxxabi files!) Note that COMPILER_RT_BUILD_SANITIZERS required to create libclang_rt.asan*.so libs.
  • Some people want to use LLVM only to compile other code, so they may want del self.info.settings.compiler based on some condition (env. var?).
  • Some people want to use LLVM libs. For example, LibTooling can be used to parse source code. So they may want to keepself.info.settings.compiler based on some condition (env. var?). They also may want to configure self.cpp_info.libs, see llvm_libs in proof-of-concept conanfile.py below.

Note that it may be good idea to separate recipe into multiple smaller recipes:

  • Recipe that allows to link with LLVM libs (see llvm_libs and self.cpp_info.libs) may be separated (Is it possible?)
  • Recipe that allows to use clang as compiler, sets proper paths to LLVM tools (clang-format, etc) and propagates compiler flags. Please take look at separate recipe https://github.com/blockspacer/llvm_9_installer (Is it good idea?)

Please take look at updated proof-of-concept:
https://github.com/blockspacer/conan_llvm_9

madebr pushed a commit to madebr/conan-center-index that referenced this pull request Jun 21, 2021
* Added recipe for the LLVM core libraries

* Reuse cmake configuration

* Fix shared builds for 10x and 11x

* Parse dependencies from graphviz output

* Beware of dummy targets

* Keep only latest version for the moment

* Beware of Windows line endings

* Relax cmake version requirement

* Add list of unsupported compiler configurations

* Commit suggestion

Co-authored-by: Yoann Potinet <intelligide@hotmail.fr>

* Cannot discriminate Visual Studio by its minor version

Visual Studio version 16.4, which is known by miscompiling
 LLVM, is currently being used by conan-center-index's CCI.

Let's use LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN to make that
 compilation pass until Visual Studio is upgraded.

* Disable object targets properly

* Use external dependencies properly

* Enable building with libffi in Windows

* Do not force libc++ in clang builds

* Wrong value for a cmake flag

* Building native tools were making some configurations fail

* Allow Visual Studio generators now that native tools are disabled

* Disable relative paths for the sources in the debug info

The latest versions of clang were not able to find the standard
 library headers if compiling with `-stdlib=libc++` and
 `-no-canonical-prefixes`.

* Update supported compilers

* String comparisons are dangerous

* Remove iconv patch comment

Turns out that the hack was justified. Both, iconv and charset
 libraries, own a localcharset symbol. In normal circumstances
 this is not an issue but libLLVM gets linked with
 `--whole-archive` or `-all_load` and the linker complains
 about duplicated symbols.

It is safer to remove charset from the linking step, as its
 symbols are not needed, than to remove the linker option.

* Commit suggestion

Co-authored-by: Yoann Potinet <intelligide@hotmail.fr>

* More meaningful exception messages

* Remove unnecessary readme

* Support older Python versions

It is such a pity; pathlib is way elegant than os.path.

* Empty commit to trigger the CI

* Remove unnecessary status message

Co-authored-by: Michael Keck <git@cr0ydon.com>

* Less strict cmake minimum required version

Co-authored-by: Michael Keck <git@cr0ydon.com>

* LLVM version bumped to 11.0.1

* LLVM version bumped to 11.1.0

* Deactivate fPIC if shared

* Fix fPIC flag for shared builds

* Install LLVMTableGenGlobalISel manually

As it is not part of the install target.
This is a kind of problematic library, see:
https://reviews.llvm.org/D74588

* Add option use_sanitizer

* Disable all shared builds

* Dedent conandata.yml

Co-authored-by: Yoann Potinet <intelligide@hotmail.fr>

Co-authored-by: Yoann Potinet <intelligide@hotmail.fr>
Co-authored-by: Michael Keck <git@cr0ydon.com>
@ericLemanissier
Copy link
Contributor

@newlawrence and all, there is a glitch in the test recipe: the variable components in https://github.com/conan-io/conan-center-index/blob/master/recipes/llvm-core/all/test_package/conanfile.py#L27 is never defined. On the other hand, the variable targets is used but never defined. What was intended ?

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.