Updating package for SOLLVE by vlkale · Pull Request #12607 · spack/spack

vlkale · 2019-08-28T00:09:08Z

I'm creating a pull request for a Spack package for the SOLLVE project, which will be built based on the Spack package for the LLVM project. The Spack package for the SOLLVE project would be used by application programmers of OpenMP applications, especially ECP OpenMP applications, to experiment with the latest features in SOLLVE project's software technologies.

adamjstewart

I assume this package is largely the same as the LLVM package in Spack? You can actually subclass that package if you want so there isn't so much code duplication.

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

vlkale · 2019-09-16T04:33:23Z

I assume this package is largely the same as the LLVM package in Spack? You can actually subclass that package if you want so there isn't so much code duplication.

SOLLVE goes in depth on certain features for exascale. LLVM development is rapid. SOLLVE shouldn’t and doesn’t need to keep up with all the changes in LLVM, so we think it’s best that SOLLVE shouldn't be a subclass of LLVM.

shintaro-iwasaki · 2019-09-17T19:42:23Z

Sorry for the long delay to clean up this PR. @adamjstewart Could you please review it again?

I believe we fixed the issues you pointed out. About the subclass, in addition to @vlkale's comments, I think now the SOLLVE's code is not simply a duplication of LLVM. (Actually I tried to use LLVM as a subclass of SOLLVE, but it was quite hard to use their functions and class member variables). I believe making this package independent of the LLVM package is cleaner and simpler (and less troublesome to maintain since the LLVM package should be updated much more frequently).

vlkale · 2019-11-25T06:21:58Z

Hi @adamjstewart,

Can you look at this pull request again when you have a chance?

@shintaro-iwasaki and I have worked on this package, and we think that it's in a reasonable state to be merged.

Vivek

adamjstewart

Sorry it took me so long to review! Just a few recommended changes to make.

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

Co-authored-by: Vivek Kale <[email protected]>

Copied from llvm/package.py.

shintaro-iwasaki · 2019-11-25T20:48:06Z

Thanks for reviewing this! I rebased the whole branch to the latest develop since the master was outdated.

The second commit is to reflect your suggestions.
The third commit updates the architecture detection code.

adamjstewart

You have a lot of variants that add flags to disable things. It would be better to always be explicit and say:

if '+foo' in spec:
    args.append('-DENABLE_FOO=ON')
else:
    args.append('-DENABLE_FOO=OFF')

CMake likes to switch the default based on what it can find on your OS, leading to non-deterministic builds if you aren't explicit like this.

adamjstewart · 2019-11-25T22:41:05Z

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

+    """
+
+    homepage = 'https://www.bnl.gov/compsci/projects/SOLLVE/'
+    url      = "https://github.com/SOLLVE"


You can get rid of this url since it isn't used. You can actually move the git up here like so:

Suggested change

url = "https://github.com/SOLLVE"

git = "https://github.com/SOLLVE/llvm.git"

Then it won't need to be specified in every version() directive below.

Thanks! You're right. I updated urlt and git in version() as you suggested.

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

vlkale · 2019-11-26T00:38:50Z

Hi @shintaro-iwasaki,

Thanks for revising the file for SOLLVE’s Spack package given Adam’s suggestions from today.

Hi @adamjstewart,

I agree with all of Shintaro’s changes.

Your suggestion to always be explicit for disabling makes sense and I agree with it. Thanks for pointing out Cmake’s behavior. (@shintaro-iwasaki: can you quickly search for cases where we are disabling things in our python file for Spack package of SOLLVE, and fix them according to Adam’s suggestion and corresponding example? Let me know if you need my help with this and I’ll get on it as soon as possible).

Vivek

- url -> git - remove git in version() - explicit cmake options in else clauses - add newlines for better readability

shintaro-iwasaki · 2019-11-26T10:04:35Z

Thanks for reviewing the PR!
The fourth commit I pushed should address issues recommended by @adamjstewart

adamjstewart

Looks good to me. If there are any additional changes required they can happen in another PR.

vlkale · 2019-12-19T00:49:08Z

Referencing #14188 for Pull Request of Release of SOLLVE’s Spack package.

ax3l added the new-package label Aug 28, 2019

adamjstewart reviewed Aug 31, 2019

View reviewed changes

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

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

vlkale force-pushed the develop branch from aa91e69 to 87b80d4 Compare September 16, 2019 00:43

shintaro-iwasaki force-pushed the develop branch 2 times, most recently from 5def1dd to ab1f4ba Compare September 16, 2019 04:32

shintaro-iwasaki force-pushed the develop branch from ab1f4ba to 9434fa1 Compare September 16, 2019 04:36

adamjstewart requested changes Nov 25, 2019

View reviewed changes

shintaro-iwasaki and others added 3 commits November 25, 2019 19:22

Add SOLLVE package with Shintaro's help on rebasing.

42c7848

Co-authored-by: Vivek Kale <[email protected]>

sollve: reflect suggestions by @adamjstewart

1b04525

sollve: update target detection

3806612

Copied from llvm/package.py.

shintaro-iwasaki force-pushed the develop branch from 9434fa1 to 3eb1a02 Compare November 25, 2019 20:39

adamjstewart reviewed Nov 25, 2019

View reviewed changes

sollve: fix a few things

011b6e6

- url -> git - remove git in version() - explicit cmake options in else clauses - add newlines for better readability

adamjstewart approved these changes Nov 26, 2019

View reviewed changes

adamjstewart merged commit 07cda58 into spack:develop Nov 26, 2019

shintaro-iwasaki mentioned this pull request Jan 11, 2021

llvm-doe: add new package #20719

Merged

	url = "https://github.com/SOLLVE"
	git = "https://github.com/SOLLVE/llvm.git"

Conversation

vlkale commented Aug 28, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vlkale commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shintaro-iwasaki commented Sep 17, 2019

Uh oh!

vlkale commented Nov 25, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shintaro-iwasaki commented Nov 25, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

adamjstewart Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

shintaro-iwasaki Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vlkale commented Nov 26, 2019

Uh oh!

shintaro-iwasaki commented Nov 26, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

vlkale commented Dec 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vlkale commented Sep 16, 2019 •

edited

Loading