Skip to content
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

[RtM] Added py-proj/package.py (RtM = Ready to Merge) #717

Merged
merged 7 commits into from
Sep 15, 2016

Conversation

citibeth
Copy link
Member

@citibeth citibeth commented Apr 1, 2016

No description provided.

extends('python')

depends_on('py-cython')
depends_on('py-setuptools')
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't depends_on the proj library?

Copy link
Member Author

Choose a reason for hiding this comment

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

GitHub formatted the PR in a funny way. But at the end of the file in the
PR (not quoted above), there's a note explaining this:

+    # The py-proj git repo actually includes the correct version of PROJ.4,
+    # which is built internally as part of the py-proj build.
+    # Adding depends_on('proj') will cause mysterious build errors.
+
+    def install(self, spec, prefix):
+        python('setup.py', 'install', '--prefix=%s' % prefix)

-- Elizabeth

On Tue, Apr 5, 2016 at 11:17 AM, Ben Boeckel notifications@github.com
wrote:

In var/spack/repos/builtin/packages/py-proj/package.py:

  • version('1.9.5.1', 'a4b80d7170fc82aee363d7f980279835')
  • depends_on('py-cython')
  • depends_on('py-setuptools')

This doesn't depends_on the proj library?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/LLNL/spack/pull/717/files/552a2815cd82b04c74eac44352f5e812ea0fa7f6#r58556930

Copy link
Contributor

Choose a reason for hiding this comment

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

Not using the proj that others are using is going to be a problem as well. There's this libproj4 with the symbol pj_free. A project using proj4's actual library also uses Python (ParaView is not farfetched for this) and that code loads the proj Python module. What proj library is used? If it is the one built by spack for ParaView, the Python might not match that one. ParaView can't use the one packaged here (it can't know about it), but if the soname doesn't match, you get duplicate symbols.

This really needs to use an external proj4.

Copy link
Member

Choose a reason for hiding this comment

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

@citibeth: I think it's bad if using py-proj and proj at the same time fails... maybe @mathstuf can look into this if he's got time. @mathstuf: have you used an external proj?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tgamblin: I agree on the code sharing. But I don't think there's much to
be done here, short of significant rewrites of py-proj. If there had
been an obvious way to depend on an external proj, I would have done it.

I'm also not comfortable putting a git version in the Spack dev branch,
because it is not checksummed. The problem is, the release version of
py-proj has a small-to-fix, but show-stopping bug (for my application).
Some PR's were submitted, but remain unmerged. Unfortunately, the branch
in my fork is still the only one that works (for me).

https://github.com/jswhit/pyproj/pull/73

I can see the following ways forward:

  1. Hold this PR until upstream is fixed. The problem with that is, someone
    else who needs py-proj might waste their effort re-writing this Spack
    package.
  2. Remove the citibeth-latlong2 version from the PR that gets merged into
    dev, and hope that other Spack users don't need it (they probably won't
    unless they use Python3).
  3. Create a tagged release of citibeth-latlong2 on my fork, and add that
    version to Spack, with hashcode.

I'm thinking (3) is probably the way forward.

Your thoughts?
-- Elizabeth

On Tue, May 10, 2016 at 4:39 AM, Todd Gamblin notifications@github.com
wrote:

In var/spack/repos/builtin/packages/py-proj/package.py:

  • version('1.9.5.1', 'a4b80d7170fc82aee363d7f980279835')
  • depends_on('py-cython')
  • depends_on('py-setuptools')

@citibeth https://github.com/citibeth: I think it's bad if using py-proj
and proj at the same time fails... maybe @mathstuf
https://github.com/mathstuf can look into this if he's got time.
@mathstuf https://github.com/mathstuf: have you used an external proj?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/LLNL/spack/pull/717/files/552a2815cd82b04c74eac44352f5e812ea0fa7f6#r62631624

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh?
On May 31, 2016 11:14 AM, "Ben Boeckel" notifications@github.com wrote:

In var/spack/repos/builtin/packages/py-proj/package.py:

  • version('1.9.5.1', 'a4b80d7170fc82aee363d7f980279835')
  • depends_on('py-cython')
  • depends_on('py-setuptools')

