clingo: use python from spec by severinstrobl · Pull Request #20139 · spack/spack

severinstrobl · 2020-11-27T13:55:28Z

Without explicitly providing the path to the Python executable, clingo is prone to pick up the wrong Python package. As a safeguard, the required version is set explicitly based on the spec.

cosmicexplorer · 2020-11-29T13:46:04Z

This should probably go in before #20159, as it is more correct than the corresponding change there:

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

Lines 77 to 93 in c49a304

    
           if '+python' in self.spec: 
        
               python = self.spec['python'] 
        
               python_args = [ 
        
                   '-DCLINGO_REQUIRE_PYTHON=ON', 
        
                   '-DCLINGO_BUILD_WITH_PYTHON=ON', 
        
                   '-DCLINGO_BUILD_PY_SHARED=ON', 
        
                   '-DPYCLINGO_USER_INSTALL=OFF', 
        
                   '-DPYCLINGO_USE_INSTALL_PREFIX=ON', 
        
                   '-DCMAKE_CXX_FLAGS=-I{0}'.format( 
        
                       ':'.join(python.headers.directories)), 
        
                   '-DCLINGO_PYTHON_VERSION:LIST={0};EXACT'.format( 
        
                       python.version.up_to(2)), 
        
               ] 
        
           else: 
        
               python_args = [] 
        
           args = base_args + python_args

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in spack#20139

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in #20139

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in #20139 give the interpreter selection a piece of my mind revert setup-env.sh changes

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in #20139 give the interpreter selection a piece of my mind revert setup-env.sh changes remove extraneous cmake args

update clingo package.py fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in #20139 give the interpreter selection a piece of my mind revert setup-env.sh changes remove extraneous cmake args make clingo build on spack/centos6 via bootstrapping python and gcc

update clingo package.py fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in spack#20139 give the interpreter selection a piece of my mind revert setup-env.sh changes remove extraneous cmake args make clingo build on spack/centos6 via bootstrapping python and gcc

adamjstewart

Python is not in the spec unless +python, you need an if-statement to check this before adding the flags.

severinstrobl · 2020-12-29T12:02:27Z

Good point about the +python, somehow I overlooked that Python is (somewhat?) optional. Unless I'm mistaken, also the original version does not handle this correctly, as all the other CMake arguments regarding the Python bindings do not consider the variant at all. Do you want me to fix this in this merge request as well or should this be addressed separately?

adamjstewart · 2020-12-29T16:10:26Z

Please fix it in this PR.

severinstrobl · 2020-12-29T22:33:44Z

The support for the option CLINGO_PYTHON_VERSION is only available in the master branch, not any of the releases. Is it acceptable for a package to specify CMake options based on the check self.version == Version('master') or is this considered bad practice?

adamjstewart · 2020-12-30T00:09:10Z

That's fine, as long as it's reasonable to expect that it will be present in future versions. master is a moving target, so we don't want to get too specific about what flags it can and can't accept. If you know what the next feature release number is going to be, you can also say @5.5: or something like that.

Clingo can be configured without Python bindings, yet the CMake configure step has to explicitly handle this. Additionally, for CMake versions <= 3.15.0 the path to the Python binary is set explicitly based on the spec to ensure CMake picks up the correct Python installation.

adamjstewart · 2020-12-31T18:14:32Z

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

+            args += [
+                '-DCLINGO_BUILD_WITH_PYTHON=ON',
+                '-DCLINGO_REQUIRE_PYTHON=ON',
+                '-DCLINGO_BUILD_PY_SHARED=ON',


What does this flag control? Does it depend on whether '+shared' in spec['python']?

Good question, unfortunately I'm not that familiar with the internals of the library, maybe the original author of this package can shed some light. There seems to be an additional library libpyclingo that uses this flag. As far as I can tell, this library is not used by the Python module, which links against libclingo.so directly.

Alright, maybe @tgamblin knows

Explicitly specifying the path to the Python executable to use is necessary independent of the CMake version to avoid issues with multiple Python versions.

severinstrobl · 2021-03-16T11:22:30Z

With the recent changes to the package when using CMake >= 3.16.0 this is not an issue any more, so I'll close this one.

clingo: use python from spec

e6f488a

Without explicitly providing the path to the Python executable, clingo is prone to pick up the wrong Python package. As a safeguard, the required version is set explicitly based on the spec.

severinstrobl force-pushed the clingo_python_spec branch from 92dd63d to e6f488a Compare November 27, 2020 14:02

cosmicexplorer mentioned this pull request Nov 29, 2020

clean up how spack builds clingo #20159

Closed

cosmicexplorer added a commit to cosmicexplorer/spack that referenced this pull request Nov 30, 2020

merge in spack#20139

72ec589

cosmicexplorer added a commit to cosmicexplorer/spack that referenced this pull request Nov 30, 2020

update clingo package.py

09789f5

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in spack#20139

cosmicexplorer added a commit to cosmicexplorer/spack that referenced this pull request Nov 30, 2020

update clingo package.py

42fe989

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in spack#20139

cosmicexplorer added a commit to cosmicexplorer/spack that referenced this pull request Nov 30, 2020

update clingo package.py

b848c6e

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in spack#20139

cosmicexplorer added a commit that referenced this pull request Dec 2, 2020

update clingo package.py

15c2f29

fix python args in clingo package.py bootstrap clingo in setup-env.sh avoid setting concretizer: clingo in defaults fix long lines merge in #20139

adamjstewart requested changes Dec 27, 2020

View reviewed changes

adamjstewart added the update-package label Dec 27, 2020

alalazo requested a review from tgamblin December 28, 2020 12:00

severinstrobl force-pushed the clingo_python_spec branch from 9c73bb8 to c4b01c7 Compare December 30, 2020 00:12

severinstrobl requested a review from adamjstewart December 31, 2020 11:23

adamjstewart reviewed Dec 31, 2020

View reviewed changes

clingo: enforce python path for all cmake versions

b53754b

Explicitly specifying the path to the Python executable to use is necessary independent of the CMake version to avoid issues with multiple Python versions.

adamjstewart approved these changes Dec 31, 2020

View reviewed changes

severinstrobl closed this Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clingo: use python from spec#20139

clingo: use python from spec#20139
severinstrobl wants to merge 3 commits intospack:developfrom
severinstrobl:clingo_python_spec

severinstrobl commented Nov 27, 2020

Uh oh!

cosmicexplorer commented Nov 29, 2020

Uh oh!

adamjstewart left a comment

Uh oh!

severinstrobl commented Dec 29, 2020

Uh oh!

adamjstewart commented Dec 29, 2020

Uh oh!

severinstrobl commented Dec 29, 2020

Uh oh!

adamjstewart commented Dec 30, 2020

Uh oh!

adamjstewart Dec 31, 2020

Uh oh!

severinstrobl Dec 31, 2020

Uh oh!

adamjstewart Dec 31, 2020

Uh oh!

severinstrobl commented Mar 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

severinstrobl commented Nov 27, 2020

Uh oh!

cosmicexplorer commented Nov 29, 2020

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

severinstrobl commented Dec 29, 2020

Uh oh!

adamjstewart commented Dec 29, 2020

Uh oh!

severinstrobl commented Dec 29, 2020

Uh oh!

adamjstewart commented Dec 30, 2020

Uh oh!

adamjstewart Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

severinstrobl Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

adamjstewart Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

severinstrobl commented Mar 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants