Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have
{{ PYTHON }}
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the
script
string on the newconda-skeleton
and my guess is that{{ PYTHON }}
is safer thanpython
b/c ensure the env variable for is used. Is that correct @msarahan?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ensures that the host env Python is always used. It is equivalent to using the env var, but works on both win and Unix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any documentation on
{{ PYTHON }}
and friends?If I read
https://github.com/conda/conda-build/blob/3.12.0/conda_build/jinja_context.py#L496
https://github.com/conda/conda-build/blob/3.12.0/conda_build/environ.py#L253
https://github.com/conda/conda-build/blob/3.12.0/conda_build/environ.py#L323-L332
correctly,
{{ PYTHON }}
has the backslashes escaped for windows paths and thus it is correct (and even necessary) to use it in a double quote-style YAML scalar, correct?NB: Looking at this, the increasing list of options and more importantly things like pypa/pip#5605, it would be very advantageous (for robustness' sake) to have something like
script: fancy-conda-python-package-installer
that takes care of all those things... ref: #611There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree, @mbargull. How about
script: {{ compiler('python') }}
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, why not keep
python
instead of{{ PYTHON }}
? Alreadyconda
activates the correct environment putting the correctpython
on the path. Am not aware of any issues that just usingpython
in the recipe has caused.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @msarahan message above:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming ❤️
I don't think this abstraction/indirection is needed necessarily; just a "
fancy-conda-python-package-installer
" package/script/binary for starters.No hard feelings 😉.
IMHO,
conda-build
does some domain-specific things that I argueI usually favor more abstract/general approaches (e.g., move Python-specific things out of
conda-build
and into a dedicated package) -- but of course with limits, i.e., I agree that unnecessary indirections that only complicate things should be avoided. In the end, most beauty lies in simplicity; and the proposed slight indirection seems the more simple and maintainable solution to me.That's more of the (potentially) intransparent domain-specific "black magic" I'd like to avoid putting into
conda-build
itself and rather have it (from my POV) a little more explicitly exposed in a separate tool.Because of some reasons, the
build
environment is activated on top of thehost
one. Meaning, if you addsome-python-tool
torequirements/build
,python
will be inbuild
and thus be the one at the top ofPATH
.BUT: My "NB" was mostly only meant to reference gh-611 -- we shouldn't have this discussion here, but rather over at the linked or a new issue 😉.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like adding a new Jinja2 function where it isn't necessary. I agree that having a substitution is helpful here, but see no reason why it should be anything more than just another variable in your CBC.yaml file. Since we are already setting env vars rather globally to control pip (disable build isolation, conda/conda-build#3053), I also wonder how much value any further variable even adds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what PR ( conda/conda-build#3053 ) is meant to do.