Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
8 changes: 5 additions & 3 deletions docs/src/design/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -556,9 +556,11 @@ The active view is a scoping and discovery mechanism, not a security boundary.
**Alias conflicts are scoped to the view.** In v1, `Channel::get_aliases` would hard-error on any duplicate alias across the whole channel. Under a superset that accretes components from multiple projects, two components that no project ever activates together could collide and break *every* command. Therefore: a conflict *within the active view* is an error; a conflict that exists only in
the superset is a warning, and the component in the active view wins.

### 8.6 Partial status is derived
### 8.6 Update status is derived

Local state does not record a "partial" flag. When the upstream manifest is available, an installation is displayed as partial if its installed component set is a proper subset of the channel's complete set. When upstream is unavailable, partial status is not displayed.
Local state does not record an "update available" flag. When the upstream manifest is available, an installation is displayed as having an update if the manifest's content changed for it.

The comparison is manifest content alone. The pins install records on an authority — a branch's latest commit, a path's modification time — exist only locally and move on their own, so they are excluded: drift behind a pin is reconciled by `update` itself, which is what pinning is for, and a listing performs no source or network I/O beyond the manifest fetch.

Components with no physical output (`command` with zero artifacts) count as installed for membership purposes and are exempt from physical verification (§9.6).

Expand All @@ -569,7 +571,7 @@ upstream is unavailable the annotation is omitted entirely rather than derived l
guess would be exactly the derivation networks exist to eliminate, and a stale one would tell a user
they are on mainnet when they are not. The other markers are unchanged: `(needs reinstallation)` is
derived from local state alone - a migrated record with no publication - and is always shown, while
`(partially installed)` and `(unavailable upstream)` need upstream and are omitted with it.
`(update available)` and `(unavailable upstream)` need upstream and are omitted with it.

---

Expand Down
21 changes: 15 additions & 6 deletions src/commands/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,24 @@ pub fn list(config: &Config, state: &LocalState) -> anyhow::Result<()> {
.map(|channel| {
let channel_name = &channel.name;

// Partial status is *derived*, never recorded (spec section 8.6): an installation is
// partial exactly when it holds fewer components than the channel offers. A stored
// flag would be a second answer to a question the component set already answers, and
// the two would drift.
// Update status is *derived*, never recorded (spec section 8.6): an installation has
// an update exactly when re-resolving its recorded intent against upstream would
// change it. A stored flag would be a second answer to a question the manifest and
// the component set already answer, and the two would drift.
let installed_indicator = match state.get(&channel.name) {
Some(installation) if installation.as_channel().is_partially_installed(channel) => {
format!(" {}", "(partially installed)".yellow())
Some(installation) if super::update::needs_update(installation, channel) => {
format!(" {}", "(update available)".yellow())
},
Some(_) => format!(" {}", "(installed)".green()),
// A channel that supersedes an installed one (spec section 11.4): updating the
// predecessor migrates to it.
None if channel
.migrates_from
.as_ref()
.is_some_and(|old| state.get(old).is_some()) =>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Honor same-version precedence when displaying migration updates

Checking only whether the predecessor is installed does not match the updater's counterpart-selection rule. If upstream contains both installed 0.15.0 and an uninstalled 0.16.0 declaring migrates_from: "0.15.0", this branch prints 0.16.0 (update available). However, upstream_counterpart_raw prefers the still-published 0.15.0, so update 0.15.0 reports Toolchain 0.15.0 is up to date and performs no migration; show list also reports no update. This was reproduced against this PR's head, and the false marker remains after updating. Please use the same counterpart-selection rule as the updater when deciding whether a successor represents an available update.

{
format!(" {}", "(update available)".yellow())
},
None => String::new(),
};

Expand Down
80 changes: 46 additions & 34 deletions src/commands/show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,24 @@ use colored::Colorize;

use super::Flags;
use crate::{
channel::UpstreamMatch,
config::Config,
report,
state::LocalState,
toolchain::{Toolchain, ToolchainJustification},
};

/// Appends a status marker to a listing row: yellow as one span when color is on, plain otherwise.
fn push_marker(line: &mut String, text: &str, use_color: bool) {
use core::fmt::Write;

if use_color {
write!(line, " {}", text.yellow()).unwrap();
} else {
write!(line, " {text}").unwrap();
}
}

#[derive(Debug, Subcommand)]
pub enum ShowCommand {
/// Show the active toolchain.
Expand Down Expand Up @@ -65,8 +77,8 @@ impl ShowCommand {
Self::List { .. } => {
// Installed toolchains are recorded locally, so this works with no network at all.
// Upstream only adds *markers* -- which networks name a channel, which
// installations are partial or no longer published -- so when it is unavailable
// they are simply omitted rather than guessed at.
// installations have updates or are no longer published -- so when it is
// unavailable they are simply omitted rather than guessed at.
let upstream = config.upstream_manifest().ok();
// The upstream lookup may have emitted a report to stderr, so restore stdout's
// color policy immediately before rendering the result.
Expand Down Expand Up @@ -112,17 +124,12 @@ impl ShowCommand {
if linked != name {
continue;
}
if let Some(marker) = crate::networks::drift(
if let Some(notice) = crate::networks::drift(
network,
linked,
manifest.network_version(network),
) {
if use_color {
write!(&mut line, " {}", marker.yellow()).unwrap();
} else {
line.push(' ');
line.push_str(&marker);
}
push_marker(&mut line, &notice, use_color);
}
}
}
Expand All @@ -132,37 +139,42 @@ impl ShowCommand {
// point: the user's toolchain still works, but only after it is installed
// properly, and they should not have to infer that from a failure.
if !installation.is_managed() {
if use_color {
write!(&mut line, " {}", "(needs reinstallation)".yellow())
.unwrap();
} else {
line.push_str(" (needs reinstallation)");
}
write!(&mut line, " -- run `midenup install {name}`").unwrap();
push_marker(
&mut line,
&format!("(needs reinstallation) -- run `midenup install {name}`"),
use_color,
);
}

if let Some(manifest) = upstream {
match manifest.get_channel_by_name(name) {
Some(channel)
if installation
.as_channel()
.is_partially_installed(channel) =>
{
if use_color {
write!(&mut line, " {}", "(partially installed)".yellow())
.unwrap();
} else {
line.push_str(" (partially installed)");
if upstream.is_some() {
// The lookup follows migration lineage the way `update` does, so a
// superseded channel shows as updatable rather than gone. Verbatim
// manifest content: a listing pins nothing and reaches for no source.
match installation.as_channel().upstream_counterpart_raw(config) {
// An unmanaged record already carries its one instruction, the
// reinstall above; a second directive would contradict it.
Some(_) if !installation.is_managed() => {},
Some((channel, upstream_match)) => {
let has_update = match upstream_match {
// Being superseded is the update: running it migrates.
UpstreamMatch::Migrated { .. } => true,
UpstreamMatch::UpstreamCounterpart => {
super::update::needs_update(installation, channel)
},
};
if has_update {
push_marker(
&mut line,
&format!(
"(update available) -- run `midenup update {name}`"
),
use_color,
);
}
},
Some(_) => {},
// Retained, not deleted: the user may still want `var/` and an
// explicit uninstall (spec section 12.3).
None if use_color => {
write!(&mut line, " {}", "(unavailable upstream)".yellow())
.unwrap()
},
None => line.push_str(" (unavailable upstream)"),
None => push_marker(&mut line, "(unavailable upstream)", use_color),
}
}

Expand Down
103 changes: 102 additions & 1 deletion src/commands/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
manifest::Component,
options::{InstallationOptions, IntentUpdate, PathUpdate, UpdateOptions},
state::{Installation, LocalState},
version::Authority,
version::{Authority, GitTarget},
};

/// Updates installed toolchains.
Expand Down Expand Up @@ -409,6 +409,56 @@ fn work_for(
Ok(Work::Nothing)
}

/// Whether the manifest's content changed for this installation.
pub fn needs_update(installation: &Installation, upstream: &Channel) -> bool {
match crate::resolve::resolve(upstream, &installation.intent) {
Ok(resolved) => {
let installed_names: std::collections::BTreeSet<&str> = installation
.components
.iter()
.map(|component| component.name.as_ref())
.collect();
let resolved_names: std::collections::BTreeSet<&str> =
resolved.iter().map(|component| component.name.as_ref()).collect();
if installed_names != resolved_names {
return true;
}
},
Err(_) => return true,
}

installation.components.iter().any(|installed| {
upstream
.get_component(&installed.name)
.is_some_and(|new| definition_changed(installed, new))
})
}

/// Whether two definitions of one component differ as manifest content.
///
/// The pins install records on an authority -- a branch's commit, a path's modification time --
/// exist only locally and move on their own, so they are normalized away: drift behind them is
/// reconciled by the update itself, which is what pins for ([`classify`]); a listing must not.
fn definition_changed(installed: &Component, upstream: &Component) -> bool {
let normalized = |component: &Component| {
let mut component = component.clone();
match &mut component.version {
Authority::Path { last_modification, .. } => *last_modification = None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Normalize relative path authorities before comparing definitions

Installation rewrites a relative Authority::Path.path using config.working_directory.join(path) in refresh_path_modification_times, but this comparison only removes last_modification. Consequently, an unchanged manifest with "version": {"kind": "path", "path": "src"} shows (update available) in both list and show list immediately after a successful install: the stored absolute path differs from the raw relative path. Running update 0.15.0 --path-update=all succeeds but never clears the marker, because metadata updates preserve the installed authority. This was reproduced against this PR's head. Please account for the install-time path transformation when comparing definitions, while preserving the requirement that listing does not access component sources.

Authority::Git {
target: GitTarget::Branch { latest_revision, .. },
..
} => *latest_revision = None,
Authority::Git { .. } | Authority::Registry { .. } => (),
}
serde_json::to_value(component).ok()
};

match (normalized(installed), normalized(upstream)) {
(Some(old), Some(new)) => old != new,
_ => true,
}
}

/// Commits selection and metadata changes that no installed file reflects.
///
/// Each component's recorded *authority* is preserved rather than taken from upstream. Reaching
Expand Down Expand Up @@ -672,6 +722,57 @@ mod tests {
assert_eq!(classify_pair(base(), base()), ChangeClass::None);
}

/// The pins install records -- a branch's commit, a path's modification time -- exist only
/// locally and move on their own, so a listing must not read them as a manifest change.
#[test]
fn an_authority_pin_is_not_a_definition_change() {
let on_branch = |latest_revision: Option<&str>| {
let mut component = base();
component.version = Authority::Git {
repository_url: "https://example.invalid/repo".to_string(),
subpath: None,
target: GitTarget::Branch {
name: "main".to_string(),
latest_revision: latest_revision.map(str::to_string),
},
};
component
};
assert!(!definition_changed(&on_branch(Some("abc123")), &on_branch(None)));

let at_path = |last_modification: Option<std::time::SystemTime>| {
let mut component = base();
component.version = Authority::Path { path: "vm".into(), last_modification };
component
};
let pinned = at_path(Some(std::time::SystemTime::UNIX_EPOCH));
assert!(!definition_changed(&pinned, &at_path(None)));
}

/// Normalizing the pin must not mask a real change riding alongside it.
#[test]
fn a_change_next_to_a_pin_is_still_a_definition_change() {
let mut installed = base();
installed.version = Authority::Git {
repository_url: "https://example.invalid/repo".to_string(),
subpath: None,
target: GitTarget::Branch {
name: "main".to_string(),
latest_revision: Some("abc123".to_string()),
},
};
let mut upstream = base();
upstream.version = Authority::Git {
repository_url: "https://example.invalid/repo".to_string(),
subpath: None,
target: GitTarget::Branch {
name: "next".to_string(),
latest_revision: None,
},
};
assert!(definition_changed(&installed, &upstream));
}

/// An artifact is a file in the publication, so a component whose artifact URI moves to a new
/// release must be reinstalled even though nothing else about it changed.
#[test]
Expand Down
30 changes: 12 additions & 18 deletions src/manifest/v3/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,6 @@ impl Channel {
self.components.iter_mut().find(|c| c.name == name)
}

/// Whether this channel holds fewer components than the `upstream` one it is compared against.
///
/// Derived, never recorded (spec section 8.6): a stored "partial" flag would be a second answer
/// to a question the component set already answers, and two answers can disagree.
pub fn is_partially_installed(&self, upstream: &Channel) -> bool {
self.components.len() < upstream.components.len()
}

pub fn get_channel_dir(&self, config: &Config) -> PathBuf {
let installed_toolchains_dir = config.midenup_home.join("toolchains");
installed_toolchains_dir.join(format!("{}", self.name))
Expand Down Expand Up @@ -166,27 +158,29 @@ impl Channel {
/// channel>`. A same-version match wins: a channel that still exists upstream is not migrated
/// away from, whatever some other channel claims to supersede.
pub fn find_upstream_counterpart(&self, config: &Config) -> Option<UpstreamChannel> {
let (channel, upstream_match) = self.upstream_counterpart_raw(config)?;
Some(UpstreamChannel::new(channel.clone(), upstream_match, config))
}

/// As [`Self::find_upstream_counterpart`], but returning the manifest's definitions verbatim:
/// no sync, so no source or network I/O beyond the manifest itself.
pub fn upstream_counterpart_raw<'a>(
&self,
config: &'a Config,
) -> Option<(&'a Channel, UpstreamMatch)> {
let upstream_manifest = config.upstream_manifest().ok()?;

if let Some(same_version) =
upstream_manifest.get_channels().find(|upstream| upstream.name == self.name)
{
return Some(UpstreamChannel::new(
same_version.clone(),
UpstreamMatch::UpstreamCounterpart,
config,
));
return Some((same_version, UpstreamMatch::UpstreamCounterpart));
}

let successor = upstream_manifest
.get_channels()
.find(|upstream| upstream.migrates_from.as_ref() == Some(&self.name))?;

Some(UpstreamChannel::new(
successor.clone(),
UpstreamMatch::Migrated { old_channel: self.name.clone() },
config,
))
Some((successor, UpstreamMatch::Migrated { old_channel: self.name.clone() }))
}

// Syncs the channel to the latest changes
Expand Down
Loading
Loading