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

Add z5py recipe #6427

Merged
merged 32 commits into from Aug 16, 2018

Conversation

Projects
None yet
5 participants
@constantinpape
Contributor

constantinpape commented Aug 8, 2018

No description provided.

@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 8, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

conda-forge-linter commented Aug 8, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.
@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 8, 2018

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/z5py) and found it was in an excellent condition.

conda-forge-linter commented Aug 8, 2018

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/z5py) and found it was in an excellent condition.

@jakirkham

Thanks for adding this @constantinpape. Looks pretty good.

There have been some changes between conda-build 2 and 3. So have left some comments below along those lines. Also there are some things that happen behind the scenes (e.g. pinnings, build matrices, etc.). Have noted some changes below to take advantage of these.

Sorry there are a few comments. No need to get all of this right in one go. Happy to iterate a bit and answer questions/discuss as needed. Would suggest looking at the host/build bit first and trying that. Everything else should fall in place around that change pretty much.

Please feel free to ask questions. Thanks again for working on this. :)

Show outdated Hide outdated recipes/z5py/meta.yaml
Show outdated Hide outdated recipes/z5py/meta.yaml
Show outdated Hide outdated recipes/z5py/meta.yaml
Show outdated Hide outdated recipes/z5py/meta.yaml
Show outdated Hide outdated recipes/z5py/meta.yaml
Show outdated Hide outdated recipes/z5py/meta.yaml
Show outdated Hide outdated recipes/z5py/build.sh
Show outdated Hide outdated recipes/z5py/build.sh
Show outdated Hide outdated recipes/z5py/meta.yaml
@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 8, 2018

Member

Also noticed there was a concern about getting access to C++14 compilers correct? As Linux doesn't have compilers that support this currently, we can add the content below to a file named conda_build_config.yaml in your z5py recipe directory. This will pick up new compilers from defaults (which we are working on migrating to) for Linux.

c_compiler:    # [linux]
  - gcc        # [linux]
cxx_compiler:  # [linux]
  - gxx        # [linux]

Note: This requires the build/host and compiler changes mentioned above to work properly.

Member

jakirkham commented Aug 8, 2018

Also noticed there was a concern about getting access to C++14 compilers correct? As Linux doesn't have compilers that support this currently, we can add the content below to a file named conda_build_config.yaml in your z5py recipe directory. This will pick up new compilers from defaults (which we are working on migrating to) for Linux.

c_compiler:    # [linux]
  - gcc        # [linux]
cxx_compiler:  # [linux]
  - gxx        # [linux]

Note: This requires the build/host and compiler changes mentioned above to work properly.

@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 9, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • Jinja2 variable definitions are suggested to take a {%<one space>set<one space><variable name><one space>=<one space><expression><one space>%} form. See lines [1]

conda-forge-linter commented Aug 9, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • Jinja2 variable definitions are suggested to take a {%<one space>set<one space><variable name><one space>=<one space><expression><one space>%} form. See lines [1]
@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 9, 2018

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/z5py) and found it was in an excellent condition.

conda-forge-linter commented Aug 9, 2018

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/z5py) and found it was in an excellent condition.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 9, 2018

Contributor

@loriab @jakirkham
Thanks for your comments, I have made some changes that should address them.

@jakirkham
Two of your comments were a bit unclear to me or needed further explanation,
I left a reply there.

Contributor

constantinpape commented Aug 9, 2018

@loriab @jakirkham
Thanks for your comments, I have made some changes that should address them.

@jakirkham
Two of your comments were a bit unclear to me or needed further explanation,
I left a reply there.

@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 9, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

conda-forge-linter commented Aug 9, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 9, 2018

Contributor

I am a bit confused about the checksum.
I naively assumed that it would be the commit id, but this seems not to
be the case.
What is the preferred way of doing this?

Contributor

constantinpape commented Aug 9, 2018

I am a bit confused about the checksum.
I naively assumed that it would be the commit id, but this seems not to
be the case.
What is the preferred way of doing this?

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 9, 2018

Contributor

I ran into another issue:
I am using https://github.com/nlohmann/json to read json from c++.
I have included this as git submodule. Unfortunately, using the download from source,
I can't use git to check it out.
I see two options:

Unfortunately, it is only available on the QuantStack channel (afaik), so I don't know how to include this into the conda-forge build.

