conda-forge / ambertools-feedstock

A conda-smithy repository for ambertools.

BSD 3-Clause "New" or "Revised" License

8 stars 14 forks source link

No Cython #122

Closed mattwthompson closed 1 year ago

mattwthompson commented 1 year ago

120 and #121, except not

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

conda-forge-webservices[bot] commented 1 year ago

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.

mikemhenry commented 1 year ago

So does this work because cython isn't a runtime dep, as in the files have been pre-cythonized? I can't really remember how the cython tool chain works

mikemhenry commented 1 year ago

If this also works though, I don't expect the cython 0.3 release to break anything since it doesn't seem used (we have CI checks in this recipe that would catch a run time issue, right?)

mattwthompson commented 1 year ago

So does this work because cython isn't a runtime dep, as in the files have been pre-cythonized? I can't really remember how the cython tool chain works

IIUC that's one of the ways it can work; source can be re-cythonized at build time as well, but that doesn't seem to happen or be necessary here

If this also works though, I don't expect the cython 0.3 release to break anything since it doesn't seem used

Right, and if it's not used at build time it's even less important

(we have CI checks in this recipe that would catch a run time issue, right?)

I assume upstream would; feedstock "CI" doesn't operate like CI so it'd only catch this if we happen to be re-building for some other reason

I was hunting down some other stuff and thought this might be the cause of some downstream issues. It turns out that isn't the case, but I think it'd still be useful to merge this (if it's not needed, we shouldn't force downstreams to install Cython!) and close the other two linked PRs

mikemhenry commented 1 year ago

I meant check like https://github.com/conda-forge/ambertools-feedstock/blob/main/recipe/run_test.sh and it looks like from the output they are good

mikemhenry commented 1 year ago

Do we want someone else to look at this? I am happy with it

mattwthompson commented 1 year ago

I feel like if it was needed at runtime, we'd likely see something break. Hai seems to think it's not needed, so I think this is good to go - feel free to merge yourself or solicit more feedback

mikemhenry commented 1 year ago

I'll do it! I'm done for the day so whoever is on call will have to deal with issues :rofl: