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

fix: allow for feedstock_root to not be set #350

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

beckermr
Copy link
Member

This PR fixes the rerendering bug where FEEDSTOCK_ROOT is not set and smithy chokes.

@beckermr beckermr requested a review from a team as a code owner September 26, 2024 00:18
Copy link
Contributor

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/meta.yaml) and found it was in an excellent condition.

@beckermr
Copy link
Member Author

@conda-forge-admin rerender

@beckermr beckermr mentioned this pull request Sep 26, 2024
1 task
@leofang
Copy link
Member

leofang commented Sep 26, 2024

Thanks, Matt! We've been seeing weird rerendering errors since this morning conda-forge/openmpi-feedstock#175 (comment), I assume it's the same issue?

@beckermr
Copy link
Member Author

yes it appears to be the same issue.

@jakirkham
Copy link
Member

jakirkham commented Sep 26, 2024

Well there are two errors we are seeing:

  1. FEEDSTOCK_ROOT in this recipe causes re-rendering issues: MNT: rerender #349 (comment)
  2. CONDA_BUILD_SYSROOT not being set, which causes various odd build time issues: MNT: rerender #349 (comment)

Matt is working around the first issue for this feedstock specifically

We are still investigating what causes either error (or how to fix them)

@jakirkham
Copy link
Member

Ok think I understand 1. It is due to a conda-build change in 24.7.0. Namely this one: conda/conda-build#5371

Was able to re-render with conda-build 24.5 as show in PR: #352

Think we need to add some handling for re-rendering like this PR to address it

At least the cause of the first error is now clearer

recipe/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
recipe/meta.yaml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

(just to reconfirm it works)

@jakirkham
Copy link
Member

Looks like re-rendering simply renamed variant files. So nothing really changed

Main thing is we are able to re-render again

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Matt! 🙏

LGTM. Should we go ahead and merge?

Note: Some jobs will still fail until issue ( conda-forge/binutils-feedstock#78 ) is resolved

@jakirkham jakirkham mentioned this pull request Sep 26, 2024
5 tasks
@beckermr
Copy link
Member Author

Let's wait to merge and see if we can release a bugfix all at once for both issues.

@beckermr
Copy link
Member Author

@conda-forge-admin restart ci

@beckermr
Copy link
Member Author

@conda-forge-admin rerender

@leofang
Copy link
Member

leofang commented Sep 26, 2024

Nice, what change fixed the rerender error?

@beckermr
Copy link
Member Author

We need to define some vars at the top if they are undefined.

@beckermr beckermr added the automerge Merge the PR when CI passes label Sep 26, 2024
Comment on lines +24 to +29
{% if "FEEDSTOCK_ROOT" in os.environ %} # [linux]
- cp {{ os.environ["FEEDSTOCK_ROOT"] }}/LICENSE.txt ${RECIPE_DIR}/LICENSE.txt # [linux]
{% else %} # [linux]
- echo '${FEEDSTOCK_ROOT} is undefined. Cannot copy license file' # [linux]
- exit 1 # [linux]
{% endif %} # [linux]
Copy link
Member

Choose a reason for hiding this comment

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

For context this is the guard Matt is referring to

Note this fixes the issue in this recipe. While other recipes can implement similar fixes, the change here alone doesn't fix other recipes

@github-actions github-actions bot merged commit 1eb323b into conda-forge:main Sep 26, 2024
42 checks passed
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants