-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
MNT: rerender #349
MNT: rerender #349
Conversation
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 ( |
Hi! This is the friendly automated conda-forge-webservice. I tried to rerender for you but ran into some issues. Please check the output logs of the latest webservices GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or you can try rerendering locally. This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-ci-setup-feedstock/actions/runs/11042286022. |
@jakirkham rerendering is failing too This might be conda-build 24.9 |
The rerender is still broken in conda-build 24.7 which is fun. |
Here is the error message from CI: Traceback (most recent call last):
File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/metadata.py", line 2003, in _get_contents
rendered = template.render(environment=env)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/jinja2/environment.py", line 1304, in render
self.environment.handle_exception()
File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/jinja2/environment.py", line 939, in handle_exception
raise rewrite_traceback_stack(source=source)
File "/tmp/tmpzes4vsc5/conda-forge-ci-setup-feedstock/recipe/meta.yaml", line 24, in top-level template code
- cp {{ os.environ["FEEDSTOCK_ROOT"] }}/LICENSE.txt ${RECIPE_DIR}/LICENSE.txt # [linux]
^^^^^^^^^^^^^^^^^^^^^^^^^
jinja2.exceptions.UndefinedError: 'os._Environ object' has no attribute 'FEEDSTOCK_ROOT' Edit: This points to this line
|
Yeah this change fixes it locally:
What's weird is that I have no idea when this broke. That line of code has not been touched for ~4 years. |
Yeah does seem like a conda-build thing. That said, am confused why we have 3 different approaches for this use case conda-forge-ci-setup-feedstock/recipe/meta.yaml Lines 24 to 26 in 2523297
|
I doubt it is conda-build though with the change here. Now the cross-compile jobs are failing when they did not before. |
The build envs are radically different from this morning to this job. IDK what is going on but it is a lot. cc @isuruf @h-vetinari who have been doing work on compilers lately. Any ideas on why cross-python is failing? |
failing build env
|
passing build env
|
Summarizing discussion in PR: #337 We noticed the __main__.py: error: argument --sysroot: expected one argument This led us to the $BUILD_PREFIX/bin/python -m crossenv $PREFIX/bin/python \
--sysroot $CONDA_BUILD_SYSROOT \
--without-pip $BUILD_PREFIX/venv \
--sysconfigdata-file "$sysconfigdata_fn" \
--machine ${machine} \
--cc ${CC:-@CC@} \
--cxx ${CXX:-@CXX@} So for some reason |
Also saw this issue ( conda-forge/binutils-feedstock#76 ) earlier. Could that be related or is it something else? |
I doubt it is the binutils issue. We need to figure out what sets CONDA_BUILD_SYSROOT. A google search did not yield anything. |
It is usually set by the compiler activation scripts: https://github.com/conda-forge/ctng-compiler-activation-feedstock/blob/main/recipe/activate-gcc.sh#L187C4-L187C23 |
We set "CONDA_BUILD_SYSROOT,${CONDA_PREFIX}@LIBRARY_PREFIX@/@CHOST@/sysroot" \ Not sure if that is the only place though |
jinx! |
Well the |
The failing jobs skip activation of the compilers all together. |
recipe/meta.yaml
Outdated
# Copy feedstock license file to recipe directory | ||
- pushd {{ RECIPE_DIR }} | ||
- pushd .. | ||
- cp LICENSE.txt {{ RECIPE_DIR }} | ||
- popd | ||
- popd |
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.
As we mount the recipe directory and the feedstock directory next to each other...
conda-forge-ci-setup-feedstock/.scripts/run_docker_build.sh
Lines 87 to 88 in 2523297
-v "${RECIPE_ROOT}":/home/conda/recipe_root:rw,z,delegated \ | |
-v "${FEEDSTOCK_ROOT}":/home/conda/feedstock_root:rw,z,delegated \ |
... pushd {{ RECIPE_DIR }}/..
would resolve to /home/conda
(not mention not work on Windows)
To avoid that behavior, we pushd
to {{ RECIPE_DIR }}
. Once in {{ RECIPE_DIR }}
, we are within the feedstock directory's recipe
directory. So pushd ..
resolves to the feedstock directory (instead of /home/conda
as it would have in the other case)
Once we are in the feedstock directory, we can copy the feedstock license file and popd
back to where we were
This all works around the issue we saw where FEEDSTOCK_ROOT
didn't resolve during re-rendering
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.
Yeah using .get
works fine too. No preference on which we use but I am very confused as to why this fails now.
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.
Agreed it is weird this fails
Though ideally there would be a simple solution so we are not doing different thing for macOS, Linux & Windows
Maybe we can just use recipe-scripts-license.txt
# Copy feedstock license file to recipe directory | |
- pushd {{ RECIPE_DIR }} | |
- pushd .. | |
- cp LICENSE.txt {{ RECIPE_DIR }} | |
- popd | |
- popd | |
# Copy feedstock license file to recipe directory | |
- pushd {{ RECIPE_DIR }} | |
- cp recipe-scripts-license.txt LICENSE.txt | |
- popd |
Despite the issues here, uploads work fine with the new packages. So let's leave them in place and we can push new builds once someone who knows what is happening can figure out why compilers are not being activated in some jobs. cc @conda-forge/core |
This does the same thing. We are just now relying on conda-smithy to do it for us.
recipe/meta.yaml
Outdated
- {{ compiler('c') }} # [cuda_compiler_version != "None" or build_platform != target_platform] | ||
- {{ stdlib('c') }} # [cuda_compiler_version != "None" or build_platform != target_platform] | ||
- python # [build_platform != target_platform] | ||
- cross-python_{{ target_platform }} # [build_platform != target_platform] |
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.
Maybe we should only use cross-python
when the other compilers are used (so CUDA only)?
- {{ compiler('c') }} # [cuda_compiler_version != "None" or build_platform != target_platform] | |
- {{ stdlib('c') }} # [cuda_compiler_version != "None" or build_platform != target_platform] | |
- python # [build_platform != target_platform] | |
- cross-python_{{ target_platform }} # [build_platform != target_platform] | |
- {{ compiler('c') }} # [cuda_compiler_version != "None"] | |
- {{ stdlib('c') }} # [cuda_compiler_version != "None"] | |
- python # [cuda_compiler_version != "None" and build_platform != target_platform] | |
- cross-python_{{ target_platform }} # [cuda_compiler_version != "None" and build_platform != target_platform] |
- COPY "%RECIPE_DIR%\\..\\LICENSE.txt" "%RECIPE_DIR%\\LICENSE.txt" # [win] | ||
# Copy feedstock license file to recipe directory | ||
- pushd {{ RECIPE_DIR }} | ||
- cp recipe-scripts-license.txt LICENSE.txt |
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.
Doesn't this rely on an implementation detail in smithy? It is also very opaque since this file appears nowhere in the actual recipe.
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.
Am ok going back to the previous approach. Was just trying this to see if we could simplify
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.
yeah we should go back and add a .get
- python # [build_platform != target_platform] | ||
- cross-python_{{ target_platform }} # [build_platform != target_platform] | ||
- python # [cuda_compiler_version != "None" and build_platform != target_platform] | ||
- cross-python_{{ target_platform }} # [cuda_compiler_version != "None" and build_platform != target_platform] |
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 change is wrong. even when cuda compiler version is none we need the cross-python tools for cross-builds of the python package in this recipe.
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.
See the osx-arm64 build failures with this change for example.
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.
Yeah was just trying stuff. Agree this isn't working
Should we consider moving this to noarch
using virtual OS packages?
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.
We can but i'd like to understand why this is failing when it did not this morning.
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.
Also, do you know why we need all of the cuda variants?
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 put a clean Pr for the rerendering bug here: #350 (comment) |
My best guess is that it was this PR: conda-forge/ctng-compiler-activation-feedstock#133 |
I bet the new binutils packages with the activation scripts moved into them (so 2.43 build 1) are incompatible with the compiler packages that have the same output activation scripts (e.g., gcc 14.1.0 build < 4). |
I am trying to test this idea in #351 |
Seems this has been superseded by PRs:
Given this, should we close? |
Hi! This is the friendly automated conda-forge-webservice.
I've started rerendering the recipe as instructed in #348.
If I find any needed changes to the recipe, I'll push them to this PR shortly. Thank you for waiting!
Here's a checklist to do before merging.
Fixes #348