Skip to content

Added missing function for CMake builds.#1250

Merged
tgamblin merged 5 commits intospack:developfrom
citibeth:efischer/160714-StdCMakeArgsFix
Oct 5, 2016
Merged

Added missing function for CMake builds.#1250
tgamblin merged 5 commits intospack:developfrom
citibeth:efischer/160714-StdCMakeArgsFix

Conversation

@citibeth
Copy link
Copy Markdown
Member

@citibeth citibeth commented Jul 14, 2016

Should fix #1242

@citibeth citibeth mentioned this pull request Jul 14, 2016
Closed
@davydden
Copy link
Copy Markdown
Member

davydden commented Jul 14, 2016

maybe it's better to address it within #1186, which modifies std_cmake_args anyway?

@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Jul 14, 2016

@alalazo will see this PR and use it as appropriate. If not, merging it
singly will fix his problem as well.

On Thu, Jul 14, 2016 at 8:46 AM, Denis Davydov [email protected]
wrote:

maybe it's better to address it within #1186
#1186, which modifies std_cmake_args
anyway?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1250 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AB1cd5tJZ6w7Qkp9OFY-duGD_5jobhnDks5qVi-6gaJpZM4JMZOj
.

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Jul 18, 2016

This duplicates code from line 347, so now we have to maintain this code twice. Can you convert line 347 to use this function, as well?

@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Jul 18, 2016

OK, did it. But I'm a little confused here (maybe I confused myself):

  • Should get_std_cmake_args() exist at all, or should its functionality be put at line 347?
  • If get_std_cmake_args() should exist, should we get rid of line 347?

The following greps might help answer this question:

$ grep std_cmake_args `findpy .`
./spack/spack/build_environment.py:    m.std_cmake_args = get_std_cmake_args(pkg)
./spack/spack/build_environment.py:def get_std_cmake_args(cmake_pkg):
./spack/spack/package.py:            spack.build_environment.get_std_cmake_args(self) + \
./spack/spack/package.py:            options = self.configure_args() + spack.build_environment.get_std_cmake_args(self)
./spack/spack/cmd/create.py:        cmake     = "cmake('.', *std_cmake_args)"

In the packages, std_cmake_args() is never used, but *std_cmake-args is used a lot.

alalazo
alalazo reviewed Jul 18, 2016
View reviewed changes
lib/spack/spack/build_environment.py Outdated
'-DCMAKE_BUILD_TYPE=RelWithDebInfo']
m.std_cmake_args = get_std_cmake_args(pkg)
if platform.mac_ver()[0]:
m.std_cmake_args.append('-DCMAKE_FIND_FRAMEWORK=LAST')
Copy link
Copy Markdown
Member

@alalazo alalazo Jul 18, 2016

Choose a reason for hiding this comment

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

@citibeth Shouldn't this and the following few lines be deleted too ?

Copy link
Copy Markdown
Member Author

@citibeth citibeth Jul 18, 2016

Choose a reason for hiding this comment

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

Yes thanks, good catch. Did it.

Now I remember the logic behind this code: I needed std_cmake_args from a
point where they were not available from the results of
set_module_variables_for_package(). So I took the code out and put it in
a separate function, called from set_module_variables_for_package() (and
elsewhere where I needed it). Merge issues at some point caused the old
code to sneak back in.

I think we now have what was intended.

On Mon, Jul 18, 2016 at 9:57 AM, Massimiliano Culpo <
[email protected]> wrote:

In lib/spack/spack/build_environment.py:

@@ -345,8 +345,7 @@ def set_module_variables_for_package(pkg, module):
m.ctest = Executable('ctest')

 # standard CMake arguments
  • m.std_cmake_args = ['-DCMAKE_INSTALL_PREFIX=%s' % pkg.prefix,
  •                    '-DCMAKE_BUILD_TYPE=RelWithDebInfo']
  • m.std_cmake_args = get_std_cmake_args(pkg)
    if platform.mac_ver()[0]:
    m.std_cmake_args.append('-DCMAKE_FIND_FRAMEWORK=LAST')

@citibeth https://github.com/citibeth Shouldn't this and the following
few lines be deleted too ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLNL/spack/pull/1250/files/94e3d271d6889880333e4ca9bb89f2e2defda489#r71151069,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1cd1IevSJNfnfg0iOVRfaIQ4VJjdJXks5qW4YzgaJpZM4JMZOj
.

Elizabeth Fischer and others added 4 commits October 5, 2016 12:52
Added missing function for CMake builds.
cd702dc
Update build_environment.py
e5e9943
std_cmake_args
9280294 
Changed std_cmake_args to use get_std_cmake_args().
Update build_environment.py
29dffde 
Delete additional lines subsumed by get_std_cmake_args()
@citibeth citibeth force-pushed the efischer/160714-StdCMakeArgsFix branch from aba28c7 to 29dffde Compare October 5, 2016 16:54
@citibeth citibeth mentioned this pull request Oct 5, 2016
Closed
@citibeth citibeth added the bug Something isn't working label Oct 5, 2016
@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Oct 5, 2016

This is a bug fix. It really should get merged.

@tgamblin tgamblin merged commit 8e75575 into spack:develop Oct 5, 2016
@citibeth citibeth deleted the efischer/160714-StdCMakeArgsFix branch October 6, 2016 14:22
olupton pushed a commit to olupton/spack that referenced this pull request Feb 7, 2022
@matz-e
touchdetector: drop MVDTool with latest version. (spack#1250)
f737655
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

spack setup: fails with AttributeError

4 participants

@citibeth @davydden @tgamblin @alalazo