Skip to content

add OctavePackage#6746

Merged
adamjstewart merged 2 commits intospack:developfrom
davydden:feature/octave_package
Jan 3, 2018
Merged

add OctavePackage#6746
adamjstewart merged 2 commits intospack:developfrom
davydden:feature/octave_package

Conversation

@davydden
Copy link
Copy Markdown
Member

@davydden davydden commented Dec 21, 2017 •
edited
Loading

EDIT: I think I figure out the installation.

Overriding uninstall is still tricky as I can't get to octave executable the same way I did with install:

    def do_uninstall(self, force=False):
        """Uninstall this package by spec."""
        name = 'spline'
        inspect.getmodule(self).octave('pkg uninstall %s' % name)
        # delegate to instance-less method.
        Package.uninstall_by_spec(self.spec, force)

produces

==> Error: 'module' object has no attribute 'octave'

I guess I leave this for another time unless someone knows an easy fix.

it's WIP as I am bit confused why setup_dependent_package() is not called, defined in octave package https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/octave/package.py#L234-L241

Maybe I just need to follow PythonPackage which has

import inspect
def python(self, *args, **kwargs):
        inspect.getmodule(self).python(*args, **kwargs)

and if I understand correctly is being used as self.python.

@davydden davydden added feature A feature is missing in Spack WIP labels Dec 21, 2017
@davydden davydden force-pushed the feature/octave_package branch from ac47c6a to 34d978a Compare December 21, 2017 16:20
@davydden davydden removed the WIP label Dec 21, 2017
@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Dec 21, 2017

You also need to edit lib/spack/docs/packaging_guide.rst and lib/spack/spack/cmd/create.py. See #4936 for the last build system I added.

@davydden
Copy link
Copy Markdown
Member Author

davydden commented Dec 21, 2017

thanks @adamjstewart .

lib/spack/spack/cmd/create.py.

apparently, we already have OctavePackageTemplate. I will adjust it...

@davydden davydden added the ready label Dec 21, 2017
@davydden davydden force-pushed the feature/octave_package branch from 74572ae to 774e3a3 Compare December 21, 2017 20:51
@davydden
add OctavePackage
3f16fa8 
1. remove import CudaPackage which is not needed anymore
2. mention CudaPackage and OctavePackage in packaging guide
3. adjust OctavePackageTemplate
4. add clue file for Octave build
5. sanity check on self.prefix
@davydden davydden force-pushed the feature/octave_package branch from 93c99f1 to 3f16fa8 Compare December 22, 2017 10:36
adamjstewart
adamjstewart approved these changes Dec 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

This looks pretty solid to me!

Comment thread lib/spack/spack/build_systems/octave.py Outdated
# octave does not like those environment variables to be set:
os.environ.pop('CC', '')
os.environ.pop('CXX', '')
os.environ.pop('FC', '')
Copy link
Copy Markdown
Member

@adamjstewart adamjstewart Dec 22, 2017

Choose a reason for hiding this comment

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

I'm wondering if it would be better to separate this out into setup_environment.

Copy link
Copy Markdown
Member Author

@davydden davydden Dec 22, 2017

Choose a reason for hiding this comment

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

done.

Copy link
Copy Markdown
Member Author

@davydden davydden Dec 22, 2017

Choose a reason for hiding this comment

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

minor inconvenience is that we have warnings:

==> Warning: Suspicious requests to set or unset 'CC' found
==> Warning: 	    	env.set('CC', join_path(link_dir, compiler.link_paths['cc'])) at /Users/davydden/spack/lib/spack/spack/build_environment.py:146
==> Warning: 	--->	spack_env.unset('CC') at /Users/davydden/spack/lib/spack/spack/build_systems/octave.py:54
==> Warning: Suspicious requests to set or unset 'CXX' found
==> Warning: 	    	env.set('CXX', join_path(link_dir, compiler.link_paths['cxx'])) at /Users/davydden/spack/lib/spack/spack/build_environment.py:149
==> Warning: 	--->	spack_env.unset('CXX') at /Users/davydden/spack/lib/spack/spack/build_systems/octave.py:55
==> Warning: Suspicious requests to set or unset 'FC' found
==> Warning: 	    	env.set('FC', join_path(link_dir, compiler.link_paths['fc'])) at /Users/davydden/spack/lib/spack/spack/build_environment.py:155
==> Warning: 	--->	spack_env.unset('FC') at /Users/davydden/spack/lib/spack/spack/build_systems/octave.py:56

but I am ok with it.

@adamjstewart adamjstewart merged commit 4b5fe75 into spack:develop Jan 3, 2018
This was referenced Jan 3, 2018
Merged
Merged
@davydden davydden deleted the feature/octave_package branch January 3, 2018 06:59
ch741 pushed a commit to ch741/spack that referenced this pull request Apr 20, 2018
@davydden @ChrisHad
add OctavePackage (spack#6746)
ad27ae1 
* add OctavePackage

1. remove import CudaPackage which is not needed anymore
2. mention CudaPackage and OctavePackage in packaging guide
3. adjust OctavePackageTemplate
4. add clue file for Octave build
5. sanity check on self.prefix

* use setup_environment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamjstewart adamjstewart adamjstewart approved these changes

@alalazo alalazo Awaiting requested review from alalazo

Assignees

No one assigned

Labels

feature A feature is missing in Spack

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@davydden @adamjstewart