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

Updated recipe/meta.yaml to version 3.36.0 #19

Merged
merged 19 commits into from
Nov 11, 2017
Merged

Conversation

jochym
Copy link
Contributor

@jochym jochym commented Nov 9, 2017

No description provided.

@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 (recipe) and found it was in an excellent condition.

@jochym jochym changed the title Updated recipe/meta.yaml to version 3.36.0 WIP: Updated recipe/meta.yaml to version 3.36.0 Nov 9, 2017
@jochym
Copy link
Contributor Author

jochym commented Nov 9, 2017

@DRMacIver - here is a link to the log for 3.6 on linux:
https://circleci.com/gh/jochym/hypothesis-feedstock/126?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Would you care to take a look? Start at the end - it fails to import hypothesis.searchstrategy :
ModuleNotFoundError: No module named 'hypothesis.searchstrategy'

Even better is the last build:
https://circleci.com/gh/jochym/hypothesis-feedstock/126?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

@conda-forge-linter
Copy link

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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe (might be a conda-smithy bug) 😢

@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 (recipe) and found it was in an excellent condition.

@DRMacIver
Copy link

@jochym I really can't see anything obviously wrong here. In particular it's very strange for hypothesis and hypothesis.extra to import correctly and hypothesis.searchstrategy not.

My guess is that this problem must be somewhere in your build setup rather than Hypothesis because I've never seen this problem outside of conda (including a bunch of other downstream packagers), but I've no idea what that could be! Can you give me any pointers on how to reproduce this problem locally to debug?

@jochym
Copy link
Contributor Author

jochym commented Nov 9, 2017

Ok. This at least means that I am not doing something obviously wrong. I'll try to make a local version of the build/error for you to test.

@DRMacIver
Copy link

This at least means that I am not doing something obviously wrong.

At least not obvious to me. I want to reemphasise that I know remarkably little about conda packaging!

@jochym
Copy link
Contributor Author

jochym commented Nov 10, 2017

I have found at least one problem - the build dependence on attrs is circular. I think the package for attrs should move hypothesis dependency to testin requirements. I have submitted the PR to this effect (conda-forge/attrs-feedstock#6), but it will take some time to get accepted.
On my local system the package builds properly and imports work again. @DRMacIver would you review test section of the recipe/meta.yaml to make sure the imports which are supposed to work are there and the tests are executed in the way they are supposed to work?

recipe/meta.yaml Outdated
# - cd git_src
# - python -m pytest tests/cover
- hypothesis.searchstrategy
# - hypothesis.tools

Choose a reason for hiding this comment

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

BTW this package is actually gone, so any failures to import it are genuine.

recipe/meta.yaml Outdated
commands:
- git clone https://github.com/HypothesisWorks/hypothesis-python.git src
- cd src
- bash scripts/basic-test.sh

Choose a reason for hiding this comment

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

You might want to not use this - it has a lot of reliance on pip to manage installs as it runs the tests.

recipe/meta.yaml Outdated
# - python -m pytest tests/cover
# - python -m pytest tests/datetime
# - python -m pytest tests/fakefactory
# - python -m pytest tests/pytest -p pytester --runpytest subprocess

Choose a reason for hiding this comment

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

If you restore these it might be worth adding tests for the tests/numpy and tests/pandas sections too, given conda's user demographics! Both of those would then have to be added as testing dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I switched the tests and made it to:

- python -m pytest tests/cover
- python -m pytest tests/datetime
- python -m pytest tests/fakefactory
- python -m pytest tests/numpy
- python -m pytest tests/pandas
- python -m pytest tests/py2  # [py2k]
- python -m pytest tests/py3  # [py3k]
- python -m pytest tests/pytest -p pytester --runpytest subprocess

Is this supposed to work?

Choose a reason for hiding this comment

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

Yes, that should work (except for comment about pytest-xdist elsewhere)

recipe/meta.yaml Outdated
# requires:
# - pytest
requires:
- pytest
imports:
- hypothesis
- hypothesis.extra

Choose a reason for hiding this comment

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

I'm not totally clear on what the imports tests are for, but might it be worth switching these to correspond more to which ones are in the public API? That would probably be:

  • hypothesis
  • hypothesis.extra
  • hypothesis.strategies (note: not the same as hypothesis.searchstrategy)
  • hypothesis.stateful

The others that are currently there are mostly internal packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done all above. Thanks - let us wait for the results of CI.

recipe/meta.yaml Outdated
# requires:
# - pytest
requires:
- pytest

Choose a reason for hiding this comment

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

If you move away from using the basic-test.sh then the following are probably also useful testing requirements:

  • pytz
  • numpy
  • pandas
  • faker

Choose a reason for hiding this comment

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

Sorry a thing that I've just noticed will probably give you trouble: I think pytest-xdist should also be a testing dependency - otherwise when you run pytest in the testing section it will pick up the "-n 2" flag from the tox.ini and won't recognise it.

Choose a reason for hiding this comment

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

(If you want to disable it running in parallel you can then pass "-n 0" to pytest to override)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://circleci.com/gh/jochym/hypothesis-feedstock/152

  • python -m pytest tests/cover
    usage: pytest.py [options] [file_or_dir] [file_or_dir] [...]
    pytest.py: error: unrecognized arguments: -n
    inifile: /feedstock_root/build_artefacts/hypothesis_1510330029786/test_tmp/src/tox.ini
    rootdir: /feedstock_root/build_artefacts/hypothesis_1510330029786/test_tmp/src
    TESTS FAILED: hypothesis-3.36.0-py36_0.tar.bz2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added pytest-xdist to the dependencies.

Choose a reason for hiding this comment

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

Sorry, I forgot about flaky. That also needs to be in the testing dependencies.

@@ -12,34 +12,39 @@ source:
sha256: {{ sha256 }}

build:
skip: True # [py<34]

Choose a reason for hiding this comment

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

Depending on which versions of Python are currently supported in conda you may need to restrict this further (all of the ones in your actual CI are fine though)

@DRMacIver
Copy link

Looking at your failing tests:

  • I think the failure on Python 2 is a bug in the faker packaging for conda. It should be installing an ipaddress backport under Python 2. That being said it looks like it is meant to based on the run dependencies: https://github.com/conda-forge/faker-feedstock/blob/master/recipe/meta.yaml#L31
  • No idea what's going on with the xdist error on Python 3.5. FWIW if you don't want to debug, just passing "-n 0" to each pytest command will make those go away by turning off xdist.

@jochym
Copy link
Contributor Author

jochym commented Nov 10, 2017

OK, Thanks @DRMacIver - it looks like we have done it.
Any additional tests we should add to the package?
BTW: To respond to your earlier question - imports just test if the package is properly visible after install.

- faker
- mock
- flaky
- ipaddress # [py2k]

Choose a reason for hiding this comment

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

Hmm is this a workaround? Because really this should be installed as part of the faker dependency.

@DRMacIver
Copy link

Any additional tests we should add to the package?

No, this looks good to me.

@jochym jochym changed the title WIP: Updated recipe/meta.yaml to version 3.36.0 Updated recipe/meta.yaml to version 3.36.0 Nov 10, 2017
@jochym jochym merged commit 0d00a87 into conda-forge:master Nov 11, 2017
@jochym
Copy link
Contributor Author

jochym commented Nov 11, 2017

Thanks @DRMacIver for your help!

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.

3 participants