@mathstuf https://github.com/mathstuf: have you used an external proj?

Never used py-proj myself (I'm more familiar with proj4, but that's
really just "it's in our build stack and I'm one of the build stack guys"
kind of relationship); just pointing out that mixing multiple libraries
with the same symbols tends to not go too well…


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLNL/spack/pull/717/files/552a2815cd82b04c74eac44352f5e812ea0fa7f6#r65201976,
or mute the thread
https://github.com/notifications/unsubscribe/AB1cd6AWPD01dI1JJSI3dq3c30cvNkq4ks5qHFA-gaJpZM4H9_nz
.

Copy link
Member

Choose a reason for hiding this comment

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

So is this one still WIP?

Copy link
Member

Choose a reason for hiding this comment

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

marked it WIP -- feel free to unmark when ready...

Copy link
Member

Choose a reason for hiding this comment

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

py-setuptools is probably a build dependency? Not sure about py-cython.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. But a grep through the repo reveals some confusion over
type='build' vs. type=nolink. My guess is type=build is right, and
type=nolink needs to be converted. Your opinions?

I'll fix the packages below that are mine (glint2, ibmisc, icebin, py-proj).

cantera/package.py:    depends_on('py-cython', when='+python', type=nolink)
glint2/package.py:    depends_on('py-cython', when='+python')
ibmisc/package.py:    depends_on('py-cython', when='+python', type=nolink)
icebin/package.py:    depends_on('py-cython', when='+python')
py-h5py/package.py:    depends_on('py-cython@0.19:', type='build')
py-iminuit/package.py:    depends_on('py-cython', type='build')
py-netcdf/package.py:    depends_on('py-cython', type=nolink)
py-proj/package.py:    depends_on('py-cython')
py-pytables/package.py:    depends_on('py-cython', type=nolink)

On Fri, Sep 2, 2016 at 9:26 AM, Adam J. Stewart notifications@github.com
wrote:

In var/spack/repos/builtin/packages/py-proj/package.py:

  • version('1.9.5.1', 'a4b80d7170fc82aee363d7f980279835')
  • depends_on('py-cython')
  • depends_on('py-setuptools')

py-setuptools is probably a build dependency? Not sure about py-cython.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLNL/spack/pull/717/files/552a2815cd82b04c74eac44352f5e812ea0fa7f6#r77345291,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1cd69ohdDBe-Im4zrFwehp40mIpyubks5qmCQOgaJpZM4H9_nz
.

Elizabeth Fischer and others added 2 commits May 14, 2016 16:09
Added a blank line that Travis wanted.
@tgamblin tgamblin changed the title Added py-proj/package.py [WIP] Added py-proj/package.py Jun 2, 2016
@citibeth citibeth changed the title [WIP] Added py-proj/package.py [RtM] Added py-proj/package.py (RtM = Ready to Merge) Jun 5, 2016
@citibeth
Copy link
Member Author

@tgamblin This is ready to merge.

@citibeth
Copy link
Member Author

citibeth commented Sep 2, 2016

@tgamblin ping, can this please get merged? I believe all discussion has been resolved.

@@ -0,0 +1,27 @@
from spack import *
Copy link
Member

Choose a reason for hiding this comment

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

Missing licensing info above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a problem? I find tons of packages with missing license info.

On Fri, Sep 2, 2016 at 9:25 AM, Adam J. Stewart notifications@github.com
wrote:

In var/spack/repos/builtin/packages/py-proj/package.py:

@@ -0,0 +1,27 @@
+from spack import *

Missing licensing info above.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLNL/spack/pull/717/files/15797aace19e798d7a7cf80e7e3e345687672414#r77345182,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1cdw86zpWnQEg3R35UVDGdNMwDsChWks5qmCPfgaJpZM4H9_nz
.

2. Used setup_py
3. Added type='build' for dependencies.
@citibeth
Copy link
Member Author

citibeth commented Sep 2, 2016

Should be ready to merge now.

@tgamblin tgamblin merged commit 2a823fb into spack:develop Sep 15, 2016
@citibeth citibeth deleted the efischer/160401-PyProj branch October 6, 2016 14:15
matz-e pushed a commit to matz-e/spack that referenced this pull request Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants