Skip to content

Use start /machine in multi-output's script_interpreter - #6080

Open
jaimergp wants to merge 15 commits into
26.7.xfrom
machine-for-outputs
Open

Use start /machine in multi-output's script_interpreter#6080
jaimergp wants to merge 15 commits into
26.7.xfrom
machine-for-outputs

Conversation

@jaimergp

@jaimergp jaimergp commented Aug 5, 2026

Copy link
Copy Markdown
Member

Description

Follow up for #6047

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@github-project-automation github-project-automation Bot moved this to 🆕 New in 🔎 Review Aug 5, 2026
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Aug 5, 2026
@codspeed-hq

codspeed-hq Bot commented Aug 5, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 5 untouched benchmarks


Comparing machine-for-outputs (7c61cd8) with 26.7.x (656a81f)

Open in CodSpeed

@jaimergp
jaimergp changed the base branch from main to 26.7.x August 6, 2026 09:57
Comment thread conda_build/build.py Outdated
Comment on lines +1806 to +1816
if not dest_file.endswith((".sh", ".bat", ".ps1")):
# Check whether the interpreter comes from BUILD_PREFIX or not
executable = shutil.which(args[0])
if executable is not None and not executable.startswith(
metadata.config.build_prefix
):
log.warning(
"If the output script (%s) is not .sh, .bat or .ps1, "
"its script_interpreter field must be set to a tool installed in BUILD_PREFIX.",
output["script"],
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is to get closer to rattler-build's behavior. Just a warning for now.

@jaimergp
jaimergp marked this pull request as ready for review August 6, 2026 11:32
@jaimergp
jaimergp requested a review from a team as a code owner August 6, 2026 11:32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are unrelated changes from main/#6066

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, will rebase, sorry

Comment thread conda_build/windows.py Outdated
def wrap_script_with_machine(m, script: str | Path, pre_script: str = "") -> str:
from .build import INTERPRETER_BAT

# See docstring of _cmd_machine_flag()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is _cmd_machine_flag?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, meant _build_arch().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Probably worth bringing the docstring here though.

Comment thread conda_build/windows.py Outdated

# See docstring of _cmd_machine_flag()
script = Path(script)
wrapper = script.parent / (script.stem + ".wrapper" + script.suffix)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The input script could be .bat, .cmd, .ps1, etc. but the wrapper script is batch, shouldn't we just use .bat then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oof yes, excellent catch, thanks.

Comment thread conda_build/build.py
# See conda_build.windows._build_arch for more info.
args_to_run = [
*INTERPRETER_BAT,
wrap_script_with_machine(metadata, dest_file, " ".join(args)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if dest_file/script ends in .bat/.cmd wrap_script_with_machine will ignore args/pre_script, is that intended?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops I think I was missing an if not pre_script and bit there.

@kenodegard kenodegard mentioned this pull request Aug 6, 2026
13 tasks
@jaimergp
jaimergp force-pushed the machine-for-outputs branch from 5f1bce5 to ff42df2 Compare August 7, 2026 08:23
@jaimergp
jaimergp force-pushed the machine-for-outputs branch from ff42df2 to d6a5508 Compare August 7, 2026 08:25
@jaimergp
jaimergp requested a review from kenodegard August 11, 2026 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed [bot] added once the contributor has signed the CLA

Projects

Status: In review 🔍
Status: 🆕 New

Development

Successfully merging this pull request may close these issues.

5 participants