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

5.1.4, noarch #51

Closed
wants to merge 3 commits into from
Closed

5.1.4, noarch #51

wants to merge 3 commits into from

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 27, 2020

Having dropped Python 2 support, we no longer have conditional dependencies and can be noarch.

@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.

@jamesmyatt
Copy link

Looks like it's hitting this: pytest-dev/pytest#6517

@jamesmyatt
Copy link

Might be simplest to pin pytest at 5.3.3 for this release.


build:
number: 0
skip: True # [py<34]
noarch: python
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we in fact go noarch when the kernel.json still has a platform-specific path? I've wondered if we couldn't go to file:/// refs, which are more portable... I may tinker with the bot PR...

@@ -22,17 +22,17 @@ requirements:
- ipython >=5.0
- jupyter_client
- pip
- python
- python >=3.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Given it's no longer tested on 3.4, shouldn't this be 3.5?

Choose a reason for hiding this comment

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

Surely it should match whatever setup.py says. i.e. what it can be installed with, rather than what it should be installed with.

@@ -22,17 +22,17 @@ requirements:
- ipython >=5.0
- jupyter_client
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to force a pin here from the recent client releases w/r/t py38/win issues?

Choose a reason for hiding this comment

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

I'd just make this match setup.py

- tornado >=4.2
- traitlets >=4.1

test:
requires:
- curio # [unix]
- curio
- nose
- numpy
- pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, pinning this to pytest !=5.3.4 does get the noarch to build locally... haven't tried on windows.

@bollwyvl
Copy link
Contributor

So I think we can try pushing this out as noarch, but I don't know if the win/py38 issues have actually been resolved, so a lot of folk might start grabbing this, and then have a whole new set of problems that we haven't even seen yet...

@bollwyvl
Copy link
Contributor

Over on #52, i was able to get the tests to pass (with one flake) by applying the asyncio patch before programmatically running the tests. That seems pretty heavy, as "works with py38 on windows" is a pretty big feature, and i haven't dug into the failures not covered to know whether this will meet every downstreams needs without a patch. However, I assume the path that is covered by the patch, namely folks starting kernels under notebook or nbconvert, will be fine.

@bollwyvl
Copy link
Contributor

I have still not tried building this noarch on linux and installing on windows to see if the kernel.json gets rewritten properly...

@bollwyvl
Copy link
Contributor

Confirmed: the kernel.json isn't going to work as noarch, as %PREFIX%/bin/python just isn't going to work.

@bollwyvl
Copy link
Contributor

However, once i changed the kernel.json, everything does work on py38/win: notebook, lab and nbconvert --execute. Hooray!

@minrk
Copy link
Member Author

minrk commented Jan 28, 2020

Can we in fact go noarch when the kernel.json still has a platform-specific path?

bwomp, nope. Forgot about that file. Merged #52 instead. Thanks for keeping it correct!

@minrk minrk closed this Jan 28, 2020
@bollwyvl
Copy link
Contributor

bollwyvl commented Jan 28, 2020 via email

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.

4 participants