Contributor

constantinpape commented Aug 9, 2018

I ran into another issue:
I am using https://github.com/nlohmann/json to read json from c++.
I have included this as git submodule. Unfortunately, using the download from source,
I can't use git to check it out.
I see two options:

Unfortunately, it is only available on the QuantStack channel (afaik), so I don't know how to include this into the conda-forge build.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 9, 2018

Member

Thanks for all the updates @constantinpape. Will give this another look.

Tried to clarify comments above. Feel free to ask anything else I've missed inline.

The checksum can be gotten by running openssl sha256 <tarball> locally on the tarball used.

That said, unfortunately I missed that z5py requires a submodule. GitHub's autogenerated tarballs are a bit broken as they do not include the submodule as well. Sorry about that. Emailed GitHub support about this (it's a common issue we run into). We could solve this a couple of ways.

  1. Manually generate the tarball with the submodule and add it to the GitHub release
  2. Package nlohmann_json in conda-forge (cc @SylvainCorlay, possibly of common interest)
  3. Switch back to cloning with git

Any of these should be fine. We may want to do 2 anyways as it sounds like this is something people are interested in having packaged anyways.

Member

jakirkham commented Aug 9, 2018

Thanks for all the updates @constantinpape. Will give this another look.

Tried to clarify comments above. Feel free to ask anything else I've missed inline.

The checksum can be gotten by running openssl sha256 <tarball> locally on the tarball used.

That said, unfortunately I missed that z5py requires a submodule. GitHub's autogenerated tarballs are a bit broken as they do not include the submodule as well. Sorry about that. Emailed GitHub support about this (it's a common issue we run into). We could solve this a couple of ways.

  1. Manually generate the tarball with the submodule and add it to the GitHub release
  2. Package nlohmann_json in conda-forge (cc @SylvainCorlay, possibly of common interest)
  3. Switch back to cloning with git

Any of these should be fine. We may want to do 2 anyways as it sounds like this is something people are interested in having packaged anyways.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 9, 2018

Member

Generally this is looking pretty good. Couple minor comments above. We can worry about them once we've got the build more or less working.

Member

jakirkham commented Aug 9, 2018

Generally this is looking pretty good. Couple minor comments above. We can worry about them once we've got the build more or less working.

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 9, 2018

Member

@jakirkham unfortunately if I recall correctly, nlohmann_json's cmake checks for the version of the compiler at play and won't install if it is too old, so we won't be able to have it on conda-forge before modern compilers are enabled.

Member

SylvainCorlay commented Aug 9, 2018

@jakirkham unfortunately if I recall correctly, nlohmann_json's cmake checks for the version of the compiler at play and won't install if it is too old, so we won't be able to have it on conda-forge before modern compilers are enabled.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 9, 2018

Member

Thanks for the info.

Based on a quick skim, sounds like they want C++11, which largely matches up with our compilers. Though there are some bugs in gcc 4.8, which causes them to require gcc 4.9. Their requirements for Clang and VS seem to match what we have currently.

Given this, what if we do the same thing for nlohmann_json that we are doing here? Namely opt-in to the new compilers for Linux only. Thoughts?

Member

jakirkham commented Aug 9, 2018

Thanks for the info.

Based on a quick skim, sounds like they want C++11, which largely matches up with our compilers. Though there are some bugs in gcc 4.8, which causes them to require gcc 4.9. Their requirements for Clang and VS seem to match what we have currently.

Given this, what if we do the same thing for nlohmann_json that we are doing here? Namely opt-in to the new compilers for Linux only. Thoughts?

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 9, 2018

Contributor

@jakirkham @SylvainCorlay
I think providing nlohmann_json on conda-forge would be valuable, so I think we
should provide a recipe where we opt-in to the new Linux compilers like here.

For this package, we should go for option 2 then.

@jakirkham
Thanks for the comments, all clear.
I will push an update soon.

Contributor

constantinpape commented Aug 9, 2018

@jakirkham @SylvainCorlay
I think providing nlohmann_json on conda-forge would be valuable, so I think we
should provide a recipe where we opt-in to the new Linux compilers like here.

For this package, we should go for option 2 then.

@jakirkham
Thanks for the comments, all clear.
I will push an update soon.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 9, 2018

