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

ocgis #159

Merged
merged 1 commit into from
Apr 21, 2016
Merged

ocgis #159

merged 1 commit into from
Apr 21, 2016

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Mar 19, 2016

Ping @bekozi.

PS: I believe we have to tweak the gdal recipe.

Edit: This PR needs #158

@ocefpaf ocefpaf self-assigned this Mar 20, 2016
@pelson
Copy link
Member

pelson commented Mar 22, 2016

Seems like there are a few test issues with this one.

@pelson pelson changed the title ocgis WIP: ocgis Mar 22, 2016
@ocefpaf
Copy link
Member Author

ocefpaf commented Mar 22, 2016

I need to get back to this one at a later time. I will close it for now to reduce the noise here.

@ocefpaf ocefpaf closed this Mar 22, 2016
@pelson
Copy link
Member

pelson commented Mar 22, 2016

I need to get back to this one at a later time. I will close it for now to reduce the noise here.

It's fine. I suspect this repo will never be free of noise - that is the nature of lots of timezones and day jobs. 😄

@ocefpaf ocefpaf reopened this Apr 1, 2016
@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ocgis) and found it was in an excellent condition.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/ocgis) and found some lint.

Here's what I've got...

For recipes/ocgis:

  • Selectors are suggested to take a <space><space>#<space>[<selector>] form.

@pelson
Copy link
Member

pelson commented Apr 2, 2016

Selectors are suggested to take a #[] form.

really is ugly trying to render the fact that it should be a leading double space " # [selector]"

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ocgis) and found it was in an excellent condition.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 2, 2016

❤️ the new label!

@pelson
Copy link
Member

pelson commented Apr 2, 2016

❤️ the new label!

Hehe, that was me trying to get the linting service to recognise your bad selector 😜

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 6, 2016

Ping @bekozi . (Re-building from that branch.)

@bekozi
Copy link

bekozi commented Apr 6, 2016

@ocefpaf, the failures on osx and windows are both related to gdal linking. For osx, the gdal shared library cannot be found by fiona:

===== testing package: ocgis-1.3.0-py27_0 =====
import: 'ocgis'
Traceback (most recent call last):
  File "/Users/travis/miniconda/conda-bld/test-tmp_dir/run_test.py", line 25, in <module>
    import ocgis
  File "/Users/travis/miniconda/envs/_test/lib/python2.7/site-packages/ocgis/__init__.py", line 1, in <module>
    from ocgis.util.environment import env
  File "/Users/travis/miniconda/envs/_test/lib/python2.7/site-packages/ocgis/util/environment.py", line 10, in <module>
    from ocgis.util.helpers import get_iter
  File "/Users/travis/miniconda/envs/_test/lib/python2.7/site-packages/ocgis/util/helpers.py", line 10, in <module>
    import fiona
  File "/Users/travis/miniconda/envs/_test/lib/python2.7/site-packages/fiona/__init__.py", line 72, in <module>
    from fiona.collection import Collection, BytesCollection, vsi_path
  File "/Users/travis/miniconda/envs/_test/lib/python2.7/site-packages/fiona/collection.py", line 7, in <module>
    from fiona.ogrext import Iterator, ItemsIterator, KeysIterator
ImportError: dlopen(/Users/travis/miniconda/envs/_test/lib/python2.7/site-packages/fiona/ogrext.so, 2): Library not loaded: /usr/local/lib/libgeotiff.2.dylib
  Referenced from: /Users/travis/miniconda/envs/_test/lib/libgdal.1.dylib
  Reason: image not found
TESTS FAILED: ocgis-1.3.0-py27_0

For windows, the issue is GDAL_DATA again. However, this looks like something I may be available to fix. Do you think the environment variable is appropriately set on the windows builds? I remember you saying that is was most likely fixed.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 6, 2016

@ocefpaf, the failures on osx and windows are both related to gdal linking. For osx, the gdal shared library cannot be found by fiona:

I just rebased this PR because we are removing homebrew from Travis-CI now (and I forgot to do that here). That should fix gdal/fiona issues.

Do you think the environment variable is appropriately set on the windows builds? I remember you saying that is was most likely fixed.

I need to check that again. I will get back to you.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 6, 2016

The failures have nothing to do with this PR. I am preparing a new gdal that should fix this.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 8, 2016

Ping @bekozi. Things are green on OS X and Linux now. The Windows failure is the GDAL folder issue.

Here is how we call the activate script and here is the script. Let me know if you can see any obvious errors and/or if that could be fixed in ocgis.

@bekozi
Copy link

bekozi commented Apr 8, 2016

Thanks @ocefpaf. I'll take a look.

if 'LIBRARY_PREFIX' in os.environ: # Windows.
gdalData = os.path.join(os.environ['LIBRARY_PREFIX'], 'share', 'gdal')
else: # Linux/OS X.
gdalData = os.path.join(os.environ['PREFIX'], 'share', 'gdal')
Copy link
Member Author

Choose a reason for hiding this comment

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

@bekozi this is the trick we do in gdal. In theory that var will be available in normal envs, not sure why it does not work on the _test env. Can ocgis use that var?

Copy link

Choose a reason for hiding this comment

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

@ocefpaf, this should be usable. I can add it to the bag of tricks used to find the GDAL_DATA directory. Thanks for the snippet. I'll do some testing on my end.

Copy link
Member

Choose a reason for hiding this comment

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

We are leaking GDAL implementation detail into this recipe, which isn't ideal. Maybe we can come up with a better solution in GDAL?

I've raised an issue in conda-forge/gdal-feedstock#35.

Don't let it hold this PR up though - we can always go back and remove it!

Copy link

Choose a reason for hiding this comment

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

@ocefpaf, this should be good to go with the v1.3.1 tag!

@pelson, I'm not enamored with the solution either. I'd love to find an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ocefpaf, this should be good to go with the v1.3.1 tag!

Rebased, updated, and waiting for the green.

Thanks @bekozi!

@bekozi
Copy link

bekozi commented Apr 12, 2016

@ocefpaf, we can retry these builds. Hopefully for the last time. 😄 I made a change to the path search for windows and was able to build successfully on linux and windows locally. You can revert run_test.py to:

from ocgis.test import run_simple

run_simple()

The GDAL_DATA path moved with the new build, so I added another search option. The reason it wasn't working with your test file changes was that the environment variable was set below the ocgis import. However, we should be able to avoid this now!

@bekozi
Copy link

bekozi commented Apr 12, 2016

We'll need to wait for the bug fix version before merging of course...

source:
git_url: https://github.com/NCPP/ocgis.git
#git_tag: v{{ version }}
git_tag: i415-conda-forge
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 re-started the CIs. If that works do you plan on a new release or do you want to make the conda package using this branch for a while? (That is fine if you have reasons to do so.)

Copy link

Choose a reason for hiding this comment

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

There are a couple other bugs so I don't mind tagging one. Will take a day or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

No rush. We can wait for good software.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 12, 2016

And we have green! 🎉

@jakirkham
Copy link
Member

So, I guess we are waiting for a new release. Correct?

@bekozi
Copy link

bekozi commented Apr 12, 2016

Excellent! @jakirkham, yes, I'll tag one in the near-term and report back here when it's ready.

@@ -0,0 +1,2 @@
"%PYTHON%" setup.py install --single-version-externally-managed --record record.txt
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this file anymore 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to remove it!

@ocefpaf ocefpaf force-pushed the ocgis branch 2 times, most recently from ee55c09 to a7043a9 Compare April 18, 2016 23:00
@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 18, 2016

ocgis is fine. The failures are due to a sequence of updates that were not "digested well" by gdal. I am working to get gdal (fiona and rasterio) working again so we can merge this.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/ocgis) and found it was in an excellent condition.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 21, 2016

@bekozi the fiona issue was solved and I re-started the CIs, but now we have some unitests that are failing: https://circleci.com/gh/conda-forge/staged-recipes/2322

@bekozi
Copy link

bekozi commented Apr 21, 2016

@ocefpaf it looks like the version in the recipe needs to be incremented to 1.3.1. Those failures should be fixed.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 21, 2016

@ocefpaf it looks like the version in the recipe needs to be incremented to 1.3.1. Those failures should be fixed.

Done! (I will be out of office for a few days. If this is green bug @pelson to merge it)

@jakirkham
Copy link
Member

Have a nice holiday, @ocefpaf.

@pelson pelson merged commit a30262f into conda-forge:master Apr 21, 2016
@bekozi
Copy link

bekozi commented Apr 21, 2016

Thanks, all! 😄

@jakirkham jakirkham changed the title WIP: ocgis ocgis Apr 21, 2016
@jakirkham
Copy link
Member

Figured we should drop the WIP. 😉

@ocefpaf ocefpaf deleted the ocgis branch April 24, 2016 21:37
@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 24, 2016

🎉

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

Successfully merging this pull request may close these issues.

6 participants