BLD, ENH: Reading of extra flags from site.cfg to extend flexibility

[zerothi]

BLD: Because it allows greater flexibility of the build phase for numpy, but no changes to how numpy is build has been made

Addition to the distutils module to be able
to read in more optional arguments and flags
from the site.cfg file.

Up till this commit it has really been a pain in the neck to optimize the build for certain flags, etc.
With this any user should be capable of providing their own flags/linker options to control the build
more easily.
This commit can also be merged in the 1.9 maintenance branch.

Especially the runtime_library_dirs can come handy if users rely on LD_LIBRARY_PATH to super seed such env's.

There is still the caveat that hard-coded flags are not overwritten in any way so that providing additional flags has to be compatible with the hard-coded flags for the specific compiler.

Currently these additional options read:

• runtime_library_dirs:
  It allows users to set the runtime
  library directories so that LD_LIBRARY_PATH can be ignored.
  This has the same format as the library_dirs option.
• extra_compile_args:
  This allows the user to add specific compiler
  flags when compiling sources.
  There will be no formatting/checking of these additional compile
  flags, the compiler should complain if something is wrong.
• extra_link_args:
  This allows the user to add specific linker flags
  when linking the .so objects.
  There will be no formatting/checking of these additional compile
  flags, the linker should complain if something is wrong.

When the config is runned it automatically prints out the
read in information, thereby allowing the user to see
what has been set and what has not.

Tested with and without the added options to check that it builds correctly.

[rgommers]

This looks interesting. As an enhancement, should go into master only though, not 1.9.x

Can you please add:

• tests, in distutils/tests/test_npy_pkg_config.py
• some comments in site.cfg.example to explain how to use this functionality
• a mention in the 1.10 release notes (doc/release/1.10.0-notes.rst)

[rgommers]

You may also want to ping the mailing list about this PR; probably there are a few people interested in testing this.

[zerothi]

Thanks, will do during today. :)

2015-02-24 5:11 GMT+00:00 Ralf Gommers notifications@github.com:

You may also want to ping the mailing list about this PR; probably there
are a few people interested in testing this.

—
Reply to this email directly or view it on GitHub
#5597 (comment).

Kind regards Nick

[rgommers]

great. please ping once that's done - Github doesn't send notifications when only adding commits

[zerothi]

ping @rgommers , requests have been added

However, the test is difficult to complete due to different architectures etc. For now it simply checks that the flags are read in by the system_info object in distutils.
I have added in the message log how to improve on this test, but I have not tried linking in distutils and thus I think it would not be wise for me to do.

[rgommers]

@zerothi nice, those tests are quite a bit more extensive than what I expected! It'll take me some time to wrap my head around it, and test on a few other platforms than only 64-bit Linux (which is what TravisCI uses).

[rgommers]

formatting: this should be a reST list, so - in front of each entry and blank lines above and below the list. and names with underscores should be surrounded by double backticks, otherwise Sphinx interprets those underscores as markup

[rgommers]

For the extra compile flags, it might be useful to add a comment to point out that this does what now goes into 'ignored' for a [cflags] section in site.cfg.

[rgommers]