Contributor

Ok, pushed some updates.
I will add the checksum once we have a nlohmann_json package and other build issues are fixed.

@SylvainCorlay
If you don't mind, I would go ahead and create a conda-forge feedstock for nlohmann_json based on
https://github.com/QuantForge/nlohmann_json-recipe

Contributor

constantinpape commented Aug 9, 2018

Ok, pushed some updates.
I will add the checksum once we have a nlohmann_json package and other build issues are fixed.

@SylvainCorlay
If you don't mind, I would go ahead and create a conda-forge feedstock for nlohmann_json based on
https://github.com/QuantForge/nlohmann_json-recipe

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 10, 2018

Member

Given this, what if we do the same thing for nlohmann_json that we are doing here? Namely opt-in to the new compilers for Linux only. Thoughts?

Wait, we can do that?!!
This would be awesome. I have some other header-only packages on QuantStack that perform this kind of check upon installation (namely cxxopts). That would help us start the migration from QuantStack to conda-forge and reduce the number of QuantStack packages.

Member

SylvainCorlay commented Aug 10, 2018

Given this, what if we do the same thing for nlohmann_json that we are doing here? Namely opt-in to the new compilers for Linux only. Thoughts?

Wait, we can do that?!!
This would be awesome. I have some other header-only packages on QuantStack that perform this kind of check upon installation (namely cxxopts). That would help us start the migration from QuantStack to conda-forge and reduce the number of QuantStack packages.

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 10, 2018

Member

If you don't mind, I would go ahead and create a conda-forge feedstock for nlohmann_json based on
https://github.com/QuantForge/nlohmann_json-recipe

Please go ahead. Don't hesitate to add me, @JohanMabille and @wolfv as recipe maintainers.

Member

SylvainCorlay commented Aug 10, 2018

If you don't mind, I would go ahead and create a conda-forge feedstock for nlohmann_json based on
https://github.com/QuantForge/nlohmann_json-recipe

Please go ahead. Don't hesitate to add me, @JohanMabille and @wolfv as recipe maintainers.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 10, 2018

Contributor

I created a PR for the nlohmann_json recipe #6447.
We can continue this PR once #6447 is merged.

Contributor

constantinpape commented Aug 10, 2018

I created a PR for the nlohmann_json recipe #6447.
We can continue this PR once #6447 is merged.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 10, 2018

Member

Given this, what if we do the same thing for nlohmann_json that we are doing here? Namely opt-in to the new compilers for Linux only. Thoughts?

Wait, we can do that?!!

Sure. They are header only after all. 😉 Worst case the user has compilers that won't work with the header.

This would be awesome. I have some other header-only packages on QuantStack that perform this kind of check upon installation (namely cxxopts). That would help us start the migration from QuantStack to conda-forge and reduce the number of QuantStack packages.

Glad to hear this. Yes please feel free to contribute these. We'd be happy to have them. 😄

Member

jakirkham commented Aug 10, 2018

Given this, what if we do the same thing for nlohmann_json that we are doing here? Namely opt-in to the new compilers for Linux only. Thoughts?

Wait, we can do that?!!

Sure. They are header only after all. 😉 Worst case the user has compilers that won't work with the header.

This would be awesome. I have some other header-only packages on QuantStack that perform this kind of check upon installation (namely cxxopts). That would help us start the migration from QuantStack to conda-forge and reduce the number of QuantStack packages.

Glad to hear this. Yes please feel free to contribute these. We'd be happy to have them. 😄

@jakirkham jakirkham referenced this pull request Aug 10, 2018

Merged

Nlohmann json #6447

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 12, 2018

Member

xtensor-python should have >=0.20 instead of >0.20.

Member

SylvainCorlay commented Aug 12, 2018

xtensor-python should have >=0.20 instead of >0.20.

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 12, 2018

Member

The linter issue "When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably)." should be easily fixed.

You can get the right value for the sha256 by running the conda build with a wrong value. The error message gives you the right one :)

Member

SylvainCorlay commented Aug 12, 2018

The linter issue "When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably)." should be easily fixed.

You can get the right value for the sha256 by running the conda build with a wrong value. The error message gives you the right one :)

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 12, 2018

Member

I think that the windows build error comes from a syntax error with xcopy.

Member

