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

Create recipe.yaml for simplejson #458

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tylere
Copy link

@tylere tylere commented May 13, 2023

This should fix #456

@jtpio jtpio requested a review from DerThorsten May 13, 2023 15:53
@proppy
Copy link

proppy commented May 13, 2023

Looking at the CI logs it doesn't look like anything is being built:

2023-05-13T15:51:09.6806670Z Extracting download
2023-05-13T15:51:09.6807056Z 
2023-05-13T15:51:09.6807333Z Preparing environment for simplejson
2023-05-13T15:51:09.6807744Z 
2023-05-13T15:51:09.6816232Z 
2023-05-13T15:51:09.6816472Z Starting build for simplejson
2023-05-13T15:51:09.6816752Z 
2023-05-13T15:51:09.6832498Z source tree in: 
2023-05-13T15:51:09.6833222Z /home/runner/micromamba-root/envs/ci-env/conda-bld/simplejson-0_1683993068571/wo
2023-05-13T15:51:09.6833631Z rk
2023-05-13T15:51:09.7308385Z number of files: 0
2023-05-13T15:51:09.7308669Z Fixing permissions
2023-05-13T15:51:09.7309843Z 
2023-05-13T15:51:09.7310287Z Adding files for simplejson
2023-05-13T15:51:09.7310626Z 
2023-05-13T15:51:09.7314134Z ATTENTION: No files added in target simplejson
2023-05-13T15:51:09.7316084Z 
2023-05-13T15:51:09.7316105Z 

version: "{{ version }}"

build:
number: 0
Copy link

Choose a reason for hiding this comment

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

since there are some C part to this library:
https://github.com/simplejson/simplejson/blob/master/simplejson/_speedups.c
shouldn't we also add {{ compiler('c') }} as a build requirement (see https://github.com/conda-forge/simplejson-feedstock/blob/main/recipe/meta.yaml#LL23C7-L23C26) or disable the speed ups with REQUIRE_SPEEDUPS to make it pure python.

Copy link
Contributor

Choose a reason for hiding this comment

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

@proppy absolutely correct!


build:
number: 0

Copy link

Choose a reason for hiding this comment

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

should we also get the test from the original conda-forge recipe? https://github.com/conda-forge/simplejson-feedstock/blob/main/recipe/meta.yaml#L30-L38

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to add such tests in the form of a test_simplejson.py / test_<some_name>.py and test the imports there.
ie:

def test_imports():
    import simplejson
    import simplejson._speedups 


source:
- url: https://files.pythonhosted.org/packages/c0/5c/61e2afbe62bbe2e328d4d1f426f6e39052b73eddca23b5ba524026561250/simplejson-{{ version }}.tar.gz
sha256: 6277f60848a7d8319d27d2be767a7546bc965535b28070e310b3a9af90604a4c
Copy link

Choose a reason for hiding this comment

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


source:
- url: https://files.pythonhosted.org/packages/c0/5c/61e2afbe62bbe2e328d4d1f426f6e39052b73eddca23b5ba524026561250/simplejson-{{ version }}.tar.gz
sha256: 6277f60848a7d8319d27d2be767a7546bc965535b28070e310b3a9af90604a4c
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

@DerThorsten DerThorsten left a comment

Choose a reason for hiding this comment

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

tests should be added

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.

Package simplejson
3 participants