powellb / seapy

State Estimation and Analysis in Python
MIT License
28 stars 21 forks source link

Packaging #51

Closed ocefpaf closed 7 years ago

ocefpaf commented 7 years ago

This PR seems very intrusive but is is not. I will try to explain it in detail in the diff.

I left the individual commits to make it easier to review.

ocefpaf commented 7 years ago

If the this package is used in development mode by just directory adding the code to the path I can:

that way both package and current status quo can live together. However, there are other ways to use the python package in dev mode too. Although a cycle of release+install is always recommend for its use in production.

powellb commented 7 years ago

Compilation no longer works. Final link fails with bad object codes. The python packaging isn't honoring the fortran environment.

ocefpaf commented 7 years ago

Compilation no longer works. Final link fails with bad object codes. The python packaging isn't honoring the fortran environment.

Not sure what you mean. I am using it without any both locally and package in conda-forge. (I was even able to install it with pip!)

Anyways, looking at the code there would be more issues to package it properly, like removing the bundled sources and listing all the various licenses used from the different files in the source tree. So I am closing this. Thanks!

powellb commented 7 years ago

I'm not sure we need to close it. Licensing wise, everything can be under MIT, but I have to go through the oalib a bit more carefully to ensure that.

I'm not sure why it won't compile correctly. Possibly something with anaconda... The option -fPIC is required, and for some reason it isn't being compiled with that. The original Makefile specified it to make sure.

powellb commented 7 years ago

So, I was able to get it to work with:

% FFLAGS="-fPIC" python setup.py build --fcompiler=gfortran

The -fPIC is a requirement for all of the big clusters that we deploy on. It doesn't make a difference on my laptop (but we do all of our work on big machines where we can't install into the shared python installations).

powellb commented 7 years ago

I modified setup.py to have:

config.add_extension('oalib', sources='src/oalib.F', extra_f77_compile_args=["-fPIC"]) config.add_extension('hindices', sources='src/hindices.F', extra_f77_compile_args=["-fPIC"])

This is working for all of my test cases now, and I can do a:

python setup.py develop

to keep local changes, etc.

So, I ask that you resubmit the (corrected) pull request, and I will approve. After approval, I am going to go through and update everything to be designated as MIT license.

ocefpaf commented 7 years ago

I never noticed that b/c we define the -fPIC globally in our builds but, as you already found out, it is quite easy to add extra flags.

If you are interested in this PR take a look at the points raised in https://github.com/powellb/seapy/pull/51#issuecomment-313974239 and let me know what you think about it.

powellb commented 7 years ago

I think that things work as-is with the PR.

* restore the makefiles

I don't think this is needed.

* restore the original path of the Fortran extensions

I don't think this is needed.

* add a cd seapy as the first dev-mode installation step

What would this do? It doesn't seem to be necessary.

With the current PR (after adding the extra compile flags), I am able to build and install or build and 'develop' the package without issue. With the 'develop' installation, it provides the ability that I needed for testing and modification.

ocefpaf commented 7 years ago

What would this do? It doesn't seem to be necessary.

Without the first two it does not. The goal was to preserver the original installation instructions while still providing a python package.

With the current PR (after adding the extra compile flags), I am able to build and install or build and 'develop' the package without issue. With the 'develop' installation, it provides the ability that I needed for testing and modification.

OK. I will update the PR with the flag.