Skip to content

Re-worked setup.py to avoid the need for separate/non-standard build commands #2891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -88,31 +88,14 @@ install:
ln -s $(pwd)/iris-test-data-${IRIS_TEST_DATA_SUFFIX} iris-test-data;
fi

# prepare iris build directory
- if [[ $TEST_TARGET -ne 'coding' ]]; then
IRIS=$(ls -d1 build/lib*/iris);
mkdir $IRIS/etc;
else
IRIS=lib/iris;
fi

# set config paths
- SITE_CFG=$IRIS/etc/site.cfg
- SITE_CFG=lib/iris/etc/site.cfg
- echo "[Resources]" > $SITE_CFG
- echo "test_data_dir = $(pwd)/iris-test-data/test_data" >> $SITE_CFG
- echo "doc_dir = $(pwd)/docs/iris" >> $SITE_CFG
- echo "[System]" >> $SITE_CFG
- echo "udunits2_path = $PREFIX/lib/libudunits2.so" >> $SITE_CFG

# The coding standards tests expect all the standard names and PyKE
# modules to be present.
- if [[ $TEST_TARGET == 'coding' ]]; then
python setup.py std_names;
PYTHONPATH=lib python setup.py pyke_rules;
fi

# iris
- python setup.py --quiet build
- python setup.py --quiet install
Copy link
Member

Choose a reason for hiding this comment

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

The test_pep8_conformance is failing as (I think) the rule are not being compiled and installed.

On install it is compiling the rules but (I think) they are relative to the root i.e. in the same directory as the setup.py, and thus not being installed, hence why on installation there are no compiled rules for the kb.

The the problem in within the setup.py refactor of the compile_pyke_rules


script:
Expand Down Expand Up @@ -160,5 +143,7 @@ script:
fi
- if [[ $TEST_TARGET == 'coding' ]]; then
cd $INSTALL_DIR;
python setup.py pyke_rules;
python setup.py std_names;
Copy link
Member

@bjlittle bjlittle Nov 1, 2017

Choose a reason for hiding this comment

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

At this point, (I think) it's too late to compile the rules, as it'll put the compiled rules in the lib (if anywhere) and won't be installed (which has already happened), and so won't be used

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, but this is in the coding block, which actually checks the source, not the installed version. Caught me out too!

Copy link
Member

Choose a reason for hiding this comment

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

But it's the travis coding pep8 tests that are failing ... hinting (to me) that the compiled files aren't where they should be ... ? They might be in the lib source, but aren't the tests running against the installed version of iris?

Copy link
Member Author

Choose a reason for hiding this comment

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

They weren't for standard_names (at least, that is what I observed in an earlier commit!)

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm something isn't quite as it seems. Not quite sure what's going on ...

python setup.py test --coding-tests;
fi
12 changes: 5 additions & 7 deletions MANIFEST.in
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,18 @@
include CHANGES COPYING COPYING.LESSER INSTALL

# Files from setup.py package_data that are not automatically added to source distributions
recursive-include lib/iris/tests/results *.cml *.cdl *.txt *.xml
recursive-include lib/iris/etc *.txt
recursive-include lib/iris/tests/results *.cml *.cdl *.txt *.xml *.json
recursive-include lib/iris/etc *
include lib/iris/fileformats/_pyke_rules/*.k?b
include lib/iris/tests/stock*.npz

# File required to build docs
recursive-include docs Makefile *.js *.png *.py *.rst
recursive-exclude docs/iris/build *
prune docs/iris/build

# Files required to build std_names module
include tools/generate_std_names.py
include etc/cf-standard-name-table.xml

# Extension source
recursive-include src *.c *.cc *.cpp *.h


global-exclude *.pyc
global-exclude __pycache__
223 changes: 118 additions & 105 deletions setup.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,16 @@
from __future__ import print_function

import contextlib
from distutils.command import build_ext, build_py
from distutils.core import setup, Command
from distutils.sysconfig import get_config_var
from contextlib import contextmanager
from distutils.core import Command
from distutils.util import convert_path
import fnmatch
import multiprocessing
import os
from shutil import copyfile
import sys
import textwrap

import numpy as np
import setuptools

# Add full path so Python doesn't load any __init__.py in the intervening
# directories, thereby saving setup.py from additional dependencies.
sys.path.append('lib/iris/tests/runner')
from _runner import TestRunner
from setuptools import setup
from setuptools.command.develop import develop as develop_cmd
from setuptools.command.build_py import build_py


# Returns the package and all its sub-packages
Expand All @@ -38,7 +32,7 @@ def find_package_tree(root_path, root_package):
if dir_names:
prefix = dir_path.split(os.path.sep)[root_count:]
packages.extend(['.'.join([root_package] + prefix + [dir_name])
for dir_name in dir_names])
for dir_name in dir_names])
return packages


Expand All @@ -55,24 +49,34 @@ def file_walk_relative(top, remove=''):
yield os.path.join(root, file).replace(remove, '')


def std_name_cmd(target_dir):
script_path = os.path.join('tools', 'generate_std_names.py')
xml_path = os.path.join('etc', 'cf-standard-name-table.xml')
module_path = os.path.join(target_dir, 'iris', 'std_names.py')
cmd = (sys.executable, script_path, xml_path, module_path)
return cmd
@contextmanager
def temporary_path(directory):
"""
Context manager that adds and subsequently removes the given directory
to sys.path

"""
sys.path.insert(0, directory)
try:
yield
finally:
del sys.path[0]


# Add full path so Python doesn't load any __init__.py in the intervening
# directories, thereby saving setup.py from additional dependencies.
with temporary_path('lib/iris/tests/runner'):
from _runner import TestRunner # noqa:


class SetupTestRunner(TestRunner, setuptools.Command):
class SetupTestRunner(TestRunner, Command):
Copy link
Member Author

Choose a reason for hiding this comment

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

Was almost tempted to pull the import into this class, and make the runner import entirely lazy...

pass


class CleanSource(Command):
"""
Removes orphaned pyc/pyo files from the sources.
class BaseCommand(Command):
"""A valid no-op command for setuptools & distutils."""

"""
description = 'clean orphaned pyc/pyo files from sources'
description = 'A no-op command.'
user_options = []

def initialize_options(self):
Expand All @@ -81,6 +85,13 @@ def initialize_options(self):
def finalize_options(self):
pass

def run(self):
pass
Copy link
Member

@bjlittle bjlittle Nov 1, 2017

Choose a reason for hiding this comment

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

I guess using an ABC abstractmethod would be too much?

It's the only method that's specialized in the sub-classes, otherwise this class is nop

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely too much. In almost all situations 😉
I'm not going to do this one unless you push me to. Is that OK?

Copy link
Member

Choose a reason for hiding this comment

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

Just a passing comment, I really didn't expect you to make that change 👍



class CleanSource(BaseCommand):
description = 'clean orphaned pyc/pyo files from the source directory'

def run(self):
for root_path, dir_names, file_names in os.walk('lib'):
for file_name in file_names:
Expand All @@ -92,92 +103,85 @@ def run(self):
os.remove(compiled_path)


class MakeStdNames(Command):
"""
Generates the CF standard name module containing mappings from
CF standard name to associated metadata.

"""
description = "generate CF standard name module"
user_options = []
def compile_pyke_rules(cmd, directory):
# Call out to the python executable to pre-compile the Pyke rules.
# Significant effort was put in to trying to get these to compile
# within this build process but there was no obvious way of finding
# a workaround to the issue presented in
# https://github.com/SciTools/iris/issues/2481.

def initialize_options(self):
pass
shelled_code = textwrap.dedent("""\

def finalize_options(self):
pass
import os

def run(self):
cmd = std_name_cmd('lib')
self.spawn(cmd)
# Monkey patch the load method to avoid "ModuleNotFoundError: No module
# named 'iris.fileformats._pyke_rules.compiled_krb'". In this instance
# we simply don't want the knowledge engine, so we turn the load method
# into a no-op.
from pyke.target_pkg import target_pkg
target_pkg.load = lambda *args, **kwargs: None

# Compile the rules by hand, without importing iris. That way we can
# avoid the need for all of iris' dependencies being installed.
os.chdir(os.path.join('{bld_dir}', 'iris', 'fileformats', '_pyke_rules'))

# Import pyke *after* changing directory. Without this we get the compiled
# rules in the wrong place. Identified in
# https://github.com/SciTools/iris/pull/2891#issuecomment-341404187
from pyke import knowledge_engine
knowledge_engine.engine('')

class MakePykeRules(Command):
"""
Compile the PyKE CF-NetCDF loader rule base.
""".format(bld_dir=directory)).split('\n')
shelled_code = '; '.join(
[line for line in shelled_code
if not line.strip().startswith('#') and line.strip()])
args = [sys.executable, '-c', shelled_code]
cmd.spawn(args)

"""
description = "compile CF-NetCDF loader rule base"
user_options = []

def initialize_options(self):
pass
def copy_copyright(cmd, directory):
# Copy the COPYRIGHT information into the package root
iris_build_dir = os.path.join(directory, 'iris')
for fname in ['COPYING', 'COPYING.LESSER']:
copyfile(fname, os.path.join(iris_build_dir, fname))

def finalize_options(self):
pass

@staticmethod
def _pyke_rule_compile():
"""Compile the PyKE rule base."""
from pyke import knowledge_engine
import iris.fileformats._pyke_rules
knowledge_engine.engine(iris.fileformats._pyke_rules)
def build_std_names(cmd, directory):
# Call out to tools/generate_std_names.py to build std_names module.

def run(self):
# Compile the PyKE rules.
MakePykeRules._pyke_rule_compile()
script_path = os.path.join('tools', 'generate_std_names.py')
xml_path = os.path.join('etc', 'cf-standard-name-table.xml')
module_path = os.path.join(directory, 'iris', 'std_names.py')
args = (sys.executable, script_path, xml_path, module_path)

cmd.spawn(args)

class MissingHeaderError(Exception):
"""
Raised when one or more files do not have the required copyright
and licence header.

def custom_cmd(command_to_override, functions, help_doc=""):
Copy link
Member

@bjlittle bjlittle Nov 1, 2017

Choose a reason for hiding this comment

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

command_to_override -> cls ? ... that's kinda what it is, or at least what we expect

Copy link
Member Author

Choose a reason for hiding this comment

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

Compromise? command_cls_to_override?

Copy link
Member

Choose a reason for hiding this comment

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

@pelson This change got lost in the noise ... although, I'm not going to lose any sleep if you don't do it 😜

"""
pass

Allows command specialisation to include calls to the given functions.

class BuildPyWithExtras(build_py.build_py):
"""
Adds the creation of the CF standard names module and compilation
of the PyKE rules to the standard "build_py" command.
class ExtendedCommand(command_to_override):
description = help_doc or command_to_override.description

"""
@contextlib.contextmanager
def temporary_path(self):
"""
Context manager that adds and subsequently removes the build
directory to the beginning of the module search path.

"""
sys.path.insert(0, self.build_lib)
try:
yield
finally:
del sys.path[0]
def run(self):
# Run the original command first to make sure all the target
# directories are in place.
command_to_override.run(self)

def run(self):
# Run the main build command first to make sure all the target
# directories are in place.
build_py.build_py.run(self)
# build_lib is defined if we are building the package. Otherwise
# we want to to the work in-place.
dest = getattr(self, 'build_lib', None)
if dest is None:
print(' [Running in-place]')
# Pick the source dir instead (currently in the sub-dir "lib")
dest = 'lib'

# Now build the std_names module.
cmd = std_name_cmd(self.build_lib)
self.spawn(cmd)
for func in functions:
func(self, dest)

# Compile the PyKE rules.
with self.temporary_path():
MakePykeRules._pyke_rule_compile()
return ExtendedCommand


def extract_version():
Expand All @@ -193,24 +197,33 @@ def extract_version():
return version


custom_commands = {
'test': SetupTestRunner,
'develop': custom_cmd(
develop_cmd, [build_std_names, compile_pyke_rules]),
'build_py': custom_cmd(
build_py,
[build_std_names, compile_pyke_rules, copy_copyright]),
'std_names':
custom_cmd(BaseCommand, [build_std_names],
help_doc="generate CF standard name module"),
'pyke_rules':
custom_cmd(BaseCommand, [compile_pyke_rules],
help_doc="compile CF-NetCDF loader rules"),
'clean_source': CleanSource,
}


setup(
name='Iris',
version=extract_version(),
url='http://scitools.org.uk/iris/',
author='UK Met Office',

author_email='scitools-iris-dev@googlegroups.com',
packages=find_package_tree('lib/iris', 'iris'),
package_dir={'': 'lib'},
package_data={
'iris': list(file_walk_relative('lib/iris/etc', remove='lib/iris/')) + \
list(file_walk_relative('lib/iris/tests/results',
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that: before, we just put everything in lib/iris/tests/results. Now we are being explicit with a glob in MANIFEST. Not necessarily better or wrong - just different.

remove='lib/iris/')) + \
['fileformats/_pyke_rules/*.k?b'] + \
['tests/stock*.npz']
},
data_files=[('iris', ['CHANGES', 'COPYING', 'COPYING.LESSER'])],
include_package_data=True,
tests_require=['nose'],
cmdclass={'test': SetupTestRunner, 'build_py': BuildPyWithExtras,
'std_names': MakeStdNames, 'pyke_rules': MakePykeRules,
'clean_source': CleanSource},
)
cmdclass=custom_commands,
zip_safe=False,
)