Skip to content

manager: fix two bugs in custom runtime selection #5922

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.

Sign up for GitHub

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

Merged
gz merged 1 commit into from
Mar 26, 2026
Merged

manager: fix two bugs in custom runtime selection #5922

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion crates/pipeline-manager/src/compiler/rust_compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,7 @@ async fn checkout_runtime_version(
"protocol.version=2",
Copy link
Copy Markdown

@mythical-fred mythical-fred Mar 26, 2026

Choose a reason for hiding this comment

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

Nit: commit subject says "fix bug" (singular) but the body describes two fixes. "fix two bugs" (matching the PR title) would be more accurate.

"fetch",
"--prune",
"--tags",
"https://github.com/feldera/feldera.git",
requested_runtime_version.as_commitish(),
])
Expand Down Expand Up @@ -1194,6 +1195,47 @@ async fn checkout_runtime_version(
}
}

/// We need to pass the actual revision SHA to the binary for verification,
/// so in case we have a version tag, we need to convert it to the sha.
pub async fn resolve_runtime_sha(
Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Mar 26, 2026

Choose a reason for hiding this comment

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

no tests?

Copy link
Copy Markdown
Contributor Author

@gz gz Mar 26, 2026

Choose a reason for hiding this comment

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

we test the feature already in CI every time we run something in the merge queue.. but we don't usually set tag names, just runtime SHAs so this bug fell through. It's not obvious how to test this part easily because it's not obvious a version tag in a test is compatible with the current runtime. This feature is also behind a feature flag.

runtime_version: &RuntimeSelector,
config: &CompilerConfig,
) -> Result<String, RustCompilationError> {
match runtime_version {
RuntimeSelector::Sha(sha) => Ok(sha.clone()),
RuntimeSelector::Platform(platform_sha) => Ok(platform_sha.clone()),
RuntimeSelector::Version(version) => {
let repo_location = runtime_version.runtime_sources(config);
match Command::new("git")
.current_dir(&repo_location)
.args([
"-c",
"protocol.version=2",
"rev-parse",
version,
])
.output()
.await
{
Ok(output) => {
if !output.status.success() {
return Err(RustCompilationError::SystemError(format!(
"Failed to determine commit SHA for '{runtime_version}'.\nGit command failed with exit code: {}\nstderr:\n{}\nstdout:\n{}",
output.status.code().unwrap_or(-1),
String::from_utf8_lossy(&output.stderr),
String::from_utf8_lossy(&output.stdout)
)));
}
Ok(String::from_utf8_lossy(&output.stdout).trim().to_string())
}
Err(e) => Err(RustCompilationError::SystemError(format!(
"Failed to determine commit SHA for '{runtime_version}', `git rev-parse` failed: {e}",
))),
}
}
}
}

/// Prepare the workspace for compilation of the specific pipeline.
/// This involves extracting the results of the prior SQL compilation and
/// configuring the workspace itself (e.g., its Cargo.toml and Cargo.lock).
Expand Down Expand Up @@ -1494,7 +1536,10 @@ async fn call_compiler(
command.env_clear();
command.env("PATH", env_path);
if !runtime_selector.is_platform() {
command.env("FELDERA_RUNTIME_OVERRIDE", runtime_selector.as_commitish());
command.env(
"FELDERA_RUNTIME_OVERRIDE",
resolve_runtime_sha(runtime_selector, config).await?,
);
}
if let Some(env_rustflags) = optional_env_rustflags {
command.env("RUSTFLAGS", env_rustflags);
Expand Down
Loading