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

Add poliastro and jplephem recipes #214

Merged
merged 10 commits into from
Apr 2, 2016
Merged

Conversation

astrojuanlu
Copy link
Member

By suggestion of @pelson I'm trying conda-forge with my own package. It was not clear to me what the rules are for a proper feedstock so I will wait for the bot to lint my recipe :) (inspired by #84 (comment))

@astrojuanlu
Copy link
Member Author

Oh well... my second last name has a non-ASCII character.

https://github.com/conda/conda/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+unicode+OR+ascii

(Closest: conda/conda#1486)

Would you consider using Miniconda3 on https://github.com/pelson/Obvious-CI?

@jakirkham
Copy link
Member

Could you please open the above as an issue on this repo (with reference back to this PR and conda issue)?

@astrojuanlu
Copy link
Member Author

The problem is here:

https://github.com/conda-forge/staged-recipes/blob/280b670/.CI/build_all#L8

I guess it would be a matter of changing the major Python version.

@@ -0,0 +1,3 @@
#!/bin/bash

pip install --no-deps .
Copy link
Member

Choose a reason for hiding this comment

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

Ack! Can we not use pip? This is likely to cause problems for conda.

Copy link
Member

Choose a reason for hiding this comment

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

I've seems some recipes like that in the wild and I wonder what are the possible problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact this works just fine, I've been using this recipe for a while already:

poliastro/poliastro@9d25954

I don't know what is the current (March 2016) consensus on this, but half a year ago this thread on numpy-dev prompted me to change that line:

https://mail.scipy.org/pipermail/numpy-discussion/2015-October/074086.html

Of course I can restore it to its previous state.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's use setup.py. Sometimes conda can be a bit finicky with using pip in a recipe. Normally we tack on a few arguments that tend to help anyways. In particular, we do the following.

python setup.py install --single-version-externally-managed --record record.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, I will make the change then. For reference: https://github.com/conda-forge/cartopy-feedstock/blob/a23c28a/recipe/build.sh#L5

Copy link
Member

Choose a reason for hiding this comment

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

Well, when it comes to Mac and Linux. We will fold this into script under build in the meta.yaml file. Unfortunately, we can't do this for Windows due to a conda build bug.

@jakirkham
Copy link
Member

Added some comments for the recipe. Once these are done we can go through another round.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

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

Here's what I've got...

For recipes/jplephem:

  • The recipe could do with some maintainers listed in the "extra/recipe-maintainers" section.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/poliastro, recipes/jplephem) and found it was in an excellent condition.

@jakirkham
Copy link
Member

Yeah, that linter is pretty neat, right? Very new addition (thanks to @pelson).

@@ -0,0 +1,2 @@
%PYTHON% setup.py install
Copy link
Member

Choose a reason for hiding this comment

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

Also needs --single-version-externally-managed --record record.txt.

@astrojuanlu
Copy link
Member Author


requirements:
build:
- python
Copy link
Member

Choose a reason for hiding this comment

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

Please add setuptools.

Copy link
Member

Choose a reason for hiding this comment

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

Done.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 28, 2016

Perhaps setuptools is missing?

Yep. If poliastro uses setuptools add it as a dependency. Otherwise just do a python setup.py install.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/poliastro, recipes/jplephem) and found it was in an excellent condition.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/poliastro, recipes/jplephem) and found it was in an excellent condition.

@astrojuanlu
Copy link
Member Author

I forgot to add that poliastro does not support Python 2. What would be the best way to disable it from the build?

@ocefpaf
Copy link
Member

ocefpaf commented Mar 29, 2016

I forgot to add that poliastro does not support Python 2. What would be the best way to disable it from the build?

Add this to your build section (last line):

build:
  number: 0
  skip: True # [py2k]

@astrojuanlu
Copy link
Member Author

Well, I just remembered another PyPI-only requirement I've got with poliastro. I will have to step back and provide a feedstock for it too... 😤

@astrojuanlu
Copy link
Member Author

Done, waiting on #226.

@astrojuanlu astrojuanlu reopened this Mar 31, 2016
@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/jplephem, recipes/poliastro) and found it was in an excellent condition.

@astrojuanlu
Copy link
Member Author

Closing and reopening to trigger CI.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 31, 2016

@Juanlu001 sometimes that does not work. You can ping me to re-start them manually.

@astrojuanlu
Copy link
Member Author

@ocefpaf True, it did work in this case though. Don't worry, I think I still have to make some extra commits 😅

@ocefpaf
Copy link
Member

ocefpaf commented Mar 31, 2016

No it didn't 😉 (I restarted CircleCI.)

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/jplephem, recipes/poliastro) and found it was in an excellent condition.

@astrojuanlu
Copy link
Member Author

The Windows builds are failing because pytest-benchmark is not available on Windows yet. This is because the build is still queued on AppVeyor 😦

https://ci.appveyor.com/project/conda-forge/pytest-benchmark-feedstock

@ocefpaf
Copy link
Member

ocefpaf commented Mar 31, 2016

There is nothing you can do now but wait...

@pelson
Copy link
Member

pelson commented Apr 2, 2016

Some of the matrix items passed, and I have no reason to believe that the others wont now that the dependencies are available. I'm going to merge this, but if you could keep an eye on the feedstock(s) and raise any issues there if there are Windows build problems.

@pelson pelson merged commit 849ffe4 into conda-forge:master Apr 2, 2016
@jakirkham
Copy link
Member

Yeah, I have taken the same attitude with AppVeyor before. We should really discuss our options here because it is become clear the current strategy is not working.

@astrojuanlu
Copy link
Member Author

Thanks guys! 🎉 I will make use of this material for my talk at PyData Madrid next week 😄

@astrojuanlu
Copy link
Member Author

Folks, is there a way to restart the poliastro builds? I just checked the poliastro-feedstock recipes and the jplephem package was missing. I suppose that if they were created at the same time, perhaps the dependencies were not in place by the time the CI builds triggered.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 3, 2016

@Juanlu001 I re-started the CIs for you, but you should be able to re-start CircleCI and Travis-CI as you are part of the team poliastro. I guess you'll still need to ask for help to re-start AppVeyor.

PS: Note that Travis-CI might not be in sync and may not show the re-start button. Just sync it manually and the button will appear.

@astrojuanlu
Copy link
Member Author

Perfect, thanks!

@pelson
Copy link
Member

pelson commented Apr 4, 2016

An alternative heavy weight solution: add a commit to the repo 😉 (though I know it isn't ideal).

Not looking forward to it, but I suspect I'm going to have to get down and dirty with the AppVeyor API sometime soon. 😒

@astrojuanlu astrojuanlu changed the title WIP: Add poliastro recipe WIP: Add poliastro and jplephem recipes Apr 8, 2016
@jakirkham jakirkham changed the title WIP: Add poliastro and jplephem recipes Add poliastro and jplephem recipes Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants