add no strip exposure - hit with archive package on older RHEL variants#418
Merged
add no strip exposure - hit with archive package on older RHEL variants#418
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a per-package configuration option to disable binary stripping during R CMD INSTALL, addressing install-time crashes (e.g., archive on older RHEL variants) while keeping the current default behavior unchanged.
Changes:
- Introduces
[project].no_stripin config and exposes it viaConfig::no_strip(). - Computes
stripper dependency inSyncHandlerand threads it through all install paths (repo/git/local/url). - Makes
R CMD INSTALLstripping flags/env conditional based on thestripboolean, and adds config parsing tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/config.rs | Adds no_strip to project config, accessor, and parsing/default tests. |
| src/sync/handler.rs | Determines whether to strip per package and passes the flag into source installers. |
| src/r_cmd.rs | Makes --strip/--strip-lib and _R_SHLIB_STRIP_ conditional on strip. |
| src/sync/sources/repositories.rs | Threads strip through repository-based installation. |
| src/sync/sources/git.rs | Threads strip through git/R-universe installation. |
| src/sync/sources/local.rs | Threads strip through local package installation. |
| src/sync/sources/url.rs | Threads strip through URL-based installation. |
Comments suppressed due to low confidence (4)
src/config.rs:600
- This test parses via
toml::from_str, which skipsConfig::finalize()(run byConfig::from_str/from_file). Consider switching toConfig::from_str(toml_str)so the test exercises the actual config-loading path.
fn no_strip_defaults_to_empty() {
let toml_str = r#"
[project]
name = "test"
r_version = "4.4"
repositories = []
"#;
let config: Config = toml::from_str(toml_str).unwrap();
assert!(config.no_strip().is_empty());
src/config.rs:613
config_r_version_round_trips_as_stringusestoml::from_strdirectly; to ensure round-trips reflect the real config entrypoint (includingfinalize()), consider constructing the initial config withConfig::from_strinstead.
fn config_r_version_round_trips_as_string() {
let toml_str = r#"
[project]
name = "test"
r_version = "4.4"
repositories = []
"#;
let config: Config = toml::from_str(toml_str).unwrap();
let serialized = toml::to_string(&config).unwrap();
let deserialized: Config = toml::from_str(&serialized).unwrap();
src/sync/handler.rs:271
- The rustdoc link syntax
[project.no_strip]doesn’t resolve to an item here and can produce broken intra-doc link warnings. Consider using backticks (project.no_strip) or linking to an actual Rust item (e.g.,Config::no_strip).
/// Check whether stripping should be applied for a package.
/// Returns false if the package is listed in [project.no_strip].
fn should_strip(&self, package_name: &str) -> bool {
src/config.rs:588
- These config parsing tests call
toml::from_strdirectly, which bypassesConfig::from_str/Config::finalize()validation. UsingConfig::from_str(toml_str)would better reflect real config-loading behavior and catch future validation changes.
fn can_parse_no_strip() {
let toml_str = r#"
[project]
name = "test"
r_version = "4.4"
repositories = []
no_strip = ["rgl", "sf"]
"#;
let config: Config = toml::from_str(toml_str).unwrap();
assert_eq!(config.no_strip(), &["rgl", "sf"]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Keats
approved these changes
Feb 20, 2026
| use crate::sync::errors::SyncError; | ||
| use crate::{Cancellation, CommandExecutor, RCmd, ResolvedDependency}; | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
Collaborator
There was a problem hiding this comment.
We should create a struct for some of those args, it's getting a bit crazy
Member
Author
|
@Keats - here is some investigation - what do you think about the options/recommendation #419 one thing is we need to get this out somewhat ASAP because we've found any dep tree that needs archive + installs from src to fail on RHEL and variants. So if this needs a good bit more thought on your end, I'd say we move forward with just getting this merged as is, then do a more significant refactor at whatever pace needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
current behavior for archive package:
given:
we see the error:
after adding this: