Convert llvm Spackage to use the monorepo by homerdin · Pull Request #11392 · spack/spack

homerdin · 2019-05-08T15:30:41Z

This change updates the llvm Spackage to use the monorepo at https://github.com/llvm/llvm-project. This will be the primary code repository moving forward for the llvm-project. In this repo all projects are side by side in a single tarball and projects are enabled with -DLLVM_ENABLE_PROJECTS.

To enable flang in this structure, the flang fork of clang and llvm will need to replace the llvm and clang directories. This is what I am doing below, but it requires a change allowing resources in Spack to overwrite a location.

Someone more familiar with flang could likely make a cleaner mapping from the flang forks to the monorepo releases.

var/spack/repos/builtin/packages/llvm/package.py

homerdin · 2019-05-09T15:18:21Z

@junghans @gklimowicz

Does placing the appropriate versions of flang-llvm and flang-driver into the monorepo seem like the best way to support flang? Again, this would require letting a resource overwrite an existing directory.

var/spack/repos/builtin/packages/llvm/package.py

homerdin · 2019-05-20T18:22:38Z

@adamjstewart I made the Python 2.6 change. Is it reasonable to change resources to allow overwriting an existing directory? In this structure Flang wants to replace the 'llvm' and 'clang' directories with their own version (previously we only pulled their version).

adamjstewart · 2019-05-20T18:24:27Z

@homerdin That sounds like a reasonable use case/feature request. I don't think it works right now, but I can see why that would be useful. I think @scheibelp has more experience with resources than me.

homerdin · 2019-06-11T22:12:26Z

I changed the update to build the Flang fork in it's own directory. Should I include Flang package changes in this pull request or a separate one?

This also removes the need to have resources overwrite directories. This is ready to go if there are no issues.

chuckatkins · 2019-06-12T19:57:15Z

var/spack/repos/builtin/packages/llvm/package.py

+        if '+flang' not in spec:
+            # Semicolon seperated list of projects to enable
+            cmake_args.append(
+                '-DLLVM_ENABLE_PROJECTS:STRING=' + ';'.join(projects))


So, if +flang is in the spec then how are the various "projects" specified by the variants enabled or disabled?

Other than openmp and clang the other projects are not enabled for the Flang fork. The Flang fork is likely to have compatibility issues with the other tools compiler-rt, libcxx etc. This is what the current package does, it only has resources for flang-driver and openmp for the flang versions.

openmp and clang and enabled by being placed within the flang fork directory and are enabled automatically by being in the old layout. The openmp and clang resources are included when +flang, which is confusing, +flang~openmp will still build openmp.

From what I understand the flang version/variant is meant to be a dependency package for flang, and not built alone.

homerdin · 2019-07-01T15:33:13Z

Ping

naromero77 · 2019-10-31T18:00:45Z

@homerdin @chuckatkins @adamjstewart

I believe this PR would actually resolve the issue I filed a couple of weeks ago.
#13117
(I also wouldn't need to do all the refactoring that I proposed, since what Brian has done supersedes it).

adamjstewart

I don't use LLVM, so I don't feel comfortable approving and merging such a big change to the package on my own. @alalazo @svenevs might know more about LLVM compilation and usage than me.

My other question, and this may be outside the scope of this PR, is whether we want to have separate packages for subcomponents in addition to the single monolithic package we already have. In my experience, I run things on macOS, where Clang is already installed, but without OpenMP support. When trying to build py-scikit-learn, which now requires OpenMP, I went down a long adventure of trying to build llvm+openmp only to find that I couldn't install it on my laptop. I eventually added a llvm-openmp package which only contains the OpenMP bits and built in seconds as opposed to hours.

adamjstewart · 2019-11-04T16:02:00Z

var/spack/repos/builtin/packages/llvm/package.py

-    # currently required by mesa package
-    version('3.0', 'a8e5f5f1c1adebae7b4a654c376a6005',
-            url='http://llvm.org/releases/3.0/llvm-3.0.tar.gz')
+    version('develop', git='https://github.com/llvm/llvm-project')


Spack now has the ability to list both url and git at the top-level. Can you move this git next to url above? Then just add branch='master'. I would actually rename this version to master now that #1983 has been merged.

adamjstewart · 2019-11-04T16:04:39Z

var/spack/repos/builtin/packages/flang/package.py

-    depends_on('llvm@flang-ppc64le-20180612', when='@20180612 target=ppc64le')
+    depends_on('llvm@develop+flang~gold~lldb~lld~compiler-rt~libcxx~polly~internal_unwind', when='@develop')
+    depends_on('llvm@7.0.1+flang~gold~lldb~lld~compiler-rt~libcxx~polly~internal_unwind', when='@20180921')
+    depends_on('llvm@6.0.0+flang~gold~lldb~lld~compiler-rt~libcxx~polly~internal_unwind', when='@20180612')


My only problem with this syntax is that if someone already has llvm+flang+anything_else installed, Spack will be forced to reinstall llvm+flang~anything_else. The concretizer still sucks at reusing installed software, so I'm fine with keeping this as is, but in the future we may want to change this to llvm+flang.

I am not sure I understand. Do you mean?
spack spec llvm@develop+flang
vs.
spack spec llvm+flang@develop
are not the same thing to the concretizer?

I normally use the later syntax because it's less of an eyesore, but aren't these supposed to be equivalent?

What I'm saying is that if I have llvm@develop+flang+gold or llvm@develop+flang+libcxx installed, this package will force Spack to reinstall llvm@develop+flang~gold~libcxx, even though either of those installations should work fine as dependencies.

Right. I was trying to be explicit, but it does prevent using a usable installed llvm+flang. I will make the change and rebase this pull request.

adamjstewart · 2019-11-04T16:06:02Z

var/spack/repos/builtin/packages/llvm/package.py

            run_env.set('CC', join_path(self.spec.prefix.bin, 'clang'))
            run_env.set('CXX', join_path(self.spec.prefix.bin, 'clang++'))

+    # When building flang we do not use the mono repo


Does this mean that llvm+flang+anything_else won't build properly?

Saw the comment below. In that case, we may want to add a conflicts for +flang and +anything_else.

Yeah, really not fond of having it like that, also not sure why that would be the case. Is there any reason you can’t build llvm with clang and Flang simultaneously? If so, I almost want Flang to be a separate package entirely, but at a minimum have a check for nonsensical requirements.

Clang and Flang do use different versions of LLVM. Flang is a separate package, but the LLVM component of it is part of this package.

@trws I know what you are talking about. I raised this question a while back and I didn't get a whole lot of feedback.
#13117

flang will be superceded by f18 in about 6-months to a year, but the time scales for upstreaming the flang-version of LLVM into the upstream clang-version of LLVM may be longer.

@homerdin may know more.

Fair enough, we may be stuck with that then. I’d personally prefer that move into the Flang package honestly, since the resulting LLVM isn’t usable as an LLVM, but that issue predates this PR so I’m not going to hold @homerdin to implementing it.

Right, the Flang parts can be reworked or removed. For this PR I'm mostly trying to move the build to use the mono-repo which is the official way to build now and greatly simplifies the package. The functionality is largely the same, currently if you build llvm@flang20180921+polly you do not build polly.

While having a version that is only meant to be used by another package might be better than a variant that is, I don't think either is what we want.

naromero77 · 2019-11-05T18:36:15Z

I don't use LLVM, so I don't feel comfortable approving and merging such a big change to the package on my own. @alalazo @svenevs might know more about LLVM compilation and usage than me.

My other question, and this may be outside the scope of this PR, is whether we want to have separate packages for subcomponents in addition to the single monolithic package we already have. In my experience, I run things on macOS, where Clang is already installed, but without OpenMP support. When trying to build py-scikit-learn, which now requires OpenMP, I went down a long adventure of trying to build llvm+openmp only to find that I couldn't install it on my laptop. I eventually added a llvm-openmp package which only contains the OpenMP bits and built in seconds as opposed to hours.

@adamjstewart I noticed that there were separate packages for some of the subcomponents, e.g. llvm-openmp and also pgmath Is that main use case apple laptops? Are there some useful subcomponents that don't have a separate package now? I think we could fix/add those in a subsequent PR.

adamjstewart · 2019-11-05T18:39:53Z

@naromero77 I can only speak for llvm-openmp. I'm not very familiar with the LLVM environment, I only needed OpenMP support to build a single package on macOS.

homerdin · 2019-11-12T18:26:40Z

My other question, and this may be outside the scope of this PR, is whether we want to have separate packages for subcomponents in addition to the single monolithic package we already have. In my experience, I run things on macOS, where Clang is already installed, but without OpenMP support. When trying to build py-scikit-learn, which now requires OpenMP, I went down a long adventure of trying to build llvm+openmp only to find that I couldn't install it on my laptop. I eventually added a llvm-openmp package which only contains the OpenMP bits and built in seconds as opposed to hours.

I think having separate packages for the subcomponents that want to be built on their own is a good idea. The LLVM community is working on improving the build process for the components that don't need to be built with the rest of LLVM. Building stand alone subcomponents will end up needing completely different logic for their build so I think it would make sense to have a separate package for them.

homerdin · 2019-11-21T21:20:11Z

I removed the explicit variants in Flang's llvm dependency. I am seeing permission issues for the resources when building llvm+flang.

naromero77 · 2019-11-22T18:26:27Z

I removed the explicit variants in Flang's llvm dependency. I am seeing permission issues for the resources when building llvm+flang.

Not sure what you mean by "permission issues", you mean you can't download from the git repos?

homerdin · 2019-11-27T04:13:35Z

The permission issues I was seeing was because of my local config. I've updated the patches to understand the new directory layout and updated some logic for the llvm+flang variant that was previously covered by llvm@flang versions being evaluated to less than numbered versions.

naromero77 · 2019-11-29T16:55:07Z

Could we please get another reviewer @alalazo @svenevs ?

naromero77 · 2019-11-29T17:04:14Z

@homerdin Some CI tests are failing because old APIs are still being use. Since I went through this recently for my last QMCPACK PR, you can take a look to see what needs to be change:
#13832

naromero77 · 2019-12-03T18:54:57Z

@tgamblin @becker33 @alalazo @chuckatkins @svenevs

Is there someone else that can review or can it just be merged in? I also have more changes to go in but I will make a separate PR later and they are more specific to Flang.

trws

This looks like a massive improvement to me, and I’d be happy to see it go in as soon as we can get the existing requests taken care of. Move the git property up by the URL, and figure out the Flang situation. Is there a reason not to pull the resources for Flang out of the monorepo, then link or move them into place so you could do a full build with Flang and clang and so-forth?

There are other things I’d like to see done in this package, but I’ll make them a subsequent PR to add libomptarget and so-forth. Do you have cycles to do the small fixups to get this through this week @homerdin?

homerdin · 2019-12-04T20:44:38Z

@trws I can take care of the small changes. @gklimowicz Can you take a look at the Flang parts? The resources for Flang want to replace parts (llvm, clang, openmp) of the monorepo with Flang's forks. If the resources could overwrite those directories we could possibly build the flang version in the monorepo. I'm not sure if it would work and if you tried to build the other variants(polly, livcxx) with the flang fork of LLVM it also may not work.

The llvm+flang/llvm@flang builds produce a clang executable but I'm not sure if it is a fully functional clang compiler. However, the flang executable that you want to use ends up in the flang package.

naromero77 · 2019-12-04T22:03:38Z

I have a bunch of flang changes that I am working on too, but I wanted to see this PR go through first.

It sounds like people agree that flang-version of LLVM should be factored out of this package?

It would be desirable to document that we are all in agreement about what needs to get done
#13117

Also good to document any additional flang variants that are needed.

trws · 2019-12-04T22:05:59Z

I concur, when as and if f18 gets upstreamed we'll have a flang here, but it would be that flang. Having this flang here ongoing seems like it would become a problem at some point anyway.

homerdin · 2019-12-05T21:02:44Z

@trws I think I've addressed all the outstanding requests for this PR. The Flang rework and other additions can go in a subsequent PR. Let me know if there is anything you would like changed.

trws

I'm satisfied. Unless there's another issue I missed remaining here LGTM so we can get to work on some of the subsequent updates.

naromero77 · 2019-12-06T17:12:26Z

@trws don't forget to merge. Thanks in advance.

trws · 2019-12-06T17:38:59Z

I don’t have merge rights anymore, or at least not with the codecov result, guess it’s been a while. @tgamblin or @adamjstewart, could we get a merge over here?

trws · 2019-12-06T18:56:03Z

Thanks!

tgamblin · 2019-12-06T19:05:58Z

Thanks everyone!

junghans · 2019-12-06T19:41:52Z

Yeah!

chuckatkins reviewed May 8, 2019

View reviewed changes

var/spack/repos/builtin/packages/llvm/package.py Show resolved Hide resolved

adamjstewart reviewed May 19, 2019

View reviewed changes

var/spack/repos/builtin/packages/llvm/package.py Outdated Show resolved Hide resolved

adamjstewart added the clang label May 19, 2019

homerdin changed the title ~~[WIP] Convert llvm Spackage to use the monorepo~~ Convert llvm Spackage to use the monorepo Jun 11, 2019

chuckatkins reviewed Jun 12, 2019

View reviewed changes

homerdin mentioned this pull request Oct 31, 2019

Refactoring FLANG out of main LLVM package #13117

Closed

naromero77 mentioned this pull request Nov 4, 2019

Flang Oct 2019 Update #13510

Closed

adamjstewart reviewed Nov 4, 2019

View reviewed changes

homerdin added 7 commits November 15, 2019 21:57

[WIP] Convert llvm spackage to use the monorepo

f3bc052

Correcting python dependencies

d180bec

Adding develop version for llvm monorepo

d656488

Python 2.6 Fix

ceb9690

Build Flang fork in a different root_cmakelists_dir

16392e0

Formatting Fix

4eec69c

Including flang package changes

525d534

homerdin force-pushed the llvm-mono-repo branch from 9a3b2f3 to 525d534 Compare November 15, 2019 23:09

Removing explicit llvm dependencies variants

8587a19

flake8 fix

418e4e1

Updating patches and llvm+flang logic

1d09669

Updating env setup API

0ba53bd

trws requested changes Dec 4, 2019

View reviewed changes

trws self-assigned this Dec 4, 2019

homerdin added 2 commits December 4, 2019 22:21

Add top level git

8edd634

Conflicting flang variant with other variants

1f697b3

trws approved these changes Dec 5, 2019

View reviewed changes

adamjstewart merged commit 5e20bd8 into spack:develop Dec 6, 2019

trws mentioned this pull request Dec 9, 2019

Llvm libomptarget support #14060

Merged

homerdin deleted the llvm-mono-repo branch December 11, 2019 15:43

Conversation

homerdin commented May 8, 2019

Uh oh!

Uh oh!

homerdin commented May 9, 2019

Uh oh!

Uh oh!

homerdin commented May 20, 2019

Uh oh!

adamjstewart commented May 20, 2019

Uh oh!

homerdin commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homerdin commented Jul 1, 2019

Uh oh!

naromero77 commented Oct 31, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naromero77 commented Nov 5, 2019

Uh oh!

adamjstewart commented Nov 5, 2019

Uh oh!

homerdin commented Nov 12, 2019

Uh oh!

homerdin commented Nov 21, 2019

Uh oh!

naromero77 commented Nov 22, 2019

Uh oh!

homerdin commented Nov 27, 2019

Uh oh!

naromero77 commented Nov 29, 2019

Uh oh!

naromero77 commented Nov 29, 2019

Uh oh!

naromero77 commented Dec 3, 2019

Uh oh!

trws left a comment

Choose a reason for hiding this comment

Uh oh!

homerdin commented Dec 4, 2019

Uh oh!

naromero77 commented Dec 4, 2019

Uh oh!

trws commented Dec 4, 2019

Uh oh!

homerdin commented Dec 5, 2019

Uh oh!

trws left a comment

Choose a reason for hiding this comment

Uh oh!

naromero77 commented Dec 6, 2019

Uh oh!

trws commented Dec 6, 2019 •

edited

Loading