Skip to content
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.

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

Migrate to config 1.0 #1533

Open
mistydemeo opened this issue Nov 7, 2024 · 4 comments
Open

Migrate to config 1.0 #1533

mistydemeo opened this issue Nov 7, 2024 · 4 comments
Milestone

Comments

@mistydemeo
Copy link
Contributor

Internally, we've migrated to the planned 1.0 config format - we parse the original format, then do a conversion to the new structure. The new structure is set up with serialization and deserialization routes that makes it theoretically possible to read the new files - but nothing is set up to actually do so. Our final steps to complete this migration will be:

  • Introduce parallel deserialization of both config formats. We need to keep being able to read the old format to report to users that they need to upgrade, and so that we can:
  • Use dist init to read the old config format and migrate to the new one, and then use that new format as the basis for edits we make to the user's config files on their behalf. This should allow us to painlessly migrate everyone to the new format.

My plan is that when we introduce this, any commands except init will refuse to run if they're loaded from the old config files, but can use that as information to notify users that they need to upgrade via dist init.

@ashleygwilliams ashleygwilliams added this to the 0.27.0 milestone Nov 7, 2024
@mistydemeo
Copy link
Contributor Author

I did a quick look at the TomlLayer to confirm, and we are definitely set for serializing and deserializing it from TOML files. Here's a sample of taking dist's own existing v0 config, passing it through the TomlLayer upconversion carousel, and then serializing it to disk.

dist-version = "0.24.1"
targets = [
    "aarch64-apple-darwin",
    "aarch64-unknown-linux-gnu",
    "aarch64-unknown-linux-musl",
    "x86_64-apple-darwin",
    "x86_64-unknown-linux-gnu",
    "x86_64-unknown-linux-musl",
    "x86_64-pc-windows-msvc",
]

[[artifacts.extra]]
build = [
    "cargo",
    "run",
    "--release",
    "--",
    "dist",
    "manifest-schema",
    "--output=dist-manifest-schema.json",
]
artifacts = ["dist-manifest-schema.json"]

[ci]
pr-run-mode = "plan"
publish-jobs = ["./publish-crates"]

[ci.github.runners]
aarch64-unknown-linux-gnu = "buildjet-8vcpu-ubuntu-2204-arm"
aarch64-unknown-linux-musl = "buildjet-8vcpu-ubuntu-2204-arm"

[hosts]
axodotdev = true

[hosts.github]
attestations = true

[installers]
install-path = "CARGO_HOME"
powershell = true
shell = true
updater = false

[installers.bin-aliases]
dist = ["cargo-dist"]

[installers.homebrew]
tap = "axodotdev/homebrew-tap"

[installers.npm]
scope = "@axodotdev"

[publishers]
homebrew = true
npm = true

@mistydemeo
Copy link
Contributor Author

Note: the above generated config is missing the top-level dist key. The TomlLayer struct has the values directly inside itself, so when we deserialize from disk we'll want to add one extra layer so it looks more like this:

[dist]
dist-version = "0.24.1"
targets = [
    "aarch64-apple-darwin",
    "aarch64-unknown-linux-gnu",
    "aarch64-unknown-linux-musl",
    "x86_64-apple-darwin",
    "x86_64-unknown-linux-gnu",
    "x86_64-unknown-linux-musl",
    "x86_64-pc-windows-msvc",
]

[[dist.artifacts.extra]]
# ...

Currently, the TomlLayer is produced as an intermediate in between the old and new configs. That's convenient for us, since it makes it possible to manage this conversion conveniently. We currently call it like so:

  1. In DistGraphBuilder, after the workspace has located, we call config::parse_metadata_table_or_manifest with the workspace data: https://github.com/axodotdev/cargo-dist/blob/6a08ed8d505a56a93503912714c584ed4bb72789/cargo-dist/src/tasks.rs#L944-950
  2. The implementation of parse_metadata_table_or_manifest then does one of two things, depending on the arguments it's passed. If the passed dist_manifest_path is Some, it tries to parse that file as a dist-manifest.toml. Otherwise, it tries to fetch metadata from the table inside the Cargo.toml, which axoproject pre-parsed and passed along. https://github.com/axodotdev/cargo-dist/blob/6a08ed8d505a56a93503912714c584ed4bb72789/cargo-dist/src/config/mod.rs#L937-945 Regardless of which path it follows, it returns a DistMetadata.
  3. Immediately after assigning that, we call to_toml_layer on that returned DistMetadata, which transforms it into a TomlLayer.
    let workspace_layer = workspace_metadata.to_toml_layer(true);
  4. We then call config::v1::workspace_config with the set of workspaces and the TomlLayer to some additional workspace config.
    let config = workspace_config(workspaces, workspace_layer.clone());
  5. Finally, starting on tasks.rs L963, we merge in package config to produce a single final set of config.
    // Compute/merge package configs
  6. Each package metadata is also parsed via config::parse_metadata_table_or_manifest and then passed through a DistMetadata::to_toml_layer conversion. https://github.com/axodotdev/cargo-dist/blob/6a08ed8d505a56a93503912714c584ed4bb72789/cargo-dist/src/tasks.rs#L968-978

Steps 1 and 2 are the steps that are specific to the old config format. Step 3 ensures that we're now in config 1.0 land. Any branching we do just needs to be in the form of obtaining a TomlLayer, allowing us to move forward with a format that's compatible with the rest of the existing code. The same is true for package config in steps 5-6.

Meanwhile, over in init land, we skip the standard parsing code entirely. Instead, we parse it using toml_edit and directly edit the file as TOML using that API.

let mut workspace_toml = config::load_cargo_toml(&workspace.manifest_path)?;

This means that to handle the config 1.0 conversion in init, we'll want to do one of two things:

  1. First parse the config using config::parse_metadata_table_or_manifest, convert it to a TomlLayer, and serialize that back to disk before then opening the file with toml_edit.
  2. Open it with toml_edit and manually reorder all of the fields. This would require reimplementing all associated code, but would allow us to keep any user-authored comments that exist.

@duckinator
Copy link
Contributor

duckinator commented Dec 6, 2024

talked with Ashley a bit, and my current rough plan is:

  • ensure there are tests for the existing dist init migration functionality (the snaps seem to handle this)
  • split dist init migration functionality out into dist migrate, and make dist init reuse it (Refactor dist init and split out dist migrate #1611)
  • extend dist migrate to handle v0->v1 + have dist use config 1.0

@duckinator
Copy link
Contributor

  1. make dist migrate move v0 to v1
  2. make dist init use v1
  3. make everything but dist init load v1
  4. make sure dist plan gives an actionable error for v0 configs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants