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

Try to render the recipe again if it requires downloading source #697

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tsibley
Copy link
Contributor

@tsibley tsibley commented Feb 18, 2021

Preserves the faster path for most recipes by not downloading source,
but now allows recipes to use some of Conda's recipe features which
require source (e.g. load_setup_py_data()).

Preserves the faster path for most recipes by not downloading source,
but now allows recipes to use some of Conda's recipe features which
require source (e.g. load_setup_py_data()).
@tsibley
Copy link
Contributor Author

tsibley commented Feb 18, 2021

Looks like two checks failed, both of which I'm not sure I can do anything about:

  1. OS X tests on GitHub Actions, with 166 AttributeError: type object 'object' has no attribute 'dtype' errors in seemingly-unrelated code. Example. This smells to me like bad tests/CI weirdness, not caused by changes this PR made, but it's hard for me to say for sure as a first-time contributor. Would definitely appreciate some pointers from someone more familiar so I don't have to spend a lot of time digging.

  2. bioconda-utils-test / build-docs with an "unauthorized" error from CircleCI, presumably because PRs from forks are not allowed to run that job.

@mbargull
Copy link
Member

Hi @tsibley, thanks for the suggestion and PR!

I think load_setup_py_data and similar are nice features, e.g., when you use them in a project's source repository to get an effortless Conda recipe alongside your setup.py.
For build script repositories like bioconda-recipes I'd be rather cautious in using them.
In case of load_setup_py_data you might even execute arbitrary code that could be disruptive to the CI setup.
But even use of load_file_regex and similar it can, due to its content-dependent dynamic nature, hinder future automations like automatic reformatting of recipes or conversion to future recipe formats (e.g., there is ongoing work for new recipe formats which are more structured with less Jinja control structures etc.).

@tsibley
Copy link
Contributor Author

tsibley commented Feb 19, 2021

@mbargull Ah, bummer, so does that mean this is a "won't fix" then?

My concrete use case is to prevent re-packaging fatigue and divergence/drift from the metadata in the source package, such as requirement specs from setup.py. I have an example of this working locally (along with this PR). Such automatic support would have prevented a real-world, bug-causing issue with packaging drift that I recently had to fix.

I did first scour the Bioconda docs, in particular the recipe guidelines, for anything related to this class of recipe features, but came up empty handed. If the official Bioconda policy is to not allow or support these features, it would be very useful to have that documented somewhere visible for recipe authors.

I appreciate the issue of complicating future transformations when the YAML is templated, but it seems to me like that horse has already bolted with the current state of recipes and would be very hard to lead back to the barn. As for the worry about arbitrary code execution, I'm not sure I follow. Every single recipe is already performing arbitrary code execution when it's built. Ostensibly, the digests ensure that someone vouched for the source at some point (though in practice I suspect no one audits every version bump and instead does what ~everyone does: trusts that it mostly works out ok).

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.

2 participants