Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 4 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ lto = "yes"
anstream = "0.6"
anyhow = "1.0.82"
camino = "1.1.6"
canon-json = "0.2.1"
cap-std-ext = "4.0.3"
chrono = { version = "0.4.38", default-features = false }
clap = "4.5.4"
Expand Down
1 change: 1 addition & 0 deletions lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ uuid = { version = "1.8.0", features = ["v4"] }
tini = "1.3.0"
comfy-table = "7.1.1"
thiserror = { workspace = true }
canon-json = { workspace = true }

[dev-dependencies]
similar-asserts = { workspace = true }
Expand Down
6 changes: 3 additions & 3 deletions lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use anyhow::{anyhow, ensure, Context, Result};
use bootc_utils::CommandRunExt;
use camino::Utf8Path;
use camino::Utf8PathBuf;
use canon_json::CanonJsonSerialize;
use cap_std::fs::{Dir, MetadataExt};
use cap_std_ext::cap_std;
use cap_std_ext::cap_std::fs::FileType;
Expand Down Expand Up @@ -619,7 +620,7 @@ pub(crate) fn print_configuration() -> Result<()> {
let mut install_config = config::load_config()?.unwrap_or_default();
install_config.filter_to_external();
let stdout = std::io::stdout().lock();
serde_json::to_writer(stdout, &install_config).map_err(Into::into)
anyhow::Ok(install_config.to_canon_json_writer(stdout)?)
}

#[context("Creating ostree deployment")]
Expand Down Expand Up @@ -1349,8 +1350,7 @@ async fn install_with_sysroot(
rootfs
.physical_root
.atomic_replace_with(BOOTC_ALEPH_PATH, |f| {
serde_json::to_writer(f, &aleph)?;
anyhow::Ok(())
anyhow::Ok(aleph.to_canon_json_writer(f)?)
})
.context("Writing aleph version")?;

Expand Down
5 changes: 3 additions & 2 deletions lib/src/progress_jsonl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//! see <https://jsonlines.org/>.

use anyhow::Result;
use canon_json::CanonJsonSerialize;
use schemars::JsonSchema;
use serde::Serialize;
use std::borrow::Cow;
Expand Down Expand Up @@ -199,8 +200,8 @@ impl TryFrom<RawProgressFd> for ProgressWriter {
impl ProgressWriter {
/// Serialize the target value as a single line of JSON and write it.
async fn send_impl_inner<T: Serialize>(inner: &mut ProgressWriterInner, v: T) -> Result<()> {
// serde is guaranteed not to output newlines here
let buf = serde_json::to_vec(&v)?;
// canon_json is guaranteed not to output newlines here
let buf = v.to_canon_json_vec()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the progress stuff needs reproducibility, but I guess it doesn't hurt either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but ideally i would like to just ban serde_json::to_{vec,string,writer} with some linter (haven't looked how to do it yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgwalters ok to add those 3 functions to clippy disallowed-methods ? (in a follow up PR)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not opposed to it but not really excited by it either...it doesn't seem to me we will have that many places where we really need the reproducibility that we need to explicitly ban it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will leave it as is then

inner.fd.write_all(&buf).await?;
// We always end in a newline
inner.fd.write_all(b"\n").await?;
Expand Down
5 changes: 4 additions & 1 deletion lib/src/status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::io::Read;
use std::io::Write;

use anyhow::{Context, Result};
use canon_json::CanonJsonSerialize;
use fn_error_context::context;
use ostree::glib;
use ostree_container::OstreeImageReference;
Expand Down Expand Up @@ -320,7 +321,9 @@ pub(crate) async fn status(opts: super::cli::StatusOpts) -> Result<()> {
};
let format = opts.format.unwrap_or(legacy_opt);
match format {
OutputFormat::Json => serde_json::to_writer(&mut out, &host).map_err(anyhow::Error::new),
OutputFormat::Json => host
.to_canon_json_writer(&mut out)
.map_err(anyhow::Error::new),
OutputFormat::Yaml => serde_yaml::to_writer(&mut out, &host).map_err(anyhow::Error::new),
OutputFormat::HumanReadable => human_readable_output(&mut out, &host),
}
Expand Down
1 change: 1 addition & 0 deletions ostree-ext/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ indexmap = { version = "2.2.2", features = ["serde"] }
indoc = { version = "2", optional = true }
xshell = { version = "0.2", optional = true }
similar-asserts = { version = "1.5.0", optional = true }
canon-json = { workspace = true }

[dev-dependencies]
quickcheck = "1"
Expand Down
12 changes: 8 additions & 4 deletions ostree-ext/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use anyhow::{Context, Result};
use camino::{Utf8Path, Utf8PathBuf};
use canon_json::CanonJsonSerialize;
use cap_std::fs::Dir;
use cap_std_ext::cap_std;
use cap_std_ext::prelude::CapStdExtDirExt;
Expand Down Expand Up @@ -880,7 +881,9 @@ async fn container_store(
if let Some(check) = check.as_deref() {
let rootfs = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
rootfs.atomic_replace_with(check.as_str().trim_start_matches('/'), |w| {
serde_json::to_writer(w, &prep.manifest).context("Serializing manifest")
prep.manifest
.to_canon_json_writer(w)
.context("Serializing manifest")
})?;
// In check mode, we're done
return Ok(());
Expand Down Expand Up @@ -1010,7 +1013,8 @@ fn handle_serialize_to_file<T: serde::Serialize>(path: Option<&Utf8Path>, obj: T
let mut out = std::fs::File::create(path)
.map(BufWriter::new)
.with_context(|| anyhow::anyhow!("Opening {path} for writing"))?;
serde_json::to_writer(&mut out, &obj).context("Serializing output")?;
obj.to_canon_json_writer(&mut out)
.context("Serializing output")?;
}
Ok(())
}
Expand Down Expand Up @@ -1136,9 +1140,9 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
let stdout = std::io::stdout().lock();
let mut stdout = std::io::BufWriter::new(stdout);
if config {
serde_json::to_writer(&mut stdout, &image.configuration)?;
image.configuration.to_canon_json_writer(&mut stdout)?;
} else {
serde_json::to_writer(&mut stdout, &image.manifest)?;
image.manifest.to_canon_json_writer(&mut stdout)?;
}
stdout.flush()?;
Ok(())
Expand Down
21 changes: 15 additions & 6 deletions ostree-ext/src/container/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::sysroot::SysrootLock;
use crate::utils::ResultExt;
use anyhow::{anyhow, Context};
use camino::{Utf8Path, Utf8PathBuf};
use canon_json::CanonJsonSerialize;
use cap_std_ext::cap_std;
use cap_std_ext::cap_std::fs::{Dir, MetadataExt};
use cap_std_ext::cmdext::CapStdExtCommandExt;
Expand Down Expand Up @@ -603,9 +604,13 @@ impl ImageImporter {
Self::CACHED_KEY_MANIFEST_DIGEST,
manifest_digest.to_string(),
);
let cached_manifest = serde_json::to_string(manifest).context("Serializing manifest")?;
let cached_manifest = manifest
.to_canon_json_string()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Side note: this isn't a new problem, but we probably should have always been storing the original manifest instead of re-serializing.

.context("Serializing manifest")?;
commitmeta.insert(Self::CACHED_KEY_MANIFEST, cached_manifest);
let cached_config = serde_json::to_string(config).context("Serializing config")?;
let cached_config = config
.to_canon_json_string()
.context("Serializing config")?;
commitmeta.insert(Self::CACHED_KEY_CONFIG, cached_config);
let commitmeta = commitmeta.to_variant();
// Clone these to move into blocking method
Expand Down Expand Up @@ -1042,15 +1047,19 @@ impl ImageImporter {
let _ = self.layer_byte_progress.take();
let _ = self.layer_progress.take();

let serialized_manifest = serde_json::to_string(&import.manifest)?;
let serialized_config = serde_json::to_string(&import.config)?;
let mut metadata = HashMap::new();
metadata.insert(
META_MANIFEST_DIGEST,
import.manifest_digest.to_string().to_variant(),
);
metadata.insert(META_MANIFEST, serialized_manifest.to_variant());
metadata.insert(META_CONFIG, serialized_config.to_variant());
metadata.insert(
META_MANIFEST,
import.manifest.to_canon_json_string()?.to_variant(),
);
metadata.insert(
META_CONFIG,
import.config.to_canon_json_string()?.to_variant(),
);
metadata.insert(
"ostree.importer.version",
env!("CARGO_PKG_VERSION").to_variant(),
Expand Down