[WIP] Implement config v0 -> v1 migration. #1653

duckinator · 2024-12-18T20:56:16Z

Required functionality for this PR:

make dist migrate move v0 to v1
- Have do_migrate_from_v0() actually copy workspace members to the new configuration.
make dist init use v1
make everything but dist init load v1
make sure dist plan (and other commands) gives an actionable error for v0 configs

Current state (2025-01-10):

duckinator · 2024-12-20T17:15:39Z

This is currently based off #1661. When that gets merged, I'll rebase off main.

duckinator · 2025-01-07T20:58:08Z

The various apply_*() functions should compile now, and some of it will actually be updated properly.

TODO:

get_new_dist_metadata()
finish up the apply_*() functions

mistydemeo · 2025-01-07T22:40:19Z

We were discussing get_new_dist_metadata during the call. I'll summarize what it does.

This is the interactive portion of init. It's used in two code paths: a newly-initted system, in which case we have no existing config, and updating existing configuration. If there's existing config, the interactive portions are designed to prefill the choice with the decisions that had already been made the last time; this ensures that mashing enter through the settings preserves the user's existing config without changing anything.

Unlike most of the rest of init, which operates on toml_edit documents, get_new_dist_metadata operates on a DistMetadata struct. It parses the existing config to DistMetadata, edits it in place, and returns the altered document. If there's no existing configuration, it instead initializes a completely empty DistMetadata.

Because this code is working from and generating a DistMetadata, we might not actually have to do any changes at all for this branch beyond swapping to the v1 version of DistMetadata.

For each bit of user-selectable config, we use the dialoguer crate to prompt the user for their input unless the user passed the --yes/-y flag, in which case we pick defaults/their existing config without prompting.

…onfigs.

…rship errors instead of type errors

…space

…gs." This reverts commit 46dc61d.

…n DistGraphBuilder

It's perfectly valid for package config to be missing; rather than an error case, this just indicates that there's no package-specific config. The previous rendition of the config code handled this by initializing the config to a default value if not specified.

…ommonInstallerLayer.

mistydemeo

Left a comment on the as-yet incomplete items in init.rs, just to track them, along with a couple of things to keep an eye on testing.

Aside from the remaining init.rs changes, I think the only other remaining work is in the tests. All of our tests are written in the old config format, and are specifically using Cargo.toml, so they need to be updated to use dist-workspace.toml in the new config format in order to pass.

mistydemeo · 2025-01-10T23:39:01Z

cargo-dist/src/init.rs

        // FIXME: when there is more than one option this should be a proper
        // multiselect like the installer selector is! For now we do
        // most of the multi-select logic and then just give a prompt.
-        let known = &[CiStyle::Github];
+        /*let known = &[CiStyle::Github];


This whole section with the complex "known styles" logic either needs to be uncommented out or deleted. Since we currently don't have any non-GitHub CI options, deletion seems reasonable at this point.

mistydemeo · 2025-01-10T23:45:00Z

cargo-dist/src/init.rs

    // Enable installer backends (if they have a CI backend that can provide URLs)
    // FIXME: "vendored" installers like msi could be enabled without any CI...
-    let has_ci = meta.ci.as_ref().map(|ci| !ci.is_empty()).unwrap_or(false);
+    //let has_ci = meta.ci.as_ref().map(|ci| !ci.is_empty()).unwrap_or(false);


Need to clear up whether this commented-out version can simply be deleted or not, given the version below.

mistydemeo · 2025-01-10T23:46:29Z

cargo-dist/src/init.rs

@@ -733,37 +773,40 @@ fn get_new_dist_metadata(
                InstallerStyle::Npm,
                InstallerStyle::Homebrew,
                InstallerStyle::Msi,
+                // Pkg intentionally left out because it's currently opt-in only.


Due to be added in #1381, but it needs to be rewritten in the style of this new code.

mistydemeo · 2025-01-10T23:52:05Z

cargo-dist/src/init.rs

    Ok(meta)
 }

 /// Update a workspace toml-edit document with the current DistMetadata value
 pub(crate) fn apply_dist_to_workspace_toml(
    workspace_toml: &mut toml_edit::DocumentMut,
-    workspace_kind: WorkspaceKind,
-    meta: &DistMetadata,
+    _workspace_kind: WorkspaceKind,


We can probably remove this param entirely now!

mistydemeo · 2025-01-10T23:53:00Z

cargo-dist/src/init.rs

    } = &meta;

+    /*


This whole section needs to be uncommented and reimplemented for the new structure.

mistydemeo · 2025-01-11T00:20:46Z

cargo-dist/src/init.rs

+
+    apply_installers_common(npm_table, &npm.common);
+
+    // TODO: similar to shell


Impl missing here.

mistydemeo · 2025-01-11T00:21:22Z

cargo-dist/src/init.rs

+
+    apply_installers_common(powershell_table, &powershell.common);
+
+    // TODO: similar to shell


Impl missing here.

mistydemeo · 2025-01-11T00:21:44Z

cargo-dist/src/init.rs

+
+    apply_installers_common(shell_table, &shell.common);
+
+    // TODO


Impl missing here.

mistydemeo · 2025-01-11T00:22:38Z

cargo-dist/src/init.rs

+
+    apply_optional_value(
+        pkg_table,
+        "install_location",


Typo: this one is still kebab-case.

Suggested change

"install_location",

"install-location",

mistydemeo · 2025-01-11T00:23:56Z

cargo-dist/src/init.rs

+        panic!("Expected [dist.publishers] to be a table");
+    };
+
+    // ...


Impl missing here.

duckinator force-pushed the dist-migrate-v1 branch 4 times, most recently from ce26547 to 9a7d939 Compare December 20, 2024 17:15

duckinator force-pushed the dist-migrate-v1 branch from 9a7d939 to 8670bcb Compare December 20, 2024 17:54

duckinator force-pushed the dist-migrate-v1 branch from bd40eca to 101bc69 Compare January 6, 2025 21:12

duckinator added 4 commits January 8, 2025 11:02

Have "dist init" only migrate from Cargo.toml configs to v0 generic c…

953b2db

…onfigs.

Make "dist migrate" convert from v0 to v1 config.

e1576c3

[WIP, BROKEN] start making "dist init" use v1 config

0b7f6d8

[WIP, BROKEN] apply_*() at least seem like they'll compile now

9f39ac6

duckinator force-pushed the dist-migrate-v1 branch from 4f99c57 to 9f39ac6 Compare January 8, 2025 16:02

duckinator and others added 16 commits January 8, 2025 14:48

Add BoolOr::truthy() to simplify logic elsewhere.

1d9f415

derive Default for InstallerLayer.

0999c3a

Migrate first chunk of get_new_dist_metadata() to v1 config

f9527bf

derive Default for PublisherLayer.

b0ff20a

made enough progress on get_new_dist_metadata() that i'm getting owne…

942ec4e

…rship errors instead of type errors

[formatting cleanup] clean up /* and */ indentation levels, and white…

05867a5

…space

formatting cleanup

4cf359e

initial stab at cleaning up ownership etc

441902c

Have "dist init" run do_migrate() if using a v0 config.

3434474

Have "dist generate" print an error if you have a v0 config.

c376686

[WIP] attempt to migrate "dist generate" to v1 config

25eab8c

have is_v0_config() return false when trying to load v1 configs.

46dc61d

Revert "have is_v0_config() return false when trying to load v1 confi…

9abf07c

…gs." This reverts commit 46dc61d.

Use try_load_config() instead of parse_metadata_table_or_manifest() i…

ddef86e

…n DistGraphBuilder

??? third ??? attempt at making is_v0_config work right

77872aa

Try being more precise about config versions

d833cd9

mistydemeo and others added 12 commits January 9, 2025 17:10

init: handle CLI hosting options

37b251a

init: update installers to use orig_meta

dfdec22

Make "dist init" automatically migrate from v0 to v1 config.

20b9a88

Have init perform migrations before everything else.

495726f

Have apply_dist_to_metadata() handle targets field.

230eed5

add some ... plumbing?... related code to apply_dist_to_metadata()

152e1fa

v1: remove duplicate hosts.github handler

083daaa

config: remove unused functions

29b2df1

More dist init TOML formatting.

983610d

Have dist init add most of the comments for [dist.installers]

107e212

Have "dist init" add comments under [dist.installers] for things in C…

83a7619

…ommonInstallerLayer.

mistydemeo mentioned this pull request Jan 10, 2025

feat: pkg followups #1381

Open

mistydemeo reviewed Jan 11, 2025

View reviewed changes

mistydemeo mentioned this pull request Jan 11, 2025

dist rename rollout plan #1481

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implement config v0 -> v1 migration. #1653

[WIP] Implement config v0 -> v1 migration. #1653

duckinator commented Dec 18, 2024 •

edited

Loading

duckinator commented Dec 20, 2024 •

edited

Loading

duckinator commented Jan 7, 2025

mistydemeo commented Jan 7, 2025 •

edited

Loading

mistydemeo left a comment

mistydemeo Jan 10, 2025

mistydemeo Jan 10, 2025

mistydemeo Jan 10, 2025

mistydemeo Jan 10, 2025

mistydemeo Jan 10, 2025

mistydemeo Jan 11, 2025

mistydemeo Jan 11, 2025

mistydemeo Jan 11, 2025

mistydemeo Jan 11, 2025

mistydemeo Jan 11, 2025


		apply_installers_common(npm_table, &npm.common);

		// TODO: similar to shell


		apply_installers_common(powershell_table, &powershell.common);

		// TODO: similar to shell

[WIP] Implement config v0 -> v1 migration. #1653

Are you sure you want to change the base?

[WIP] Implement config v0 -> v1 migration. #1653

Conversation

duckinator commented Dec 18, 2024 • edited Loading

duckinator commented Dec 20, 2024 • edited Loading

duckinator commented Jan 7, 2025

mistydemeo commented Jan 7, 2025 • edited Loading

mistydemeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duckinator commented Dec 18, 2024 •

edited

Loading

duckinator commented Dec 20, 2024 •

edited

Loading

mistydemeo commented Jan 7, 2025 •

edited

Loading