Merge pull request #6029 from kit-ty-kate/nullify-pos-effective

Nullify positions of the extensions fields in OpamFile.OPAM.effective_part

master_changes.md

@@ -41,6 +41,7 @@ users)
 ## Var/Option
 
 ## Update / Upgrade
+  * Fix `opam upgrade` wanting to recompile opam files containing the `x-env-path-rewrite` field [#6029 @kit-ty-kate - fix #6028]
 
 ## Tree
 
@@ -102,6 +103,7 @@ users)
 
 ## Reftests
 ### Tests
+  * add a complete test to make sure effectively_equal does not take the location of the fields into account [#6029 @kit-ty-kate]
 
 ### Engine
 
@@ -121,5 +123,6 @@ users)
 ## opam-solver
 
 ## opam-format
+  * `OpamTypesBase`: Add `nullify_pos_map` and `nullify_pos_value` [#6029 @kit-ty-kate]
 
 ## opam-core

src/format/opamFile.ml

@@ -3629,7 +3629,10 @@ module OPAM = struct
 
       (* We keep only `x-env-path-rewrite` as it affects build/install *)
       extensions  =
-        OpamStd.String.Map.filter (fun x _ -> String.equal rewrite_xfield x)
+        OpamStd.String.Map.filter_map (fun k v ->
+            if String.equal rewrite_xfield k
+            then Some (OpamTypesBase.nullify_pos_value v)
+            else None)
           t.extensions;
 
       url         = OpamStd.Option.map effective_url t.url;

src/format/opamTypesBase.ml

@@ -62,6 +62,56 @@ let pos_null =
     stop = -1, -1;
   }
 let nullify_pos pelem = {pelem; pos = pos_null}
+let nullify_pos_map f {pelem; pos = _} = nullify_pos (f pelem)
+
+let rec nullify_pos_value {pelem; pos = _} =
+  nullify_pos @@
+  match pelem with
+  | Bool b -> Bool (b : bool)
+  | Int i -> Int (i : int)
+  | String s -> String (s : string)
+  | Relop (relop, v1, v2) ->
+    Relop
+      (nullify_pos_map
+         (fun (x : OpamParserTypes.FullPos.relop_kind) -> x)
+         relop,
+       nullify_pos_value v1,
+       nullify_pos_value v2)
+  | Prefix_relop (relop, v) ->
+    Prefix_relop
+      (nullify_pos_map
+         (fun (x : OpamParserTypes.FullPos.relop_kind) -> x)
+         relop,
+       nullify_pos_value v)
+  | Logop (logop, v1, v2) ->
+    Logop
+      (nullify_pos_map
+         (fun (x : OpamParserTypes.FullPos.logop_kind) -> x)
+         logop,
+       nullify_pos_value v1,
+       nullify_pos_value v2)
+  | Pfxop (pfxop, v) ->
+    Pfxop
+      (nullify_pos_map
+         (fun (x : OpamParserTypes.FullPos.pfxop_kind) -> x)
+         pfxop,
+       nullify_pos_value v)
+  | Ident s -> Ident (s : string)
+  | List {pelem = l; pos = _} ->
+    List (nullify_pos (List.map nullify_pos_value l))
+  | Group {pelem = l; pos = _} ->
+    Group (nullify_pos (List.map nullify_pos_value l))
+  | Option (v, {pelem = filter; pos = _}) ->
+    Option
+      (nullify_pos_value v,
+       nullify_pos (List.map nullify_pos_value filter))
+  | Env_binding (v1, env_update_op, v2) ->
+    Env_binding
+      (nullify_pos_value v1,
+       nullify_pos_map
+         (fun (x : OpamParserTypes.FullPos.env_update_op_kind) -> x)
+         env_update_op,
+       nullify_pos_value v2)
 
 (* XXX update *)
 let pos_best pos1 pos2 =

src/format/opamTypesBase.mli

@@ -42,6 +42,8 @@ val string_of_shell: shell -> string
 (** The empty file position *)
 val pos_null: pos
 val nullify_pos : 'a -> 'a with_pos
+val nullify_pos_map : ('a -> 'b) -> 'a with_pos -> 'b with_pos
+val nullify_pos_value : value -> value
 
 (** [pos_best pos1 pos2] returns the most detailed position between [pos1] and
     [pos2] (defaulting to [pos1]) *)

tests/reftests/dune.inc

@@ -416,6 +416,24 @@
    %{targets}
    (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:download.test} %{read-lines:testing-env}))))
 
+(rule
+ (alias reftest-effectively-equal)
+ (action
+  (diff effectively-equal.test effectively-equal.out)))
+
+(alias
+ (name reftest)
+ (deps (alias reftest-effectively-equal)))
+
+(rule
+ (targets effectively-equal.out)
+ (deps root-N0REP0)
+ (package opam)
+ (action
+  (with-stdout-to
+   %{targets}
+   (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:effectively-equal.test} %{read-lines:testing-env}))))
+
 (rule
  (alias reftest-empty-conflicts-001)
  (action

tests/reftests/effectively-equal.test

@@ -0,0 +1,226 @@
+N0REP0
+### : test all the fields to check that effectively_equal works as expected
+### : each fields' value is placed on the next line to ensure the position will differ from the normalised version
+### <pkg:test-all-fields.1>
+opam-version:
+"2.0"
+name:
+"test-all-fields"
+version:
+"1"
+depends:
+["dummy-dep" {some-filter}]
+conflicts:
+["dummy-conflict" {some-filter}]
+depopts:
+["dummy-dep" {some-filter}]
+conflict-class:
+"conflict-class"
+available:
+true
+flags:
+[plugin]
+setenv:
+[ENV = "some value"]
+build:
+["false" {some-filter}]
+run-test:
+["false" {some-filter}]
+install:
+["false" {some-filter}]
+remove:
+["false" {some-filter}]
+substs:
+["some-file"]
+patches:
+["some-patches" {some-filter}]
+build-env:
+[BUILD_ENV = "some value"]
+extra-source
+"some-file"
+{
+src:
+"https://example.com"
+checksum: "md5=d41d8cd98f00b204e9800998ecf8427e"
+}
+messages:
+["message" {some-filter}]
+post-messages:
+["post-message" {some-filter}]
+depexts:
+[["depext"] {some-filter}]
+dev-repo:
+"git+https://example.com"
+pin-depends:
+["dep.dev" "https://example.com"]
+maintainer:
+"maintainer"
+authors:
+["author1" "author2"]
+license:
+"LicenseRef-custom"
+tags:
+["tag"]
+homepage:
+"https://example.com"
+doc:
+"https://example.com"
+bug-reports:
+"https://example.com"
+x-some-extension:
+[ext "something"]
+x-env-path-rewrite:
+[[ENV false]]
+url
+{
+src: "https://example.com"
+checksum: "md5=d41d8cd98f00b204e9800998ecf8427e"
+}
+synopsis:
+"some synopsis"
+description:
+"some description"
+### opam switch create eff-eq --empty
+### opam var --global some-filter=false
+Added '[some-filter "false" "Set through 'opam var'"]' to field global-variables in global configuration
+### opam install --fake test-all-fields
+The following actions will be faked:
+=== install 1 package
+  - install test-all-fields 1
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+Faking installation of test-all-fields.1
+Done.
+### cat OPAM/repo/default/packages/test-all-fields/test-all-fields.1/opam
+opam-version:
+"2.0"
+name:
+"test-all-fields"
+version:
+"1"
+depends:
+["dummy-dep" {some-filter}]
+conflicts:
+["dummy-conflict" {some-filter}]
+depopts:
+["dummy-dep" {some-filter}]
+conflict-class:
+"conflict-class"
+available:
+true
+flags:
+[plugin]
+setenv:
+[ENV = "some value"]
+build:
+["false" {some-filter}]
+run-test:
+["false" {some-filter}]
+install:
+["false" {some-filter}]
+remove:
+["false" {some-filter}]
+substs:
+["some-file"]
+patches:
+["some-patches" {some-filter}]
+build-env:
+[BUILD_ENV = "some value"]
+extra-source
+"some-file"
+{
+src:
+"https://example.com"
+checksum: "md5=d41d8cd98f00b204e9800998ecf8427e"
+}
+messages:
+["message" {some-filter}]
+post-messages:
+["post-message" {some-filter}]
+depexts:
+[["depext"] {some-filter}]
+dev-repo:
+"git+https://example.com"
+pin-depends:
+["dep.dev" "https://example.com"]
+maintainer:
+"maintainer"
+authors:
+["author1" "author2"]
+license:
+"LicenseRef-custom"
+tags:
+["tag"]
+homepage:
+"https://example.com"
+doc:
+"https://example.com"
+bug-reports:
+"https://example.com"
+x-some-extension:
+[ext "something"]
+x-env-path-rewrite:
+[[ENV false]]
+url
+{
+src: "https://example.com"
+checksum: "md5=d41d8cd98f00b204e9800998ecf8427e"
+}
+synopsis:
+"some synopsis"
+description:
+"some description"
+### cat OPAM/eff-eq/.opam-switch/packages/test-all-fields.1/opam
+opam-version: "2.0"
+synopsis: "some synopsis"
+description: "some description"
+maintainer: "maintainer"
+authors: ["author1" "author2"]
+license: "LicenseRef-custom"
+tags: "tag"
+homepage: "https://example.com"
+doc: "https://example.com"
+bug-reports: "https://example.com"
+depends: [
+  "dummy-dep" {some-filter}
+]
+depopts: [
+  "dummy-dep" {some-filter}
+]
+conflicts: [
+  "dummy-conflict" {some-filter}
+]
+conflict-class: "conflict-class"
+flags: plugin
+setenv: ENV = "some value"
+build: "false" {some-filter}
+run-test: "false" {some-filter}
+install: "false" {some-filter}
+remove: "false" {some-filter}
+substs: "some-file"
+patches: "some-patches" {some-filter}
+build-env: BUILD_ENV = "some value"
+messages: "message" {some-filter}
+post-messages: "post-message" {some-filter}
+depexts: ["depext"] {some-filter}
+dev-repo: "git+https://example.com"
+pin-depends: ["dep.dev" "https://example.com"]
+url {
+  src: "https://example.com"
+  checksum: "md5=d41d8cd98f00b204e9800998ecf8427e"
+}
+extra-source "some-file" {
+  src: "https://example.com"
+  checksum: "md5=d41d8cd98f00b204e9800998ecf8427e"
+}
+x-env-path-rewrite: [
+  [ENV false]
+]
+x-some-extension: [ext "something"]
+### opam upgrade
+Already up-to-date.
+Nothing to do.
+### rm OPAM/eff-eq/.opam-switch/packages/cache
+### opam upgrade --show
+Already up-to-date.
+Nothing to do.