I still have to go through this with a debugger (the logic in distutils doesn't fit in my head) but can you confirm that runtime_library_dirs does fill the existing variables of the same name in ccompiler.py, build_ext.py, extension.py and fcompiler/__init__.py?

[rgommers]

Overall I think the logic in this patch looks good, and system_info changes (which are always fragile) are kept as minimal as possible.

[zerothi]

I think you are thinking of the pkg reading of settings. Those are two
separate entities. The pkg information and site.cfg does not interact in
any way (to my knowledge?).

2015-02-25 10:14 GMT+01:00 Ralf Gommers notifications@github.com:

For the extra compile flags, it might be useful to add a comment to point
out that this does what now goes into 'ignored' for a [cflags] section in
site.cfg.

—
Reply to this email directly or view it on GitHub
#5597 (comment).

Kind regards Nick

[zerothi]

Yes and no, it fully depends on how the extensions/compilations are
created. What I have done only adds the flags to separate sections (and the
default one).

Example (lapack_lite):

lapack_info = get_info('lapack_opt', 0)
config.add_extension('lapack_lite',
sources = [get_lapack_lite_sources],
depends = ['lapack_litemodule.c'] +
lapack_lite_src,
extra_info = lapack_info
)

in this case the compilation of the sources [get_lapack_lite_sources] will
be done with the extra stuff in the lapack section (and hence will be
retrieved in the ccompiler etc. sources).

If I wanted a generic extension to take advantage of the ALL section in
site.cfg then I needed this:

all_info = get_info('i_just_want_the_default', 0) # a non-existing class
name returns the ALL section
config.add_extension('my_package',
sources = ['my_package.c'],
extra_info = all_info
)

Reading of the flags are not intrinsic to the compiler, nor the
configuration class, they need to be passed in some way. :(

If you want to compile a file with some flags from site.cfg you need
something like the test I created:

all_info = {'extra_link_args':[],'extra_compile_args':[]} # ensures default
of nothing
all_info.update(get_info('i_just_want_the_default', 0))
c.compile(['source.c'], output_dir='/home/user',

extra_postargs=all_info['extra_link_args']+all_info['extra_compile_args'])

Hence the answer is that it depends on how the user creates the
compilation, in any case you still need to create the info-object to
retrieve the flags.
I hope this answers your question?

2015-02-25 10:16 GMT+01:00 Ralf Gommers notifications@github.com:

I still have to go through this with a debugger (the logic in distutils
doesn't fit in my head) but can you confirm that runtime_library_dirs
does fill the existing variables of the same name in ccompiler.py,
build_ext.py, extension.py and fcompiler/init.py?

—
Reply to this email directly or view it on GitHub
#5597 (comment).

Kind regards Nick

[zerothi]

@rgommers I have corrected a few things and added rpath to be equivalent to runtime_library_dirs. As I dug deeper in the pure distutils package I saw that rpath is the way to add runtime_library_dirs, yet the keywords internally are runtime_library_dirs. This simple abstraction I think is ok as it is easily circumvented.
Note, the test also tests the rpath key.

Please advice if anything needs added or tested.

[juliantaylor]

it is not clear here what the scope of these arguments are, library/runtime/include-dir are all related to binaries, compile args apply to sources. Does it apply to all numpy sources like the OPT env variable) or only part of them?
I don't see a reason this flag should be specific to sections concerning libraries, maybe having one global one is enough?

[zerothi]

My addition only applies to codes/extensions using the get_info function from numpy.distutils. So I can't say if it is used for all numpy sources like OPT. See my comment on lapack_lite which shows one place where it would be used. So, these compile flags are not used unless get_info is called on a section containing the above mentioned args.

I have added the extra_compile_args mainly for completeness sake, I see no disadvantage of having the possibility of adding extra flags when compiling extensions. You may say it is not necessary for libraries, but it allows people to construct sections which can be used to compiling extensions with different flags, say:

[O2]
extra_compile_args = -O2
[O3]
extra_compile_args = -O3

And then compiling can be done using:

config.add_extension('my_package',
                         sources = ['my_package.c'],
                         extra_info = get_info('O2')
                         )
config.add_extension('my_package3',
                         sources = ['my_package3.c'],
                         extra_info = get_info('O3')
                         )

One can of course extend several sections together.
I like to be able to fully control my compilation of source when using Python, with this PR the user can add new sections to their site.cfg and then control all sources/extensions individually, if needed.

Furthermore using config files, I think, is much more streamlining the build process than having to use env's. This is a matter of taste, but I see no disadvantage in letting users have a choice of this.

[zerothi]

But mainly I would use it for the rpath options to control the shared libraries. :)

[juliantaylor]

thanks this looks good, especially nice that you went the extra mile and added the tests for this not very well tested area. I'll try to do some testing with the branch soon.

[charris]

@juliantaylor @rgommers What is the status of this?

[zerothi]

I am still following this in case you need anything done.
It seems you are pushing for a soon 1.10 release?

2015-03-22 23:24 GMT+01:00 Charles Harris notifications@github.com:

@jaimefrio https://github.com/jaimefrio @rgommers
https://github.com/rgommers What is the status of this?

—
Reply to this email directly or view it on GitHub
#5597 (comment).

Kind regards Nick

[charris]

Starting to prepare, yes. I was waiting for @juliantaylor, but this seems to be ready.

[zerothi]

Ok, great. :)

2015-04-07 19:14 GMT+02:00 Charles Harris notifications@github.com:

Starting to prepare, yes. I was waiting for @juliantaylor
https://github.com/juliantaylor, but this seems to be ready.

—
Reply to this email directly or view it on GitHub
#5597 (comment).

Kind regards Nick

[charris]

@juliantaylor I'm about ready to put this in. Do you want to do any testing?

[charris]

OK, putting this in. @juliantaylor can more easily test it now ;) Thanks @zerothi .