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 scikit-build #1989

Merged
merged 18 commits into from Nov 28, 2016

Conversation

Projects
None yet
7 participants
@isuruf
Member

isuruf commented Nov 23, 2016

@jcfr, @msarahan, @scopatz let me know if you are interested in maintaining this.

@jcfr, can you take a look at the build and run requirements and see if any package is missing?

@conda-forge-linter

This comment has been minimized.

conda-forge-linter commented Nov 23, 2016

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

test:
imports:
- skbuild

This comment has been minimized.

@jcfr

jcfr Nov 23, 2016

Contributor

Would it make sense to run the full test suite ?

This comment has been minimized.

@isuruf

isuruf Nov 23, 2016

Member

Yes, how do I run the full test suite?

This comment has been minimized.

@jcfr

jcfr Nov 23, 2016

Contributor

pytest or python setup.py test will work

@scopatz

This comment has been minimized.

Member

scopatz commented Nov 23, 2016

Sure, feel free to add me.

@jcfr

This comment has been minimized.

Contributor

jcfr commented Nov 23, 2016

@isuruf Thanks for contributing the recipe. You rock !

build and run requirements and see if any package is missing
Looks good

test

I wonder if it would make sense to run the full test suite

interested in maintaining this

I would be happy to learn. My experience with conda recipe is limited.

In the mean time, your help is very much appreciated 😄

Cc: @thewtex

@jcfr jcfr referenced this pull request Nov 23, 2016

Closed

Conda package? #213

isuruf added some commits Nov 23, 2016

@isuruf

This comment has been minimized.

Member

isuruf commented Nov 23, 2016

I'm getting test failures. See build log here, https://circleci.com/gh/isuruf/staged-recipes/104

Most tests fail because cmake can't find FindPythonExtensions.cmake.
Two other test failures are there. One is no fortran compiler (It's better if we can skip this test). Second one is that the current working directory is not scikit-build but scikit-build-0.4.0. This seems like a weird requirement to run the test suite.

isuruf added some commits Nov 23, 2016

@msarahan

This comment has been minimized.

Member

msarahan commented on 27a5cbb Nov 23, 2016

Can add me if you want. I should be able to help more soon.

@isuruf

This comment has been minimized.

Member

isuruf commented Nov 23, 2016

I've disabled the 3 tests that are failing (2 that check the directory name and the one that checks for the fortran compiler.)

Ready for a final review now

script:
- python setup.py install --single-version-externally-managed --record record.txt
# Tests are skipped for now.
- py.test -k "not (test_push_dir or test_fortran_compiler)"

This comment has been minimized.

@scopatz

scopatz Nov 23, 2016

Member

This should be in the test block (not the build block) and should be under the "commands" block (which lives at the same level as "imports")

This comment has been minimized.

@isuruf

isuruf Nov 23, 2016

Member

Tests are not installed, so this has to be done in build phase.

This comment has been minimized.

@scopatz

scopatz Nov 23, 2016

Member

You can cd to the source directory in the test phase. This is what many recipes do.

This comment has been minimized.

@isuruf

isuruf Nov 23, 2016

Member

Can you give an example recipe that does this?

This comment has been minimized.

@scopatz

This comment has been minimized.

- python
- setuptools
- wheel
# Following dependencies are for testing needed in build phase

This comment has been minimized.

@scopatz

scopatz Nov 23, 2016

Member

Test dependencies should be in the "test" section in the "requires" section.

@scopatz

This comment has been minimized.

Member

scopatz commented Nov 23, 2016

@isuruf - please see other recipes and the conda build docs for more info on testing. Or feel free to ask questions

@msarahan

This comment has been minimized.

Member

msarahan commented Nov 23, 2016

Better way with conda build 2 is to use the test/source_files entry to
ensure that source files are present at test time.

On Nov 23, 2016 2:07 PM, "Isuru Fernando" notifications@github.com wrote:

@isuruf commented on this pull request.

In recipes/scikit-build/meta.yaml
#1989:

+package:

  • name: {{ name|lower }}
  • version: {{ version }}

+source:

+build:

  • number: 0
  • script:
    • python setup.py install --single-version-externally-managed --record record.txt
  • Tests are skipped for now.

    • py.test -k "not (test_push_dir or test_fortran_compiler)"

@scopatz https://github.com/scopatz, added this,
https://github.com/conda-forge/staged-recipes/wiki/Common-issues


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1989, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AACV-Y2LwUsoVjkLwNDdGqPzRqm81EAxks5rBJ0PgaJpZM4K6jBt
.

@jcfr jcfr referenced this pull request Nov 23, 2016

Closed

C matrices #158

@jcfr

This comment has been minimized.

Contributor

jcfr commented Nov 23, 2016

@isuruf Thanks for your persistence 👍 I will work on addressing your comments to improve scikit-build testing asap.

jcfr added a commit to scikit-build/scikit-build that referenced this pull request Nov 24, 2016

tests: Improve "push_dir" tests to not rely on build directory name
This commit increases the robustness of the test and should ensure
it pass when executed on conda-forge ci.
See conda-forge/staged-recipes#1989

Suggested-by: Isuru Fernando <isuruf@gmail.com>
# string IDs seem to be just the vs_base.
self.default_generators.insert(0, vs_base)
+ self.default_generators.insert(1, "NMake Makefiles")

This comment has been minimized.

@isuruf

isuruf Nov 24, 2016

Member

@jcfr, I've added this patch because of this issue, conda/conda-build#1327

This comment has been minimized.

@isuruf

isuruf Nov 24, 2016

Member

scikit-build test-suite doesn't like the fact that Visual Studio 9 2008 Win64 fails, but NMake doesn't.

>           assert(get_best_generator() == generator)
E           assert 'NMake Makefiles' == 'Visual Studio 9 2008 Win64'
@isuruf

This comment has been minimized.

Member

isuruf commented Nov 24, 2016

@msarahan, conda-forge is using conda-build 2.0.10, but test/source_files is not supported
See this build log https://circleci.com/gh/isuruf/staged-recipes/109

@isuruf

This comment has been minimized.

Member

isuruf commented Nov 24, 2016

sorry about the confusion, it's using 1.2.14

# string IDs seem to be just the vs_base.
self.default_generators.insert(0, vs_base)
+ self.default_generators.insert(0, "NMake Makefiles")

This comment has been minimized.

@isuruf

isuruf Nov 24, 2016

Member

I've changed this to make NMake Makefiles the default for win and py27 and x86_64. I see that in scikit-ci-addons, this is fixed in appveyor by running a script that needs elevated permissions, which a user might not have. I think there is no harm in trying NMake first before trying a Visual Studio project. Any objections or other suggestions?

@@ -0,0 +1,13 @@
MSVC 9 x64 is broken on appveyor. Use NMake there.

This comment has been minimized.

@jcfr

jcfr Nov 24, 2016

Contributor

It could easily be fixed using the patch_vs2008 add-on provided by scikit-ci-addons

See http://scikit-ci-addons.readthedocs.io/en/latest/addons.html#patch-vs2008-py

This comment has been minimized.

@isuruf

isuruf Nov 24, 2016

Member

Yes, there is also a conda package that does this, https://github.com/conda-forge/vs2008_express_vc_python_patch-feedstock

But this is discouraged because this requires admin privileges. cc @jakirkham

This comment has been minimized.

@jcfr

jcfr Nov 24, 2016

Contributor

I created an issue in scikit-build to better support Microsoft Visual C++ Compiler for Python 2.7. See scikit-build/scikit-build#216 for more details.

This comment has been minimized.

@jcfr

jcfr Nov 24, 2016

Contributor

requires admin privileges.

I think in practice it is reasonable to assume developers have admin privileges on their workstation.

Similarly to the installation of recent Visual Studio (e.g VS2015 for Python3.5) already requiring admin privileges, expecting developer to patch their install of Visual Studio 2008 express seems fair.

That said, installing arbitrary patches to modify an official installation could be a problem here and supporting " Microsoft Visual C++ Compiler for Python 2.7" would be a more sane alternative.

This comment has been minimized.

@jakirkham

jakirkham Nov 28, 2016

Member

Yeah, my thinking is we will install those patches in the CI beforehand as they are valuable occasionally. However, trying to use Microsoft Visual C++ Compiler for Python 2.7 is preferable.

@thewtex

This comment has been minimized.

Contributor

thewtex commented Nov 25, 2016

@isuruf Thanks for driving this forward. This is excellent!

@isuruf

This comment has been minimized.

Member

isuruf commented Nov 26, 2016

I've reverted the last commit and will use a better patch coming out of scikit-build/scikit-build#216.
That means for win and py27 and x86_64, NMake is used after checking MSVC. Conda should be activating NMake with the correct parameters and therefore no more checks are needed to make sure the NMake environment matches that of the python environment

@scopatz

This comment has been minimized.

Member

scopatz commented Nov 27, 2016

This seems reasonable to me. Any objections to merging?

@jakirkham

This comment has been minimized.

Member

jakirkham commented Nov 28, 2016

No objections from me. Looks good.

@scopatz scopatz merged commit c77b9eb into conda-forge:master Nov 28, 2016

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
conda-forge-linter All recipes are excellent.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@scopatz

This comment has been minimized.

Member

scopatz commented Nov 28, 2016

Thanks All!

@jcfr

This comment has been minimized.

Contributor

jcfr commented Nov 28, 2016

Thanks a lot 👍

commands:
- cd ${SRC_DIR} && py.test -k "not (test_push_dir or test_fortran_compiler)" # [unix]
- cd %SRC_DIR% && py.test -k "not (test_push_dir or test_fortran_compiler)" # [win and not (py27 and x86_64)]
- cd %SRC_DIR% && py.test -k "not (test_push_dir or test_fortran_compiler or test_generator_selection or test_hello_builds_with_generator)" # [win and py27 and x86_64]

This comment has been minimized.

@jakirkham

jakirkham Nov 28, 2016

Member

Something to keep in mind is we had problems with these x86_64 selectors recently in our maintenance scripts. ( conda-forge/dmd-feedstock#2 ) We may need to change the feedstock not to use these.

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