-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
Unify Windows batch logic #1761
Conversation
@@ -941,7 +941,7 @@ def _get_build_setup_line(forge_dir, platform, forge_config): | |||
if platform == "win": | |||
build_setup += textwrap.dedent( | |||
"""\ | |||
run_conda_forge_build_setup | |||
CALL run_conda_forge_build_setup |
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.
Without using CALL
, this batch script coming from conda-forge-ci-setup
might (will?) exit early with errorlevel = 0, misreporting success without even running the recipe.
@conda-forge/core - ready for review! |
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 know nothing about windows but it it works, LGTM!
Checklist
news
entryWe had duplicate logic for Azure and Github Actions, which was resulting in duplicated efforts in PRs like #1732, and divergence of features/bugfixes (GHA didn't have
python=3.10
,UPLOAD_ON_BRANCH
or the%TEMP%
fixes foranaconda upload ...
).In this PR I am hoping to unify both in a single script, which should also get us closer to having
build-locally.py
features for Windows. The downside is that the Azure build steps will be now a single one, which is not terrible and can be worked with logging groups like we do in Unix if needed.One thing to pay special attention to is the error handling on CMD. It can be terribly confusing depending on too many edge cases (see #1704, I will probably restart work on that after this one lands).
Tests:
main
dav1d-feedstock#10