SylvainCorlay commented Aug 12, 2018

I think that the windows build error comes from a syntax error with xcopy.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 12, 2018

Contributor

@SylvainCorlay
thanks for the tip regarding the sha256. I will include it once the remaining build issues are fixed.
Could you elaborate on the error you suspect in the windows build?

Note that there seem to be some other issues on windows:
constantinpape/z5#72

To test that the package is properly installed, we should also run unit-tests in the tests section.
I had a quick look at https://conda-forge.org/docs/testing.html but could not figure out which approach would be the best here (not that I am not installing the with the pacakge for now).
Any advise, @jakirkham?

Contributor

constantinpape commented Aug 12, 2018

@SylvainCorlay
thanks for the tip regarding the sha256. I will include it once the remaining build issues are fixed.
Could you elaborate on the error you suspect in the windows build?

Note that there seem to be some other issues on windows:
constantinpape/z5#72

To test that the package is properly installed, we should also run unit-tests in the tests section.
I had a quick look at https://conda-forge.org/docs/testing.html but could not figure out which approach would be the best here (not that I am not installing the with the pacakge for now).
Any advise, @jakirkham?

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 12, 2018

Member

You can also test that the files are installed like we do for xtensor.

Running the tests would require making use of the modern compiler infra of conda-build 3 and keep the corresponding libcxx around for running the tests.

Member

SylvainCorlay commented Aug 12, 2018

You can also test that the files are installed like we do for xtensor.

Running the tests would require making use of the modern compiler infra of conda-build 3 and keep the corresponding libcxx around for running the tests.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 13, 2018

Contributor

To clarify, I am referring to the python tests.
As far as I can see it should be possible to run them during test, because the import test I am performing is successful, see
https://github.com/constantinpape/conda-recipes/blob/master/recipes/z5py/meta.yaml#L43.

I am unsure what's the best way to achieve this though. As far as I understand, conda build creates a
clean environment for the tests, so I don't have access to the unit-tests
https://github.com/constantinpape/z5/tree/master/src/python/test.

One option would be to also install these tests into site-packages, but there might be a more elegant way.

Contributor

constantinpape commented Aug 13, 2018

To clarify, I am referring to the python tests.
As far as I can see it should be possible to run them during test, because the import test I am performing is successful, see
https://github.com/constantinpape/conda-recipes/blob/master/recipes/z5py/meta.yaml#L43.

I am unsure what's the best way to achieve this though. As far as I understand, conda build creates a
clean environment for the tests, so I don't have access to the unit-tests
https://github.com/constantinpape/z5/tree/master/src/python/test.

One option would be to also install these tests into site-packages, but there might be a more elegant way.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 13, 2018

Member

As to the tests, one can use the test/source_files section to have these copied over into the testing environment. Here's an example.

Member

jakirkham commented Aug 13, 2018

As to the tests, one can use the test/source_files section to have these copied over into the testing environment. Here's an example.

@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 13, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • The test section contained an unexpected subsection name. command is not a valid subsection name.

conda-forge-linter commented Aug 13, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).
  • The test section contained an unexpected subsection name. command is not a valid subsection name.
@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 13, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

conda-forge-linter commented Aug 13, 2018

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

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

Here's what I've got...

For recipes/z5py:

  • When defining a source/url please add a sha256, sha1 or md5 checksum (sha256 preferably).

constantinpape added some commits Aug 13, 2018

@conda-forge-linter

This comment has been minimized.

Show comment
Hide comment
@conda-forge-linter

conda-forge-linter Aug 15, 2018

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/z5py) and found it was in an excellent condition.

conda-forge-linter commented Aug 15, 2018

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/z5py) and found it was in an excellent condition.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 15, 2018

Contributor

I have fixed the windows issues.
The error on appveyor was caused by a syntax error in xcopy and I fixed this by using
the cmake install mechanism to install the python packages.
Note that constantinpape/z5#72 is not an issue here and all tests pass on windows.
I have drafted a 1.0.0 release of z5 and updated this recipe.

@jakirkham
This is good to merge from my side if all checks pass.

Contributor

constantinpape commented Aug 15, 2018

I have fixed the windows issues.
The error on appveyor was caused by a syntax error in xcopy and I fixed this by using
the cmake install mechanism to install the python packages.
Note that constantinpape/z5#72 is not an issue here and all tests pass on windows.
I have drafted a 1.0.0 release of z5 and updated this recipe.

@jakirkham
This is good to merge from my side if all checks pass.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 15, 2018

Member

Thanks @constantinpape. Looks pretty good.

There's a test failure on Windows due to cleaning up a temporary directory used in testing. Sadly this is a common issue with Windows (particularly with AppVeyor). It often happens if antivirus is involved (i.e. a process is still accessing files we are trying to delete). Some options below.

  1. Skip cleaning up the directory.
  2. Try cleaning up the directory with X retries.
  3. Remove files once we know they are unused.
  4. Use pytest and its temporary directories (they do cleanup for you).
Member

jakirkham commented Aug 15, 2018

Thanks @constantinpape. Looks pretty good.

There's a test failure on Windows due to cleaning up a temporary directory used in testing. Sadly this is a common issue with Windows (particularly with AppVeyor). It often happens if antivirus is involved (i.e. a process is still accessing files we are trying to delete). Some options below.

  1. Skip cleaning up the directory.
  2. Try cleaning up the directory with X retries.
  3. Remove files once we know they are unused.
  4. Use pytest and its temporary directories (they do cleanup for you).
@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 15, 2018

Contributor

Oh, I see.
I wrapped all rmtrees into a try / except OSError block now.
That should also fix the issue, because the rmtree fails with an OSError

Contributor

constantinpape commented Aug 15, 2018

Oh, I see.
I wrapped all rmtrees into a try / except OSError block now.
That should also fix the issue, because the rmtree fails with an OSError

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 15, 2018

Contributor

On second thought this might not be enough, because some of the following tests might fail if we don't clean up the previous test array.

Anyway, if the tests pass I think this should be fine, otherwise I will opt in pytest.

Contributor

constantinpape commented Aug 15, 2018

On second thought this might not be enough, because some of the following tests might fail if we don't clean up the previous test array.

Anyway, if the tests pass I think this should be fine, otherwise I will opt in pytest.

- boost-cpp
- numpy
- xtensor >=0.17.1,<0.18
- xtensor-python >=0.20,<0.21

This comment has been minimized.

@jakirkham

jakirkham Aug 16, 2018

Member

Are these things we should be pinning globally or do these just work well with the current state of z5py?

@jakirkham

jakirkham Aug 16, 2018

Member

Are these things we should be pinning globally or do these just work well with the current state of z5py?

This comment has been minimized.

@constantinpape

constantinpape Aug 16, 2018

Contributor

I think it would make sense to pin these globally.
@SylvainCorlay recommended to pin to these versions because
xtensor and xtensor-python API is still a bit unstable and can change with the next minor release.

@constantinpape

constantinpape Aug 16, 2018

Contributor

I think it would make sense to pin these globally.
@SylvainCorlay recommended to pin to these versions because
xtensor and xtensor-python API is still a bit unstable and can change with the next minor release.

This comment has been minimized.

@SylvainCorlay

SylvainCorlay Aug 16, 2018

Member
  • xtensor packages follow semver for the API. >=0.17.1,<0.18 is a means to specify ^0.17.1.
  • This means that z5 may not work with e.g. xtensor 0.18.
@SylvainCorlay

SylvainCorlay Aug 16, 2018

Member
  • xtensor packages follow semver for the API. >=0.17.1,<0.18 is a means to specify ^0.17.1.
  • This means that z5 may not work with e.g. xtensor 0.18.

@jakirkham jakirkham merged commit 2620f97 into conda-forge:master Aug 16, 2018

1 check was pending

ci/circleci: build CircleCI is running your tests
Details
@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Aug 16, 2018

Member

Thanks @constantinpape. It's in. 😄

Asked one question above, but it was non-blocking.

Member

jakirkham commented Aug 16, 2018

Thanks @constantinpape. It's in. 😄

Asked one question above, but it was non-blocking.

@constantinpape

This comment has been minimized.

Show comment
Hide comment
@constantinpape

constantinpape Aug 16, 2018

Contributor

Great!
Thanks a lot @jakirkham and @SylvainCorlay for all your help.

Contributor

constantinpape commented Aug 16, 2018

Great!
Thanks a lot @jakirkham and @SylvainCorlay for all your help.

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