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

Make recipes beautiful again #2972

Merged
merged 7 commits into from
Jul 20, 2018

Conversation

ocefpaf
Copy link
Contributor

@ocefpaf ocefpaf commented Jun 15, 2018

@msarahan hopefully I did not break anything. The goal is to:

  • enforce sha256;
  • change the script section to use pip;
  • simplify the recipe by reducing the excess of jinja variables.

I'm sure there are more places we can optimize and simplify the code but I'm not too familiar with conda-skeleton to go any further.

Also, I'd like to remove the single quotes from the name and version values in the package section, to keep a consistent style with the rest of the recipe, but I'm not sure how to do that in an ruamel_yaml.comments.CommentedMap object (and that is not really that important anyway.)

fixes #3000

if any(re.match(r'^setuptools(?:\s|$)', req) for req in d['build_depends']):
ordered_recipe['build']['script'] += ('--single-version-externally-managed '
'--record=record.txt')
ordered_recipe['build']['script'] = 'python -m pip install . --no-deps --ignore-installed'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are forcing pip by default, we also need to disable the hermetic build stuff, since that duplicates conda-build's intent and can also break what conda-build does.

https://github.com/pypa/pip/pull/5053/files#diff-1457c307ea972916e3ade69c27f0c41aR456

Now the thing that really stinks is that that flag is only available on new pip, so it'll break old builds, or ecosystems that don't have pip 10 yet.

I'm OK with the rest of the changes in this PR, but I don't think we're ready for this one yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a environment variable we could set which would turn off build isolation and still work with pip 9

Copy link
Contributor

Choose a reason for hiding this comment

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

great, that would be better. I'll see if we can find it.

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 add --no-cache-dir to the pip arguments as well (to force it to re-create/re-download the wheel and not rely on local cache, if present)? I was bit by this quite a few times.

Copy link
Contributor

Choose a reason for hiding this comment

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

PIP_NO_BUILD_ISOLATION=False disables build isolation. My expectation was that this should be set to True but pypa/pip#5229 and my own testing confirms that False is the correct setting. This variable has no effect on older pip versions.

I would also suggest adding -vvv to increase verbosity which makes debugging failed builds much easier.

@msarahan msarahan force-pushed the make_recipes_beautiful_again branch from 844701e to 25b6a85 Compare July 15, 2018 17:43
@msarahan
Copy link
Contributor

I rebased onto master and added a bit. I hope that's OK.

@msarahan msarahan force-pushed the make_recipes_beautiful_again branch from 25b6a85 to 6d16307 Compare July 15, 2018 17:44
if any(re.match(r'^setuptools(?:\s|$)', req) for req in d['build_depends']):
ordered_recipe['build']['script'] += ('--single-version-externally-managed '
'--record=record.txt')
ordered_recipe['build']['script'] = ('python -m pip install . --no-deps '
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be $PYTHON instead or is that not required?

Copy link
Contributor

Choose a reason for hiding this comment

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

not strictly. python should always be on PATH always in the env. If you make it $PYTHON, that's more definite, but it's not strictly necessary. I'll go ahead and change it, though.

Copy link
Member

Choose a reason for hiding this comment

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

Hm.. although I'm usually in favor of generalizing and assuring correctness, I'd rather have just python instead of "${PYTHON}"/"%PYTHON%" here. See comments from conda-forge/conda-forge.github.io#551 (comment) onward.
Just python is also the prevalent thing on aggregate and feedstocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was some reason why Intel had this pattern. I can't remember. It did have some advantage over depending on PATH.

The current code uses jinja2 instead of env vars, and should work. Errr... it will, when I can get to a better internet connection that doesn't give me git errors.

@msarahan msarahan force-pushed the make_recipes_beautiful_again branch from 3dee69b to 6c1ef40 Compare July 17, 2018 14:08
@msarahan msarahan force-pushed the make_recipes_beautiful_again branch 5 times, most recently from 573edba to 4f4d45c Compare July 18, 2018 00:21
@ocefpaf
Copy link
Contributor Author

ocefpaf commented Jul 19, 2018

Thanks for the review here. I'll try to address them ASAP, just got caught on a storm here at the day job.

@msarahan
Copy link
Contributor

Don't worry about it. I'll fix the CI crap and it'll be ready to go. Appveyor is really misbehaving, and I don't understand why yet.

@msarahan msarahan force-pushed the make_recipes_beautiful_again branch 2 times, most recently from df52fc3 to b82b367 Compare July 19, 2018 16:44
@msarahan msarahan force-pushed the make_recipes_beautiful_again branch 5 times, most recently from a3e0b69 to 7480c9a Compare July 20, 2018 01:06
@msarahan msarahan force-pushed the make_recipes_beautiful_again branch from 7480c9a to e64bfcf Compare July 20, 2018 02:05
@msarahan
Copy link
Contributor

finally fixed. Having PYTHON set in the Appveyor matrix seemed to be overriding conda-build, which meant that the {{ PYTHON }} was something dumb, like C:\Miniconda2-x64

@msarahan msarahan merged commit 9660d13 into conda:master Jul 20, 2018
@ocefpaf ocefpaf deleted the make_recipes_beautiful_again branch July 20, 2018 11:15
@ocefpaf
Copy link
Contributor Author

ocefpaf commented Jul 20, 2018

Thanks @msarahan for taking care of this!

@jakirkham
Copy link
Member

Thanks @ocefpaf and @msarahan. This is very good to have. :)

@github-actions
Copy link

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Apr 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyPI skeleton not coping well with environment markers?
6 participants