From 7637342714c3dc7e388fcbe587c3faa6c8a9b6c1 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 21:27:08 -0300 Subject: [PATCH 1/3] fix(features): write Feature ENV lines and options in declared order The image's ENV lines for a Feature's containerEnv and the config's containerEnv, and the user's options in devcontainer-features.env, were written in sorted key order. The reference CLI writes them in JS property order (integer-index keys ascending, then declaration order). Besides the order itself, a Feature's ENV line that uses a variable an earlier line sets, such as PATH=${SDKMAN_DIR}/bin:${PATH} after SDKMAN_DIR, expanded to a different value when the sort put it first, so the built image's environment differed. The member order is now recorded where the documents are parsed (devcontainer-feature.json, devcontainer.json and its override, --additional-features) and carried to the generated Dockerfile through features.PropertyOrder. The two ts-oracle cases recorded as unresolved divergences now match TS, a TS-generated ts-features-build golden pins the dependent ENV lines, and a runtime parity case compares the built image's env and devcontainer-features.env with the reference. Co-Authored-By: Claude Opus 5.5 --- docs/parity/parity-matrix.yaml | 14 ++++ internal/cli/build.go | 4 +- internal/cli/compose_up.go | 3 +- internal/cli/feature_deps.go | 6 +- internal/cli/feature_install.go | 79 ++++++++++--------- internal/cli/feature_install_held_test.go | 2 +- internal/cli/feature_install_test.go | 59 +++++++++++++- internal/cli/features_resolve_deps.go | 2 +- internal/cli/helpers.go | 6 ++ internal/cli/helpers_held_test.go | 20 +++++ internal/cli/outdated.go | 2 +- internal/cli/read_configuration.go | 2 +- internal/cli/up.go | 2 + internal/config/key_order_test.go | 33 ++++++++ internal/config/loader.go | 19 +++++ internal/config/types.go | 3 + internal/features/graph.go | 3 + internal/features/jsobject.go | 13 +++ internal/features/jsobject_test.go | 12 +++ internal/features/order.go | 1 + internal/features/resolve.go | 8 +- internal/features/resolve_test.go | 22 +++++- internal/features/types.go | 14 +++- internal/imagemeta/extend.go | 28 +++---- internal/imagemeta/extend_test.go | 9 ++- internal/imagemeta/reference_oracle_test.go | 3 +- .../declaration-order/Dockerfile | 45 +++++++++++ .../declaration-order/input.json | 1 + .../sdk_0/devcontainer-features-install.sh | 30 +++++++ .../sdk_0/devcontainer-features.env | 3 + .../testdata/ts-oracle/imagemeta.json | 27 ++++++- internal/jsonc/jsonc.go | 26 +++--- internal/jsonc/members_test.go | 12 +++ 33 files changed, 429 insertions(+), 84 deletions(-) create mode 100644 internal/imagemeta/testdata/ts-features-build/declaration-order/Dockerfile create mode 100644 internal/imagemeta/testdata/ts-features-build/declaration-order/input.json create mode 100644 internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features-install.sh create mode 100644 internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features.env diff --git a/docs/parity/parity-matrix.yaml b/docs/parity/parity-matrix.yaml index 312ffd2..6213b21 100644 --- a/docs/parity/parity-matrix.yaml +++ b/docs/parity/parity-matrix.yaml @@ -1079,6 +1079,20 @@ initial_cases: current_status: match notes: "Without BuildKit, TS builds the dev_container_feature_content_temp image from the features folder and COPYs /tmp/build-features/hello_0 to /tmp/dev-container-features/hello_0 (kept in the image), so verify_cmd also compares the staged devcontainer-features.env and the devcontainer-features-install.sh banner (getFeatureInstallWrapperScript). Go used to copy from the build context to /tmp/build-features/_dev_container_feature_0." + - id: build.features-env-declaration-order-success + lane: runtime + command: build + priority: p1 + docker_required: true + setup_cmd: "WORKSPACE=/tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf $WORKSPACE; mkdir -p $WORKSPACE/.devcontainer/sdk; printf '%s' '{\"id\":\"sdk\",\"version\":\"1.0.0\",\"name\":\"Sdk\",\"options\":{\"version\":{\"type\":\"string\",\"default\":\"latest\"}},\"containerEnv\":{\"SDKMAN_DIR\":\"/usr/local/sdkman\",\"PATH\":\"${SDKMAN_DIR}/bin:${PATH}\"}}' > $WORKSPACE/.devcontainer/sdk/devcontainer-feature.json; printf '%s\\n' '#!/bin/sh' 'set -e' 'cp devcontainer-features.env /features-env.txt' > $WORKSPACE/.devcontainer/sdk/install.sh; chmod +x $WORKSPACE/.devcontainer/sdk/install.sh; printf '%s' '{\"image\":\"alpine:3\",\"containerEnv\":{\"ZED\":\"1\",\"ALPHA\":\"2\"},\"features\":{\"./sdk\":{\"zz\":\"1\",\"version\":\"2\",\"aa\":\"3\"}}}' > $WORKSPACE/.devcontainer/devcontainer.json; echo WORKSPACE=$WORKSPACE" + ts_cmd: "build --workspace-folder ${WORKSPACE} --image-name parity-${PARITY_CASE_ID}-${PARITY_SIDE}" + verify_cmd: "docker run --rm ${IMAGE_NAME} cat /features-env.txt; echo; docker image inspect -f '{{json .Config.Env}}' ${IMAGE_NAME}" + cleanup_cmd: "docker rmi -f parity-${PARITY_CASE_ID}-${PARITY_SIDE} >/dev/null 2>&1; rm -rf /tmp/parity-${PARITY_CASE_ID}-${PARITY_SIDE}; true" + asserts: [exit_code, stdout_normalized] + class: build-runtime + current_status: match + notes: "TS writes a Feature's containerEnv as ENV lines in devcontainer-feature.json's order and the config's containerEnv in devcontainer.json's (generateContainerEnvs, Object.keys), and devcontainer-features.env lists the options the Feature declares, then the user's other options in the user's order (getFeatureValueObject). The Feature's PATH line uses SDKMAN_DIR, which the line before it sets, so the image's PATH starts with /usr/local/sdkman/bin. Go sorted the keys: PATH came first and expanded an unset SDKMAN_DIR to /bin, ALPHA preceded ZED, and AA preceded ZZ." + - id: up.compose-features-staging-layout-success lane: runtime command: up diff --git a/internal/cli/build.go b/internal/cli/build.go index 04e3b1e..a58b91c 100644 --- a/internal/cli/build.go +++ b/internal/cli/build.go @@ -576,7 +576,7 @@ func imageBuildFor(cfg *config.DevContainer, opts *buildOpts) imageBuild { } func checkLegacyFeatures(feats map[string]any) error { - userFeatures, _ := features.UpdateDeprecatedFeaturesIntoOptions(sortedUserFeatures(feats)) + userFeatures, _ := features.UpdateDeprecatedFeaturesIntoOptions(sortedUserFeatures(feats, nil)) for _, f := range userFeatures { id, _ := features.BackwardCompatibleID(f.UserFeatureID) if features.ClassifyID(id) == features.SourceLegacyShorthand { @@ -830,6 +830,8 @@ func (r *buildRunner) featureBuildOptions(cfg *config.DevContainer, hostSub *con Push: opts.push, Output: opts.output, ContainerEnv: cfg.ContainerEnv, + ContainerEnvOrder: cfg.ContainerEnvOrder, + FeatureOptionOrder: cfg.FeatureOptionOrder, FrozenLockfile: opts.experimentalFrozenLockfile, NoLockfile: opts.noLockfile, ConfigPath: cfg.ConfigFilePath, diff --git a/internal/cli/compose_up.go b/internal/cli/compose_up.go index 630d738..3f7a051 100644 --- a/internal/cli/compose_up.go +++ b/internal/cli/compose_up.go @@ -274,7 +274,7 @@ func (r *upRunner) composeFeatureSets(ctx context.Context, cfg *config.DevContai return nil, "", err } } - res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, lockfile, cfg.OverrideFeatureInstallOrder) + res, err := fetchFeatureSetsWithOrder(ctx, r.log, cfg.Features, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(cfg.ConfigFilePath), opts.workspaceFolder}, lockfile, cfg.OverrideFeatureInstallOrder) if err != nil { return nil, "", err } @@ -454,6 +454,7 @@ func (r *upRunner) buildAndExtendCompose(ctx context.Context, s composeStart, pe UseBuildKitContexts: useContexts, Syntax: syntax, ConfigContainerEnv: cfg.ContainerEnv, + ConfigEnvOrder: cfg.ContainerEnvOrder, }) if !useContexts { if err := buildFeatureContentImage(ctx, r.docker, featuresFolder); err != nil { diff --git a/internal/cli/feature_deps.go b/internal/cli/feature_deps.go index 269b596..f3f340f 100644 --- a/internal/cli/feature_deps.go +++ b/internal/cli/feature_deps.go @@ -47,7 +47,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local loc if err != nil { return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err) } - meta.Value = node.Options + meta.Value, meta.ValueOrder = node.Options, node.OptionOrder if meta.ID == "" { meta.ID = id } @@ -64,7 +64,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local loc if err != nil { return nil, fmt.Errorf("ERR: Feature '%s' could not be processed. %w", id, err) } - meta.Value = node.Options + meta.Value, meta.ValueOrder = node.Options, node.OptionOrder return &features.Set{ SourceInfo: &features.TarballSource{TarballURI: id, UserID: id}, Features: []features.Feature{meta}, @@ -94,7 +94,7 @@ func newMetadataProcessFeature(client oci.Registry, logger log.Logger, local loc if annoID := ociAnnotationID(manifest); annoID != "" { meta.ID = annoID } - meta.Value = node.Options + meta.Value, meta.ValueOrder = node.Options, node.OptionOrder return &features.Set{ SourceInfo: &features.OCISource{ diff --git a/internal/cli/feature_install.go b/internal/cli/feature_install.go index 20287bd..0921a8b 100644 --- a/internal/cli/feature_install.go +++ b/internal/cli/feature_install.go @@ -47,6 +47,8 @@ type FeatureBuildOptions struct { Secrets []string CABundle string ContainerEnv map[string]string + ContainerEnvOrder []string + FeatureOptionOrder map[string][]string SkipPersistCustoms bool FeaturesBasePath string WorkspaceFolder string @@ -64,12 +66,12 @@ type fetchFeatureResult struct { TmpDir string } -func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, local localFeatureRoots) (*fetchFeatureResult, error) { - return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, local, nil, nil) +func fetchFeatureSets(ctx context.Context, logger log.Logger, featuresCfg map[string]any, optionOrder map[string][]string, local localFeatureRoots) (*fetchFeatureResult, error) { + return fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, optionOrder, local, nil, nil) } -func sortedUserFeatures(featuresCfg map[string]any) []features.DevContainerFeature { - userFeatures := features.UserFeaturesToArray(featuresCfg) +func sortedUserFeatures(featuresCfg map[string]any, optionOrder map[string][]string) []features.DevContainerFeature { + userFeatures := features.UserFeaturesToArray(featuresCfg, optionOrder) sort.Slice(userFeatures, func(i, j int) bool { return userFeatures[i].UserFeatureID < userFeatures[j].UserFeatureID }) @@ -110,12 +112,12 @@ func createFeaturesFolder() (string, error) { } } -func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, local localFeatureRoots, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) { +func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresCfg map[string]any, optionOrder map[string][]string, local localFeatureRoots, lockfile *features.Lockfile, overrideOrder []string) (*fetchFeatureResult, error) { if len(featuresCfg) == 0 { return nil, nil } - userFeatures := sortedUserFeatures(featuresCfg) + userFeatures := sortedUserFeatures(featuresCfg, optionOrder) userFeatures, warnings := features.UpdateDeprecatedFeaturesIntoOptions(userFeatures) for _, w := range warnings { logger.Write(w, log.LevelWarning) @@ -169,7 +171,7 @@ func fetchFeatureSetsWithOrder(ctx context.Context, logger log.Logger, featuresC const featureStagePrefix = ".stage-" -func featureFromMetadata(m features.Feature, id string, opts any, cachePath string) features.Feature { +func featureFromMetadata(m features.Feature, id string, opts any, optOrder []string, cachePath string) features.Feature { return features.Feature{ ID: id, Version: m.Version, @@ -177,6 +179,7 @@ func featureFromMetadata(m features.Feature, id string, opts any, cachePath stri Description: m.Description, DocumentationURL: m.DocumentationURL, Value: opts, + ValueOrder: optOrder, UserOptions: extractUserOptions(opts), Options: m.Options, DependsOn: m.DependsOn, @@ -184,6 +187,7 @@ func featureFromMetadata(m features.Feature, id string, opts any, cachePath stri InstallsAfter: m.InstallsAfter, LegacyIds: m.LegacyIds, ContainerEnv: m.ContainerEnv, + ContainerEnvOrder: m.ContainerEnvOrder, Mounts: m.Mounts, Init: m.Init, Privileged: m.Privileged, @@ -212,7 +216,7 @@ func processInstallFeature( nextStageDir func() string, ) (*features.Set, error) { id := node.UserFeatureID - opts := node.Options + opts, optOrder := node.Options, node.OptionOrder resolvedID, srcType, err := resolveFeatureID(logger, id) if err != nil { return nil, err @@ -248,7 +252,7 @@ func processInstallFeature( if featID == "" { featID = filepath.Base(id) } - feat := featureFromMetadata(m, featID, opts, featureDir) + feat := featureFromMetadata(m, featID, opts, optOrder, featureDir) if feat.Name == "" { feat.Name = id } @@ -287,7 +291,7 @@ func processInstallFeature( featID = tarballFeatureID(id) } - feat := featureFromMetadata(m, featID, opts, featureDir) + feat := featureFromMetadata(m, featID, opts, optOrder, featureDir) if feat.Name == "" { feat.Name = id } @@ -348,31 +352,33 @@ func processInstallFeature( } feat := features.Feature{ - ID: cmp.Or(m.ID, ref.ID), - Version: m.Version, - Name: m.Name, - Description: m.Description, - DocumentationURL: m.DocumentationURL, - Value: opts, - UserOptions: extractUserOptions(opts), - Options: m.Options, - DependsOn: m.DependsOn, - DependsOnOrder: m.DependsOnOrder, - InstallsAfter: m.InstallsAfter, - LegacyIds: m.LegacyIds, - ContainerEnv: m.ContainerEnv, - Init: m.Init, - Privileged: m.Privileged, - CapAdd: m.CapAdd, - SecurityOpt: m.SecurityOpt, - Entrypoint: m.Entrypoint, - Mounts: m.Mounts, - Customizations: m.Customizations, - Included: true, - CachePath: featureDir, - Metadata: m.Metadata, - File: m.Metadata, - SourceKeys: features.OCIFeatureKeys, + ID: cmp.Or(m.ID, ref.ID), + Version: m.Version, + Name: m.Name, + Description: m.Description, + DocumentationURL: m.DocumentationURL, + Value: opts, + ValueOrder: optOrder, + UserOptions: extractUserOptions(opts), + Options: m.Options, + DependsOn: m.DependsOn, + DependsOnOrder: m.DependsOnOrder, + InstallsAfter: m.InstallsAfter, + LegacyIds: m.LegacyIds, + ContainerEnv: m.ContainerEnv, + ContainerEnvOrder: m.ContainerEnvOrder, + Init: m.Init, + Privileged: m.Privileged, + CapAdd: m.CapAdd, + SecurityOpt: m.SecurityOpt, + Entrypoint: m.Entrypoint, + Mounts: m.Mounts, + Customizations: m.Customizations, + Included: true, + CachePath: featureDir, + Metadata: m.Metadata, + File: m.Metadata, + SourceKeys: features.OCIFeatureKeys, } if feat.Version == "" { feat.Version = ref.Tag @@ -588,7 +594,7 @@ func extendImageWithFeatures( return nil, err } } - result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, lockfile, fbOpts.OverrideFeatureInstallOrder) + result, err := fetchFeatureSetsWithOrder(ctx, logger, featuresCfg, fbOpts.FeatureOptionOrder, localFeatureRoots{fbOpts.FeaturesBasePath, fbOpts.WorkspaceFolder}, lockfile, fbOpts.OverrideFeatureInstallOrder) if err != nil { return nil, err } @@ -656,6 +662,7 @@ func extendImageWithFeatures( UseBuildKitContexts: useBuildx, Syntax: syntax, ConfigContainerEnv: fbOpts.ContainerEnv, + ConfigEnvOrder: fbOpts.ContainerEnvOrder, SecretIDs: secretIDs, CABundle: caSecrets != nil, }) diff --git a/internal/cli/feature_install_held_test.go b/internal/cli/feature_install_held_test.go index 17aaf4c..f48bd19 100644 --- a/internal/cli/feature_install_held_test.go +++ b/internal/cli/feature_install_held_test.go @@ -429,7 +429,7 @@ func TestTarballFeatureIDUsesTheURLPath(t *testing.T) { func TestFetchFeatureSetsReportsTheFirstSortedFailure(t *testing.T) { for range 5 { - _, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, devcontainerRoots(t)) + _, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./b-missing": true, "./a-missing": true, "./c-missing": true}, nil, devcontainerRoots(t)) if err == nil || !strings.Contains(err.Error(), "'./a-missing'") { t.Fatalf("err = %v, want the failure of ./a-missing", err) } diff --git a/internal/cli/feature_install_test.go b/internal/cli/feature_install_test.go index dca727f..eb48c14 100644 --- a/internal/cli/feature_install_test.go +++ b/internal/cli/feature_install_test.go @@ -148,7 +148,7 @@ func TestFetchFeatureSets(t *testing.T) { t.Run(tt.name, func(t *testing.T) { roots := devcontainerRoots(t) tt.setup(t, roots.configDir) - result, err := fetchFeatureSets(t.Context(), log.Null, tt.entries, roots) + result, err := fetchFeatureSets(t.Context(), log.Null, tt.entries, nil, roots) if result != nil && result.TmpDir != "" { defer os.RemoveAll(result.TmpDir) } @@ -232,7 +232,7 @@ func TestFetchFeatureSets_StagingLayout(t *testing.T) { if err := os.WriteFile(filepath.Join(dirB, "install.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { t.Fatal(err) } - result, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./dir-a": true}, roots) + result, err := fetchFeatureSets(t.Context(), log.Null, map[string]any{"./dir-a": true}, nil, roots) if err != nil { t.Fatal(err) } @@ -396,7 +396,7 @@ func TestFetchFeatureSetsRefusesLocalFeaturesOutsideDevcontainer(t *testing.T) { t.Run(tc.id, func(t *testing.T) { var logs bytes.Buffer logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}) - result, err := fetchFeatureSets(t.Context(), logger, map[string]any{tc.id: true}, roots) + result, err := fetchFeatureSets(t.Context(), logger, map[string]any{tc.id: true}, nil, roots) if result != nil { t.Fatalf("result = %+v, want none", result) } @@ -420,7 +420,7 @@ func TestFetchFeatureSetsRefusesLocalFeaturesOutsideDevcontainer(t *testing.T) { func TestFetchFeatureSetsMovesDeprecatedFeaturesIntoOptions(t *testing.T) { var logs bytes.Buffer logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: &logs}) - result, err := fetchFeatureSets(t.Context(), logger, map[string]any{"gradle": map[string]any{}, "node:18": map[string]any{}}, devcontainerRoots(t)) + result, err := fetchFeatureSets(t.Context(), logger, map[string]any{"gradle": map[string]any{}, "node:18": map[string]any{}}, nil, devcontainerRoots(t)) if result != nil || err == nil || err.Error() != fmt.Sprintf(msgLegacyFeature, "node:18") { t.Fatalf("result = %+v, err = %v; want the legacy Feature error for node:18", result, err) } @@ -457,3 +457,54 @@ func TestCreateFeaturesFolderIsTheRunsAlone(t *testing.T) { seen[dir] = true } } + +func TestFetchedFeaturesKeepTheDeclaredOrder(t *testing.T) { + roots := devcontainerRoots(t) + write := func(name, metadata string) { + t.Helper() + dir := filepath.Join(roots.configDir, name) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "devcontainer-feature.json"), []byte(metadata), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "install.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + } + write("sdk", `{"id": "sdk", "version": "1.0.0", + "options": {"version": {"type": "string", "default": "latest"}}, + "containerEnv": {"SDKMAN_DIR": "/usr/local/sdkman", "PATH": "${SDKMAN_DIR}/bin:${PATH}"}, + "dependsOn": {"./dep": {"zz": "1", "aa": "2"}}}`) + write("dep", `{"id": "dep", "version": "1.0.0"}`) + + result, err := fetchFeatureSets(t.Context(), log.Null, + map[string]any{"./sdk": map[string]any{"zz": "1", "version": "2", "aa": "3"}}, + map[string][]string{"./sdk": {"zz", "version", "aa"}}, roots) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(result.TmpDir) + if err := stageFeatureBuildFiles(result.TmpDir, result.FeatureSets, "root", "root"); err != nil { + t.Fatal(err) + } + wantEnv := map[string]string{"sdk": "VERSION=\"2\"\nZZ=\"1\"\nAA=\"3\"", "dep": "ZZ=\"1\"\nAA=\"2\""} + for _, set := range result.FeatureSets { + f := set.Features[0] + data, err := os.ReadFile(filepath.Join(f.CachePath, imagemeta.FeatureEnvFile)) + if err != nil { + t.Fatal(err) + } + if got := string(data); got != wantEnv[f.ID] { + t.Errorf("%s %s = %q, want %q", f.ID, imagemeta.FeatureEnvFile, got, wantEnv[f.ID]) + } + } + + dockerfile := imagemeta.GenerateFeaturesBuildOptions(imagemeta.FeaturesBuildParams{ + BaseName: "base", FeatureSets: result.FeatureSets, ContainerUser: "root", RemoteUser: "root", UseBuildKitContexts: true, + }).DockerfileContent + if want := "ENV SDKMAN_DIR=\"/usr/local/sdkman\"\nENV PATH=\"${SDKMAN_DIR}/bin:${PATH}\"\n"; !strings.Contains(dockerfile, want) { + t.Errorf("Dockerfile lacks the Feature's ENV lines in declaration order %q:\n%s", want, dockerfile) + } +} diff --git a/internal/cli/features_resolve_deps.go b/internal/cli/features_resolve_deps.go index 0e28eec..9ce5ce9 100644 --- a/internal/cli/features_resolve_deps.go +++ b/internal/cli/features_resolve_deps.go @@ -38,7 +38,7 @@ func realFeaturesResolveDepsCmd() *cobra.Command { } cfg := loadResult.Config - userFeatures := features.UserFeaturesToArray(cfg.Features) + userFeatures := features.UserFeaturesToArray(cfg.Features, cfg.FeatureOptionOrder) if len(userFeatures) == 0 { logger.Write(fmt.Sprintf("Could not parse features object in configuration '%s'", cfg.ConfigFilePath), log.LevelError) return &coreerrors.ExitCodeError{Code: 1} diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index ab8fd19..305650d 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -29,6 +29,12 @@ func mergeAdditionalFeatures(cfg *config.DevContainer, additionalFeaturesJSON st if _, exists := cfg.Features[k]; !exists { cfg.Features[k] = v additionalOnly[k] = true + if names := jsonc.MemberNames([]byte(additionalFeaturesJSON), k); len(names) > 0 { + if cfg.FeatureOptionOrder == nil { + cfg.FeatureOptionOrder = map[string][]string{} + } + cfg.FeatureOptionOrder[k] = names + } } } return additionalOnly, nil diff --git a/internal/cli/helpers_held_test.go b/internal/cli/helpers_held_test.go index 634ca37..faff35f 100644 --- a/internal/cli/helpers_held_test.go +++ b/internal/cli/helpers_held_test.go @@ -2,6 +2,7 @@ package cli import ( "maps" + "slices" "testing" "github.com/devcontainers/cli/internal/config" @@ -36,6 +37,25 @@ func TestMergeAdditionalFeatures(t *testing.T) { } } +func TestMergeAdditionalFeaturesKeepsTheOptionOrder(t *testing.T) { + cfg := &config.DevContainer{ + Features: map[string]any{"./f": map[string]any{"b": 1, "a": 2}}, + FeatureOptionOrder: map[string][]string{"./f": {"b", "a"}}, + } + if _, err := mergeAdditionalFeatures(cfg, `{"./f": {"x": 1}, "./g": {"zz": 1, "aa": 2}, "./h": true}`); err != nil { + t.Fatal(err) + } + want := map[string][]string{"./f": {"b", "a"}, "./g": {"zz", "aa"}} + if len(cfg.FeatureOptionOrder) != len(want) { + t.Errorf("FeatureOptionOrder = %v, want %v", cfg.FeatureOptionOrder, want) + } + for id, names := range want { + if !slices.Equal(cfg.FeatureOptionOrder[id], names) { + t.Errorf("FeatureOptionOrder[%s] = %v, want %v", id, cfg.FeatureOptionOrder[id], names) + } + } +} + func TestMergeAdditionalFeaturesRejectsInvalidJSON(t *testing.T) { cfg := &config.DevContainer{} if _, err := mergeAdditionalFeatures(cfg, `{"x":`); err == nil { diff --git a/internal/cli/outdated.go b/internal/cli/outdated.go index 6f739d1..ad2c180 100644 --- a/internal/cli/outdated.go +++ b/internal/cli/outdated.go @@ -447,7 +447,7 @@ func newUpgradeCmd() *cobra.Command { lockfilePath := cfg.ConfigFilePath - res, err := fetchFeatureSetsWithOrder(cmd.Context(), logger, cfg.Features, localFeatureRoots{filepath.Dir(lockfilePath), ws}, nil, nil) + res, err := fetchFeatureSetsWithOrder(cmd.Context(), logger, cfg.Features, cfg.FeatureOptionOrder, localFeatureRoots{filepath.Dir(lockfilePath), ws}, nil, nil) if err != nil { return err } diff --git a/internal/cli/read_configuration.go b/internal/cli/read_configuration.go index e632c0e..a00b22e 100644 --- a/internal/cli/read_configuration.go +++ b/internal/cli/read_configuration.go @@ -183,7 +183,7 @@ func runReadConfiguration(ctx context.Context, out Output, opts *readConfigOpts) if needsFeaturesConfiguration(opts, containerID, result) { lgr := log.New(log.Options{Level: log.ParseLevel(opts.logLevel), Format: opts.logFormat, Writer: logDst}) var featErr error - featResult, featErr = fetchFeatureSets(ctx, lgr, result.Config.Features, localFeatureRoots{filepath.Dir(result.Config.ConfigFilePath), workspaceFolder}) + featResult, featErr = fetchFeatureSets(ctx, lgr, result.Config.Features, result.Config.FeatureOptionOrder, localFeatureRoots{filepath.Dir(result.Config.ConfigFilePath), workspaceFolder}) if featErr != nil { return readConfigurationFailure(lgr, featErr) } diff --git a/internal/cli/up.go b/internal/cli/up.go index 9333308..5384f41 100644 --- a/internal/cli/up.go +++ b/internal/cli/up.go @@ -1185,6 +1185,8 @@ func (r *upRunner) featureBuildOptions(cfg *config.DevContainer, loadResult *con LockfileExcludeIDs: opts.lockfileExcludeIDs, HostSub: &loadResult.HostSub, ContainerEnv: cfg.ContainerEnv, + ContainerEnvOrder: cfg.ContainerEnvOrder, + FeatureOptionOrder: cfg.FeatureOptionOrder, AdditionalMetadata: []imagemeta.Entry{configMetadataEntry(cfg, false)}, LabelMetadata: []imagemeta.Entry{r.configLabel}, } diff --git a/internal/config/key_order_test.go b/internal/config/key_order_test.go index b857fbe..59abaaa 100644 --- a/internal/config/key_order_test.go +++ b/internal/config/key_order_test.go @@ -25,3 +25,36 @@ func TestLoadKeepsTheDocumentKeyOrder(t *testing.T) { t.Errorf("KeyOrder = %v, want %v", res.KeyOrder, want) } } + +func TestLoadKeepsContainerEnvAndFeatureOptionOrder(t *testing.T) { + dir := t.TempDir() + base := filepath.Join(dir, "devcontainer.json") + override := filepath.Join(dir, "override.json") + if err := os.WriteFile(base, []byte(`{ + "image": "i", + "containerEnv": {"ZED": "1", "ALPHA": "2"}, + "features": {"./f": {"zz": "1", "aa": "2"}, "./g": true} + }`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(override, []byte(`{"containerEnv": {"MID": "3", "ZED": "4"}, "features": {"./f": {"mm": "3"}, "./h": {"b": 1, "a": 2}}}`), 0o644); err != nil { + t.Fatal(err) + } + res, err := LoadDevContainerConfig(dir, base, override) + if err != nil { + t.Fatal(err) + } + cfg := res.Config + if want := []string{"ZED", "ALPHA", "MID", "ZED"}; !slices.Equal(cfg.ContainerEnvOrder, want) { + t.Errorf("ContainerEnvOrder = %v, want %v", cfg.ContainerEnvOrder, want) + } + want := map[string][]string{"./f": {"zz", "aa", "mm"}, "./h": {"b", "a"}} + if len(cfg.FeatureOptionOrder) != len(want) { + t.Errorf("FeatureOptionOrder = %v, want %v", cfg.FeatureOptionOrder, want) + } + for id, names := range want { + if !slices.Equal(cfg.FeatureOptionOrder[id], names) { + t.Errorf("FeatureOptionOrder[%s] = %v, want %v", id, cfg.FeatureOptionOrder[id], names) + } + } +} diff --git a/internal/config/loader.go b/internal/config/loader.go index 0660af5..6d31519 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -133,6 +133,7 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi objectPath := cmp.Or(configPath, overrideConfigPath) var raw map[string]any var keyOrder []string + var docs [][]byte if configPath != "" { baseData, err := os.ReadFile(configPath) switch { @@ -141,6 +142,7 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi return nil, wrapParseError("parse config", configPath, err) } keyOrder = jsonc.Names(baseData) + docs = append(docs, baseData) case overrideConfigPath == "": return nil, fmt.Errorf("read config %s: %w", configPath, err) } @@ -159,6 +161,7 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi } raw = deepMergeConfig(raw, overrideRaw) keyOrder = append(keyOrder, jsonc.Names(overrideData)...) + docs = append(docs, overrideData) } raw = updateFromOldProperties(raw) @@ -208,6 +211,7 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi } config.ConfigFilePath = configFilePath + config.ContainerEnvOrder, config.FeatureOptionOrder = declaredOrder(docs) return &LoadResult{ Config: &config, @@ -219,6 +223,21 @@ func LoadDevContainerConfigWithMounts(workspaceFolder, configPath, overrideConfi }, nil } +func declaredOrder(docs [][]byte) (containerEnv []string, featureOptions map[string][]string) { + for _, data := range docs { + containerEnv = append(containerEnv, jsonc.MemberNames(data, "containerEnv")...) + for _, id := range jsonc.MemberNames(data, "features") { + if names := jsonc.MemberNames(data, "features", id); len(names) > 0 { + if featureOptions == nil { + featureOptions = map[string][]string{} + } + featureOptions[id] = append(featureOptions[id], names...) + } + } + } + return containerEnv, featureOptions +} + func deepMergeConfig(base, override map[string]any) map[string]any { if base == nil { return override diff --git a/internal/config/types.go b/internal/config/types.go index 94fa26b..ef3a73a 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -56,6 +56,9 @@ type DevContainer struct { Extensions any `json:"extensions,omitempty"` Settings any `json:"settings,omitempty"` DevPort any `json:"devPort,omitempty"` + + ContainerEnvOrder []string `json:"-"` + FeatureOptionOrder map[string][]string `json:"-"` } type Customizations map[string]any diff --git a/internal/features/graph.go b/internal/features/graph.go index 400c587..2634142 100644 --- a/internal/features/graph.go +++ b/internal/features/graph.go @@ -5,6 +5,7 @@ import ( "maps" "slices" + "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/log" ) @@ -33,6 +34,7 @@ func BuildDependencyGraph( Type: "user-provided", UserFeatureID: f.UserFeatureID, Options: f.Options, + OptionOrder: f.OptionOrder, DependsOn: []*FNode{}, InstallsAfter: []*FNode{}, }) @@ -89,6 +91,7 @@ func resolveDependencyGraph( Type: "resolved", UserFeatureID: depID, Options: feat.DependsOn[depID], + OptionOrder: jsonc.MemberNames(feat.Metadata, "dependsOn", depID), DependsOn: []*FNode{}, InstallsAfter: []*FNode{}, } diff --git a/internal/features/jsobject.go b/internal/features/jsobject.go index 3c918cc..277a4bb 100644 --- a/internal/features/jsobject.go +++ b/internal/features/jsobject.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "maps" "slices" "strconv" "strings" @@ -87,6 +88,18 @@ func KeyOrder(keys []string) []string { return o.order() } +// PropertyOrder is KeyOrder of m's keys as declared, followed by the keys of +// m that declared does not name (a map a caller built), sorted. +func PropertyOrder[V any](m map[string]V, declared []string) []string { + keys := make([]string, 0, len(declared)+len(m)) + for _, k := range declared { + if _, ok := m[k]; ok { + keys = append(keys, k) + } + } + return KeyOrder(append(keys, slices.Sorted(maps.Keys(m))...)) +} + func (o *jsObject) MarshalJSON() ([]byte, error) { var buf bytes.Buffer buf.WriteByte('{') diff --git a/internal/features/jsobject_test.go b/internal/features/jsobject_test.go index 7275386..e6b314a 100644 --- a/internal/features/jsobject_test.go +++ b/internal/features/jsobject_test.go @@ -2,6 +2,7 @@ package features import ( "encoding/json" + "slices" "testing" ) @@ -52,3 +53,14 @@ func TestMetadataObject(t *testing.T) { t.Error("broken metadata: no error") } } + +func TestPropertyOrderListsKeysAsObjectKeys(t *testing.T) { + m := map[string]int{"PATH": 1, "SDKMAN_DIR": 2, "10": 3, "2": 4, "b": 5, "a": 6} + got := PropertyOrder(m, []string{"SDKMAN_DIR", "gone", "10", "PATH", "SDKMAN_DIR", "2"}) + if want := []string{"2", "10", "SDKMAN_DIR", "PATH", "a", "b"}; !slices.Equal(got, want) { + t.Errorf("PropertyOrder = %v, want %v", got, want) + } + if got := PropertyOrder(map[string]int{}, []string{"x"}); len(got) != 0 { + t.Errorf("PropertyOrder of an empty map = %v", got) + } +} diff --git a/internal/features/order.go b/internal/features/order.go index a55ca35..d0cd985 100644 --- a/internal/features/order.go +++ b/internal/features/order.go @@ -13,6 +13,7 @@ type FNode struct { Type string UserFeatureID string Options any + OptionOrder []string Set *Set DependsOn []*FNode InstallsAfter []*FNode diff --git a/internal/features/resolve.go b/internal/features/resolve.go index ea9b231..bd72815 100644 --- a/internal/features/resolve.go +++ b/internal/features/resolve.go @@ -123,18 +123,24 @@ func UpdateDeprecatedFeaturesIntoOptions(userFeatures []DevContainerFeature) (up return u.UserFeatureID == m.mapTo || u.UserFeatureID == qualified || strings.HasPrefix(u.UserFeatureID, qualified+":") }) if i == -1 { - updated = append(updated, DevContainerFeature{UserFeatureID: qualified + ":1", Options: map[string]any{m.option: true}}) + updated = append(updated, DevContainerFeature{UserFeatureID: qualified + ":1", Options: map[string]any{m.option: true}, OptionOrder: []string{m.option}}) continue } options := map[string]any{} + order := slices.Clone(updated[i].OptionOrder) switch o := updated[i].Options.(type) { case map[string]any: maps.Copy(options, o) case string: options["version"] = o + order = []string{"version"} } options[m.option] = true + if !slices.Contains(order, m.option) { + order = append(order, m.option) + } updated[i].Options = options + updated[i].OptionOrder = order } updated = slices.DeleteFunc(updated, func(u DevContainerFeature) bool { _, deprecated := deprecatedFeaturesIntoOptions[u.UserFeatureID] diff --git a/internal/features/resolve_test.go b/internal/features/resolve_test.go index 951c5ff..30b4b73 100644 --- a/internal/features/resolve_test.go +++ b/internal/features/resolve_test.go @@ -4,6 +4,7 @@ import ( "errors" "path/filepath" "reflect" + "slices" "strings" "testing" ) @@ -75,9 +76,9 @@ func TestUpdateDeprecatedFeaturesIntoOptions(t *testing.T) { } got, warnings := UpdateDeprecatedFeaturesIntoOptions(in) want := []DevContainerFeature{ - {UserFeatureID: "ghcr.io/devcontainers/features/java:1", Options: map[string]any{"version": "17", "installGradle": true}}, + {UserFeatureID: "ghcr.io/devcontainers/features/java:1", Options: map[string]any{"version": "17", "installGradle": true}, OptionOrder: []string{"version", "installGradle"}}, {UserFeatureID: "node:18", Options: map[string]any{}}, - {UserFeatureID: "ghcr.io/devcontainers/features/python:1", Options: map[string]any{"installJupyterlab": true}}, + {UserFeatureID: "ghcr.io/devcontainers/features/python:1", Options: map[string]any{"installJupyterlab": true}, OptionOrder: []string{"installJupyterlab"}}, } if !reflect.DeepEqual(got, want) { t.Errorf("features = %v, want %v", got, want) @@ -88,6 +89,19 @@ func TestUpdateDeprecatedFeaturesIntoOptions(t *testing.T) { if in[0].Options != "17" || len(in) != 4 { t.Errorf("the input was modified: %v", in) } + + ordered := []DevContainerFeature{ + {UserFeatureID: "java", Options: map[string]any{"version": "21", "installMaven": false}, OptionOrder: []string{"version", "installMaven"}}, + {UserFeatureID: "maven", Options: true}, + {UserFeatureID: "gradle", Options: true}, + } + got, _ = UpdateDeprecatedFeaturesIntoOptions(ordered) + if len(got) != 1 || !slices.Equal(got[0].OptionOrder, []string{"version", "installMaven", "installGradle"}) { + t.Errorf("option order = %v, want the declared options then the new one once", got) + } + if ordered[0].OptionOrder[1] != "installMaven" || len(ordered[0].OptionOrder) != 2 { + t.Errorf("the input's option order was modified: %v", ordered[0].OptionOrder) + } } func TestUserFeaturesToArray(t *testing.T) { @@ -95,14 +109,14 @@ func TestUserFeaturesToArray(t *testing.T) { "ghcr.io/devcontainers/features/go:1": map[string]any{"version": "1.21"}, "ghcr.io/devcontainers/features/node:1": true, } - arr := UserFeaturesToArray(features) + arr := UserFeaturesToArray(features, nil) if len(arr) != 2 { t.Errorf("len = %d, want 2", len(arr)) } } func TestUserFeaturesToArray_Nil(t *testing.T) { - arr := UserFeaturesToArray(nil) + arr := UserFeaturesToArray(nil, nil) if arr != nil { t.Errorf("expected nil, got %v", arr) } diff --git a/internal/features/types.go b/internal/features/types.go index e9d72c8..74cff4f 100644 --- a/internal/features/types.go +++ b/internal/features/types.go @@ -33,8 +33,10 @@ type Feature struct { CachePath string `json:"cachePath,omitempty"` ConsecutiveId string `json:"consecutiveId,omitempty"` Value any `json:"value"` + ValueOrder []string `json:"-"` UserOptions map[string]any `json:"-"` ContainerEnv map[string]string `json:"containerEnv,omitempty"` + ContainerEnvOrder []string `json:"-"` Mounts []any `json:"mounts,omitempty"` Init *bool `json:"init,omitempty"` Privileged *bool `json:"privileged,omitempty"` @@ -204,6 +206,11 @@ func (f *Feature) UnmarshalJSON(data []byte) error { return err } f.DependsOnOrder = jsonc.MemberNames(data, "dependsOn") + // TS writes containerEnv's ENV lines and the value's options in the + // order the document declares them (a line may use a variable an + // earlier one sets), and Go maps keep none. + f.ContainerEnvOrder = jsonc.MemberNames(data, "containerEnv") + f.ValueOrder = jsonc.MemberNames(data, "value") f.Metadata = slices.Clone(data) return nil } @@ -310,9 +317,13 @@ type Config struct { type DevContainerFeature struct { UserFeatureID string Options any + // OptionOrder is the member names of Options's object as written. + OptionOrder []string } -func UserFeaturesToArray(features map[string]any) []DevContainerFeature { +// optionOrder maps a Feature id to the member names of its options object +// in devcontainer.json; an id it lacks has its options in sorted order. +func UserFeaturesToArray(features map[string]any, optionOrder map[string][]string) []DevContainerFeature { if features == nil { return nil } @@ -321,6 +332,7 @@ func UserFeaturesToArray(features map[string]any) []DevContainerFeature { result = append(result, DevContainerFeature{ UserFeatureID: id, Options: opts, + OptionOrder: optionOrder[id], }) } return result diff --git a/internal/imagemeta/extend.go b/internal/imagemeta/extend.go index 375859b..d00e10d 100644 --- a/internal/imagemeta/extend.go +++ b/internal/imagemeta/extend.go @@ -40,6 +40,7 @@ type FeaturesBuildParams struct { UseBuildKitContexts bool Syntax string ConfigContainerEnv map[string]string + ConfigEnvOrder []string // ConfigContainerEnv's member names as declared SecretIDs []string CABundle bool } @@ -119,7 +120,7 @@ func GenerateFeaturesBuildOptions(p FeaturesBuildParams) *ExtendImageBuildInfo { "#{featureLayer}", featureLayers(p, contentSourceRootPath), "#{containerEnv}", "", "#{devcontainerMetadata}", devcontainerMetadataLabel(p.Metadata), - "#{containerEnvMetadata}", generateContainerEnvs(p.ConfigContainerEnv, true), + "#{containerEnvMetadata}", generateContainerEnvs(p.ConfigContainerEnv, p.ConfigEnvOrder, true), ) dockerfile := replacer.Replace(containerFeaturesBaseDockerfile) @@ -165,7 +166,7 @@ func featureLayers(p FeaturesBuildParams, contentSourceRootPath string) string { } for _, set := range p.FeatureSets { for _, feat := range set.Features { - b.WriteString(generateContainerEnvs(feat.ContainerEnv, false)) + b.WriteString(generateContainerEnvs(feat.ContainerEnv, feat.ContainerEnvOrder, false)) source := path.Join(contentSourceRootPath, feat.ConsecutiveId) dest := path.Join(FeaturesContainerTempDestFolder, feat.ConsecutiveId) if !p.UseBuildKitContexts { @@ -184,8 +185,11 @@ func featureLayers(p FeaturesBuildParams, contentSourceRootPath string) string { return b.String() } -// Keys are sorted, where TS writes them in insertion order: Go maps carry none. -func generateContainerEnvs(env map[string]string, escapeDollar bool) string { +// The lines follow containerEnv's property order (order holds its member +// names as declared), as TS's Object.keys does: without escapeDollar, a value +// that references a variable an earlier line sets expands to that line's +// value. +func generateContainerEnvs(env map[string]string, order []string, escapeDollar bool) string { if len(env) == 0 { return "" } @@ -193,11 +197,7 @@ func generateContainerEnvs(env map[string]string, escapeDollar bool) string { if escapeDollar { special += "$" } - keys := make([]string, 0, len(env)) - for k := range env { - keys = append(keys, k) - } - sort.Strings(keys) + keys := features.PropertyOrder(env, order) lines := make([]string, 0, len(keys)) for _, k := range keys { lines = append(lines, "ENV "+k+`="`+backslashEscape(env[k], special)+`"`) @@ -322,12 +322,7 @@ func featureValueObject(feat features.Feature, info FeatureFileInfo) ([]string, } switch v := feat.Value.(type) { case map[string]any: - userKeys := make([]string, 0, len(v)) - for k := range v { - userKeys = append(userKeys, k) - } - sort.Strings(userKeys) - for _, k := range userKeys { + for _, k := range features.PropertyOrder(v, feat.ValueOrder) { set(k, v[k]) } case nil: @@ -337,7 +332,8 @@ func featureValueObject(feat features.Feature, info FeatureFileInfo) ([]string, set("version", v) } } - return keys, values + // getFeatureEnvVariables lists the object's Object.keys. + return features.PropertyOrder(values, keys), values } func jsString(v any) string { diff --git a/internal/imagemeta/extend_test.go b/internal/imagemeta/extend_test.go index a0d5267..feb6927 100644 --- a/internal/imagemeta/extend_test.go +++ b/internal/imagemeta/extend_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/devcontainers/cli/internal/features" + "github.com/devcontainers/cli/internal/jsonc" ) // tsFeaturesBuildCase is testdata/ts-features-build//input.json: the @@ -26,6 +27,7 @@ type tsFeaturesBuildCase struct { RemoteUser string `json:"remoteUser"` Metadata []Entry `json:"metadata"` ConfigContainerEnv map[string]string `json:"configContainerEnv"` + ConfigEnvOrder []string `json:"-"` FeatureSets []struct { SourceInformation struct { Type string `json:"type"` @@ -46,6 +48,7 @@ func loadTSFeaturesBuildCase(t *testing.T, name string) (tsFeaturesBuildCase, [] if err := json.Unmarshal(data, &c); err != nil { t.Fatal(err) } + c.ConfigEnvOrder = jsonc.MemberNames(data, "configContainerEnv") infos := map[string]FeatureFileInfo{} var sets []*features.Set for _, s := range c.FeatureSets { @@ -85,7 +88,10 @@ func readGolden(t *testing.T, parts ...string) string { } func TestGenerateFeaturesBuildOptions_MatchesTS(t *testing.T) { - for _, name := range []string{"buildkit", "no-buildkit"} { + // declaration-order: a Feature's containerEnv line that uses a variable + // an earlier line sets, user options the Feature does not declare, and + // the config's containerEnv, all in the order the documents write them. + for _, name := range []string{"buildkit", "no-buildkit", "declaration-order"} { t.Run(name, func(t *testing.T) { c, sets, infos := loadTSFeaturesBuildCase(t, name) info := GenerateFeaturesBuildOptions(FeaturesBuildParams{ @@ -97,6 +103,7 @@ func TestGenerateFeaturesBuildOptions_MatchesTS(t *testing.T) { ImageUser: "imguser", UseBuildKitContexts: c.UseBuildKit, ConfigContainerEnv: c.ConfigContainerEnv, + ConfigEnvOrder: c.ConfigEnvOrder, }) if want := readGolden(t, name, "Dockerfile"); info.DockerfileContent != want { t.Errorf("Dockerfile differs from TS:\n--- got\n%s\n--- want\n%s", info.DockerfileContent, want) diff --git a/internal/imagemeta/reference_oracle_test.go b/internal/imagemeta/reference_oracle_test.go index 7bd8cad..404cc84 100644 --- a/internal/imagemeta/reference_oracle_test.go +++ b/internal/imagemeta/reference_oracle_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/devcontainers/cli/internal/features" + "github.com/devcontainers/cli/internal/jsonc" "github.com/devcontainers/cli/internal/tsoracle/oracletest" ) @@ -54,7 +55,7 @@ func TestTSOracleImageMetadata(t *testing.T) { if err := oracletest.Args(args, &env, &escapeDollar); err != nil { return nil, err } - return generateContainerEnvs(env, escapeDollar), nil + return generateContainerEnvs(env, jsonc.Names(args[0]), escapeDollar), nil }, "getFeatureValueObject": func(args []json.RawMessage) (any, error) { var feat features.Feature diff --git a/internal/imagemeta/testdata/ts-features-build/declaration-order/Dockerfile b/internal/imagemeta/testdata/ts-features-build/declaration-order/Dockerfile new file mode 100644 index 0000000..a638c51 --- /dev/null +++ b/internal/imagemeta/testdata/ts-features-build/declaration-order/Dockerfile @@ -0,0 +1,45 @@ + + + + +ARG _DEV_CONTAINERS_BASE_IMAGE=scratch + +FROM $_DEV_CONTAINERS_BASE_IMAGE AS dev_containers_feature_content_normalize +USER root +COPY --from=dev_containers_feature_content_source devcontainer-features.builtin.env /tmp/build-features/ +RUN chmod -R 0755 /tmp/build-features/ + +FROM $_DEV_CONTAINERS_BASE_IMAGE AS dev_containers_target_stage + +USER root + +RUN mkdir -p /tmp/dev-container-features +COPY --from=dev_containers_feature_content_normalize /tmp/build-features/ /tmp/dev-container-features + +RUN \ +echo "_CONTAINER_USER_HOME=$( (command -v getent >/dev/null 2>&1 && getent passwd 'root' || grep -E '^root|^[^:]*:[^:]*:root:' /etc/passwd || true) | cut -d: -f6)" >> /tmp/dev-container-features/devcontainer-features.builtin.env && \ +echo "_REMOTE_USER_HOME=$( (command -v getent >/dev/null 2>&1 && getent passwd 'root' || grep -E '^root|^[^:]*:[^:]*:root:' /etc/passwd || true) | cut -d: -f6)" >> /tmp/dev-container-features/devcontainer-features.builtin.env + +ENV SDKMAN_DIR="/usr/local/sdkman" +ENV PATH="${SDKMAN_DIR}/bin:${PATH}" +RUN --mount=type=bind,from=dev_containers_feature_content_source,source=sdk_0,target=/tmp/build-features-src/sdk_0 \ + cp -ar /tmp/build-features-src/sdk_0 /tmp/dev-container-features \ + && chmod -R 0755 /tmp/dev-container-features/sdk_0 \ + && cd /tmp/dev-container-features/sdk_0 \ + && chmod +x ./devcontainer-features-install.sh \ + && ./devcontainer-features-install.sh \ + && rm -rf /tmp/dev-container-features/sdk_0 + + + + + +ARG _DEV_CONTAINERS_IMAGE_USER=root +USER $_DEV_CONTAINERS_IMAGE_USER + + + +ENV 2="two" +ENV 10="ten" +ENV ZED="1" +ENV ALPHA="\${ZED}" diff --git a/internal/imagemeta/testdata/ts-features-build/declaration-order/input.json b/internal/imagemeta/testdata/ts-features-build/declaration-order/input.json new file mode 100644 index 0000000..2821789 --- /dev/null +++ b/internal/imagemeta/testdata/ts-features-build/declaration-order/input.json @@ -0,0 +1 @@ +{"useBuildKit": true, "containerUser": "root", "remoteUser": "root", "metadata": [], "configContainerEnv": {"ZED": "1", "ALPHA": "${ZED}", "10": "ten", "2": "two"}, "featureSets": [{"internalVersion": "2", "sourceInformation": {"type": "file-path", "userFeatureId": "./sdk"}, "features": [{"id": "sdk", "consecutiveId": "sdk_0", "name": "./sdk", "value": {"zz": "1", "version": "2", "aa": "3"}, "options": {"version": {"type": "string", "default": "latest"}}, "containerEnv": {"SDKMAN_DIR": "/usr/local/sdkman", "PATH": "${SDKMAN_DIR}/bin:${PATH}"}}]}]} diff --git a/internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features-install.sh b/internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features-install.sh new file mode 100644 index 0000000..51e95cd --- /dev/null +++ b/internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features-install.sh @@ -0,0 +1,30 @@ +#!/bin/sh +set -e + +on_exit () { + [ $? -eq 0 ] && exit + echo 'ERROR: Feature "./sdk" (Unknown) failed to install!' +} + +trap on_exit EXIT + +echo =========================================================================== + +echo 'Feature : ./sdk' +echo 'Description : ' +echo 'Id : Unknown' +echo 'Version : ' +echo 'Documentation : ' +echo 'Options :' +echo ' VERSION="2" + ZZ="1" + AA="3"' +echo =========================================================================== + +set -a +. ../devcontainer-features.builtin.env +. ./devcontainer-features.env +set +a + +chmod +x ./install.sh +./install.sh diff --git a/internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features.env b/internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features.env new file mode 100644 index 0000000..9cfc5ec --- /dev/null +++ b/internal/imagemeta/testdata/ts-features-build/declaration-order/sdk_0/devcontainer-features.env @@ -0,0 +1,3 @@ +VERSION="2" +ZZ="1" +AA="3" \ No newline at end of file diff --git a/internal/imagemeta/testdata/ts-oracle/imagemeta.json b/internal/imagemeta/testdata/ts-oracle/imagemeta.json index 2083d3c..046ec96 100644 --- a/internal/imagemeta/testdata/ts-oracle/imagemeta.json +++ b/internal/imagemeta/testdata/ts-oracle/imagemeta.json @@ -1999,9 +1999,33 @@ }, true ], - "divergence": "unresolved: TS keeps containerEnv's key order; the Go config carries containerEnv as a map, so generateContainerEnvs sorts the keys", "result": "ENV ZED=\"1\"\nENV ALPHA=\"2\"" }, + { + "name": "a line uses a variable an earlier line sets", + "fn": "generateContainerEnvs", + "args": [ + { + "SDKMAN_DIR": "/usr/local/sdkman", + "PATH": "${SDKMAN_DIR}/bin:${PATH}" + }, + false + ], + "result": "ENV SDKMAN_DIR=\"/usr/local/sdkman\"\nENV PATH=\"${SDKMAN_DIR}/bin:${PATH}\"" + }, + { + "name": "integer keys first", + "fn": "generateContainerEnvs", + "args": [ + { + "B": "1", + "10": "2", + "2": "3" + }, + true + ], + "result": "ENV 2=\"3\"\nENV 10=\"2\"\nENV B=\"1\"" + }, { "name": "unicode and newline", "fn": "generateContainerEnvs", @@ -2338,7 +2362,6 @@ } } ], - "divergence": "unresolved: TS keeps the order of the user's option values; features.Feature.Value is a map, so featureValueObject sorts the keys the options do not declare", "result": [ [ "version", diff --git a/internal/jsonc/jsonc.go b/internal/jsonc/jsonc.go index 64a3209..896a775 100644 --- a/internal/jsonc/jsonc.go +++ b/internal/jsonc/jsonc.go @@ -36,21 +36,29 @@ func Names(data []byte) []string { return objectNames(v) } -func MemberNames(data []byte, field string) []string { +// MemberNames lists, in document order, the member names of the object +// that path leads to through nested objects: MemberNames(data, "features", +// id) names the options of one Feature in a devcontainer.json. +func MemberNames(data []byte, path ...string) []string { v, err := hujson.Parse(stripBOM(data)) if err != nil { return nil } - top, ok := v.Value.(*hujson.Object) - if !ok { - return nil - } - for _, m := range top.Members { - if lit, ok := m.Name.Value.(hujson.Literal); ok && lit.String() == field { - return objectNames(m.Value) +next: + for _, field := range path { + obj, ok := v.Value.(*hujson.Object) + if !ok { + return nil } + for _, m := range obj.Members { + if lit, ok := m.Name.Value.(hujson.Literal); ok && lit.String() == field { + v = m.Value + continue next + } + } + return nil } - return nil + return objectNames(v) } func objectNames(v hujson.Value) []string { diff --git a/internal/jsonc/members_test.go b/internal/jsonc/members_test.go index 490d01e..1f503c4 100644 --- a/internal/jsonc/members_test.go +++ b/internal/jsonc/members_test.go @@ -22,6 +22,18 @@ func TestMemberNamesKeepsDocumentOrder(t *testing.T) { } } +func TestMemberNamesFollowsAPath(t *testing.T) { + doc := []byte(`{"features": {"./f": {"zz": 1, /* c */ "aa": 2}, "./g": true}}`) + if got := MemberNames(doc, "features", "./f"); !slices.Equal(got, []string{"zz", "aa"}) { + t.Errorf("MemberNames(features, ./f) = %v, want [zz aa]", got) + } + for _, path := range [][]string{{"features", "./g"}, {"features", "./h"}, {"features", "./f", "zz"}} { + if got := MemberNames(doc, path...); got != nil { + t.Errorf("MemberNames(%v) = %v, want nil", path, got) + } + } +} + func TestNamesKeepsDocumentOrder(t *testing.T) { doc := []byte("\ufeff{\n // comment\n \"name\": \"x\",\n \"image\": \"y\",\n \"capAdd\": [],\n}") if got := Names(doc); !slices.Equal(got, []string{"name", "image", "capAdd"}) { From 76f495ce1055902e163ddb900b8bb923114ee088 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 22:12:06 -0300 Subject: [PATCH 2/3] test(cli): pin additional feature option order without prior order Co-Authored-By: Claude Opus 5.5 --- internal/cli/helpers_held_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/cli/helpers_held_test.go b/internal/cli/helpers_held_test.go index faff35f..1598e8b 100644 --- a/internal/cli/helpers_held_test.go +++ b/internal/cli/helpers_held_test.go @@ -65,3 +65,15 @@ func TestMergeAdditionalFeaturesRejectsInvalidJSON(t *testing.T) { t.Errorf("features = %v, want the config untouched on a parse error", cfg.Features) } } + +// A config with no recorded option order still gets one for an additional +// Feature, even one with a single option. +func TestMergeAdditionalFeaturesRecordsASingleOptionWithoutPriorOrder(t *testing.T) { + cfg := &config.DevContainer{} + if _, err := mergeAdditionalFeatures(cfg, `{"./g": {"only": 1}}`); err != nil { + t.Fatal(err) + } + if got := cfg.FeatureOptionOrder["./g"]; !slices.Equal(got, []string{"only"}) { + t.Errorf("FeatureOptionOrder[./g] = %v, want [only]", got) + } +} From f0dfb607948d7d91578096489ff4f31a6817a558 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 23:07:52 -0300 Subject: [PATCH 3/3] refactor(features): drop the capacity hint in PropertyOrder CodeQL's go/allocation-size-overflow flagged the sum of two lengths used as a make capacity. The hint bought nothing measurable, so the slice grows on append instead. Co-Authored-By: Claude Opus 5.5 --- internal/features/jsobject.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/features/jsobject.go b/internal/features/jsobject.go index 277a4bb..2f94401 100644 --- a/internal/features/jsobject.go +++ b/internal/features/jsobject.go @@ -91,7 +91,7 @@ func KeyOrder(keys []string) []string { // PropertyOrder is KeyOrder of m's keys as declared, followed by the keys of // m that declared does not name (a map a caller built), sorted. func PropertyOrder[V any](m map[string]V, declared []string) []string { - keys := make([]string, 0, len(declared)+len(m)) + var keys []string for _, k := range declared { if _, ok := m[k]; ok { keys = append(keys, k)