Merge pull request #5564 from rjbou/undeclared-extra-files

Stop populating opam file with extra-files
ocaml · Sep 19, 2024 · 6d05a4e · 6d05a4e
2 parents b9e808b + 9a221be
commit 6d05a4e
Show file tree

Hide file tree

Showing 15 changed files with 426 additions and 139 deletions.
diff --git a/doc/pages/Manual.md b/doc/pages/Manual.md
@@ -52,7 +52,9 @@ following hierarchy:
   they override what may already be present in the `opam` file
 - [`/packages/<pkgname>/<pkgname>.<version>/files/`](#files): contains files
   that are copied over the root of the source tree of the given package before
-  it gets used.
+  it gets used. Before opam 2.3, all files in this directory were copied. 
+  Since opam 2.3 _only_ the files listed in the
+  [`extra-files`](#opamfield-extra-files) field are copied.
 - `/cache/`: cached package files, by checksum. Note that the cache location is
   configured in the [repo](#repofield-archive-mirrors) file, this name is only
   where `opam admin cache` puts it by default.
@@ -1216,8 +1218,10 @@ files.
   for path portability of environment variables on Windows.
 
 - <a id="opamfield-extra-files">`extra-files: [ [ <string> <checksum> ] ... ]`</a>:
-  optionally lists the files below `files/` with their checksums. Used
-  internally for integrity verifications.
+  lists the files below `files/` with their checksums. Used
+  internally for integrity verification. Before opam 2.3, listing files in
+  this field was optional, but since opam 2.3 all files must be listed in
+  this field or they will not be copied from the `files/` directory.
 
 - <a id="opamfield-pin-depends">`pin-depends: [ [ <package> <URL> ] ... ]`</a>:
   this field has no effect on the package repository, but is useful for
@@ -1294,7 +1298,10 @@ will be copied over the root of the package source. If already present, files
 are overwritten, and directories are recursively merged. [`opam`](#opam) file
 fields like [`patches:`](#opamfield-patches) refer to files at that same root,
 so patches specific to opam are typically included in
-this subdirectory.
+this subdirectory. Since opam 2.3, files must be listed in the
+[`extra-files:`](#opamfield-extra-files) or they are ignored. Before
+opam 2.3, all files were copied regardless of whether they appeared
+in the `extra-files` list.
 
 Also see the [`extra-sources:`](#opamsection-extra-sources) opam section, which has
 a similar behaviour and is processed before the `files/` are copied.

diff --git a/master_changes.md b/master_changes.md
@@ -83,6 +83,7 @@ users)
 ## Repository
  * Mitigate curl/curl#13845 by falling back from --write-out to --fail if exit code 43 is returned by curl [#6168 @dra27 - fix #6120]
   * Silently mark packages requiring an unsupported version of opam as unavailable [#5665 @kit-ty-kate - fix #5631]
+  * When loading a repository, don't automatically populate `extra-files:` field with found files in `files/` [#5564 @rjbou]
 
 ## Lock
 
@@ -223,6 +224,7 @@ users)
   * Fix some json output automatic replacement (duration and path on Windows) [#6184 @rjbou]
   * Add test for reftest syntax [#6184 @rjbou]
   * Add some readme file [#6184 @rjbou]
+  * Add new mechanism to add automatically files under `files/` directory to related opam file [#5564 @rjbou]
 
 ## Github Actions
   * Depexts: replace centos docker with almalinux to fake a centos [#6079 @rjbou]
@@ -264,6 +266,8 @@ users)
 ## opam-state
  * `OpamStateConfig.opamroot_with_provenance`: restore previous behaviour to `OpamStateConfig.opamroot` for compatibility with third party code [#6047 @dra27]
   * `OpamSwitchState.{,reverse_}dependencies`: make `unavailable` a non-optional argument to enforce speedups when availability information is not needed [#5317 @kit-ty-kate]
+  * `OpamFilteTools.add_aux_files`: ignore non registered extra-files and make the `files_subdir_hashes` argument optional (defaults to `false`) [#5564 @@rjbou]
+  * `OpamFileTools`: `read_opam` & `read_repo_opam` no more add non registered extra-files [#5564 @rjbou]
 
 ## opam-solver
  * `OpamCudfCriteria`, `OpamBuiltinZ3.Syntax`: Move `OpamBuiltinZ3.Syntax` into a dedicated module `OpamCudfCriteria` [#6130 @kit-ty-kate]

diff --git a/src/client/opamPinCommand.ml b/src/client/opamPinCommand.ml
@@ -36,10 +36,8 @@ let string_of_pinned ?(subpath_prefix=true) opam =
 let read_opam_file_for_pinning ?locked ?(quiet=false) name f url =
   let opam0 =
     let dir = OpamFilename.dirname (OpamFile.filename f) in
-    (* don't add aux files for [project/opam] *)
-    let add_files = OpamUrl.local_dir url = Some dir in
     let opam =
-      (OpamFormatUpgrade.opam_file_with_aux ~quiet ~dir ~files:add_files
+      (OpamFormatUpgrade.opam_file_with_aux ~quiet ~dir ~files:false
          ~filename:f) (OpamFile.OPAM.safe_read f)
     in
     if opam = OpamFile.OPAM.empty then None else Some opam

diff --git a/src/state/opamFileTools.ml b/src/state/opamFileTools.ml
@@ -1279,7 +1279,7 @@ let try_read rd f =
     let f = OpamFile.filename f in
     Some (OpamFilename.(Base.to_string (basename f)), bf)
 
-let add_aux_files ?dir ~files_subdir_hashes opam =
+let add_aux_files ?dir ?(files_subdir_hashes=false) opam =
   let dir = match dir with
     | None ->
       OpamFile.OPAM.get_metadata_dir ~repos_roots:(fun r ->
@@ -1330,7 +1330,6 @@ let add_aux_files ?dir ~files_subdir_hashes opam =
       | _, (None, None)  -> opam
     in
     let opam =
-      if not files_subdir_hashes then opam else
       let extra_files =
         OpamFilename.opt_dir files_dir >>| fun dir ->
         OpamFilename.rec_files dir
@@ -1341,20 +1340,27 @@ let add_aux_files ?dir ~files_subdir_hashes opam =
       match OpamFile.OPAM.extra_files opam, extra_files with
       | None, None -> opam
       | None, Some ef ->
-        log ~level:2
-          "Missing extra-files field for %a for %a, adding them."
-          (slog @@ OpamStd.List.concat_map ", "
-             (fun (_,f) -> OpamFilename.Base.to_string f)) ef
-          OpamStd.Op.(slog @@ OpamPackage.to_string @* OpamFile.OPAM.package)
-          opam;
-        let ef =
-          List.map
-            (fun (file, basename) ->
-               basename,
-               OpamHash.compute (OpamFilename.to_string file))
-            ef
+        let log ?level act =
+          log ?level
+            "Missing extra-files field for %a for %a, %s them."
+            (slog @@ OpamStd.List.concat_map ", "
+               (fun (_,f) -> OpamFilename.Base.to_string f)) ef
+            OpamStd.Op.(slog @@ OpamPackage.to_string @* OpamFile.OPAM.package)
+            opam act
         in
-        OpamFile.OPAM.with_extra_files ef opam
+        if files_subdir_hashes then
+          (log ~level:2 "adding";
+           let ef =
+             List.map
+               (fun (file, basename) ->
+                  basename,
+                  OpamHash.compute (OpamFilename.to_string file))
+               ef
+           in
+           OpamFile.OPAM.with_extra_files ef opam)
+        else
+          (log "ignoring";
+           opam)
       | Some ef, None ->
         log "Missing expected extra files %s at %s/files"
           (OpamStd.List.concat_map ", "
@@ -1401,7 +1407,7 @@ let read_opam dir =
     OpamFile.make (dir // "opam")
   in
   match try_read OpamFile.OPAM.read_opt opam_file with
-  | Some opam, None -> Some (add_aux_files ~dir ~files_subdir_hashes:true opam)
+  | Some opam, None -> Some (add_aux_files ~dir ~files_subdir_hashes:false opam)
   | _, Some err ->
     OpamConsole.warning
       "Could not read file %s. skipping:\n%s"

diff --git a/src/state/opamFileTools.mli b/src/state/opamFileTools.mli
@@ -83,8 +83,7 @@ val warns_to_json:
   ?filename:string -> (int * [`Warning|`Error] * string) list -> OpamJson.t
 
 (** Read the opam metadata from a given directory (opam file, with possible
-    overrides from url and descr files). Also includes the names and hashes
-    of files below files/
+    overrides from url and descr files).
     Warning: use [read_repo_opam] instead for correctly reading files from
     repositories!*)
 val read_opam: dirname -> OpamFile.OPAM.t option
@@ -100,7 +99,7 @@ val read_repo_opam:
     [files_subdir_hashes] is [true], also adds the names and hashes of files
     found below 'files/' *)
 val add_aux_files:
-  ?dir:dirname -> files_subdir_hashes:bool -> OpamFile.OPAM.t -> OpamFile.OPAM.t
+  ?dir:dirname -> ?files_subdir_hashes:bool -> OpamFile.OPAM.t -> OpamFile.OPAM.t
 
 (** {2 Tools to manipulate the [OpamFile.OPAM.t] contents} *)
 val map_all_variables:

diff --git a/src/state/opamUpdate.ml b/src/state/opamUpdate.ml
@@ -235,7 +235,7 @@ let pinned_package st ?version ?(autolock=false) ?(working_dir=false) name =
        from the repo *)
     let add_extra_files srcdir file opam =
       if OpamFilename.dirname (OpamFile.filename file) <> srcdir
-      then OpamFileTools.add_aux_files ~files_subdir_hashes:true opam
+      then OpamFileTools.add_aux_files ~files_subdir_hashes:false opam
       else opam
     in
     let locked = if autolock then OpamFile.OPAM.locked opam else None in

diff --git a/tests/reftests/admin.test b/tests/reftests/admin.test
@@ -673,7 +673,7 @@ opam-version: "2.0"
 ### <packages/no-extra/no-extra.1/files/missing-file>
 nothing here
 ### OPAMDEBUGSECTIONS="opam-file" OPAMDEBUG=-2 opam admin list
-opam-file                       Missing extra-files field for missing-file for no-extra.1, adding them.
+opam-file                       Missing extra-files field for missing-file for no-extra.1, ignoring them.
 # Packages matching: available
 # Name   # Installed # Synopsis
 no-extra --

diff --git a/tests/reftests/extrafile.test b/tests/reftests/extrafile.test
@@ -155,9 +155,9 @@ Now run 'opam upgrade' to apply any package updates.
 ### :::::::::::::::::
 ### sh -c "rm OPAM/repo/state-*.cache"
 ### OPAMDEBUGSECTIONS="opam-file" OPAMDEBUG=-2 opam list good-md5 -s | unordered
+opam-file                       Missing extra-files field for p.patch for no-checksum.1, ignoring them.
+opam-file                       Missing extra-files field for p.patch for not-mentioned.1, ignoring them.
 opam-file                       Missing expected extra files ../../../no-checksum/no-checksum.1/files/p.patch at ${BASEDIR}/OPAM/repo/default/packages/escape-good-md5/escape-good-md5.1/files
-opam-file                       Missing extra-files field for p.patch for no-checksum.1, adding them.
-opam-file                       Missing extra-files field for p.patch for not-mentioned.1, adding them.
 opam-file                       Missing expected extra files p.patch at ${BASEDIR}/OPAM/repo/default/packages/not-present/not-present.1/files
 opam-file                       Mismatching extra-files at ${BASEDIR}/OPAM/repo/default/packages/good-md5-good-sha256/good-md5-good-sha256.1: missing from 'files' directory (1)
 opam-file                       Missing expected extra files /etc/passwdd at ${BASEDIR}/OPAM/repo/default/packages/escape-absolute/escape-absolute.1/files
@@ -355,20 +355,21 @@ The following actions will be performed:
   - install no-checksum 1
 
 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> installed no-checksum.1
-Done.
+[ERROR] The compilation of no-checksum.1 failed at "test -f p.patch".
+
+
+
+
+<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
++- The following actions failed
+| - build no-checksum 1
++- 
+- No changes have been performed
+# Return code 31 #
 ### opam remove no-checksum
-[ERROR] In the opam file for no-checksum.1:
-          - At ${BASEDIR}/OPAM/repo/default/packages/no-checksum/no-checksum.1/opam:11:2-11:13::
-            expected [file checksum]
-        'extra-files' has been ignored.
-The following actions will be performed:
-=== remove 1 package
-  - remove no-checksum 1
+[NOTE] no-checksum is not installed.
 
-<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> removed   no-checksum.1
-Done.
+Nothing to do.
 ### opam install no-checksum --require-checksums
 [ERROR] In the opam file for no-checksum.1:
           - At ${BASEDIR}/OPAM/repo/default/packages/no-checksum/no-checksum.1/opam:11:2-11:13::
@@ -379,11 +380,21 @@ The following actions will be performed:
   - install no-checksum 1
 
 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> installed no-checksum.1
-Done.
+[ERROR] The compilation of no-checksum.1 failed at "test -f p.patch".
+
+
+
+
+<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
++- The following actions failed
+| - build no-checksum 1
++- 
+- No changes have been performed
+# Return code 31 #
 ### opam source no-checksum
 Successfully extracted to ${BASEDIR}/no-checksum.1
 ### test -f no-checksum.1/p.patch
+# Return code 1 #
 ### opam clean --download-cache
 Clearing cache of downloaded files
 ### :::::::::::::::::
@@ -400,27 +411,42 @@ The following actions will be performed:
   - install not-mentioned 1
 
 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> installed not-mentioned.1
-Done.
+[ERROR] The compilation of not-mentioned.1 failed at "test -f p.patch".
+
+
+
+
+<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
++- The following actions failed
+| - build not-mentioned 1
++- 
+- No changes have been performed
+# Return code 31 #
 ### opam remove not-mentioned
-The following actions will be performed:
-=== remove 1 package
-  - remove not-mentioned 1
+[NOTE] not-mentioned is not installed.
 
-<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> removed   not-mentioned.1
-Done.
+Nothing to do.
 ### opam install not-mentioned --require-checksums
 The following actions will be performed:
 === install 1 package
   - install not-mentioned 1
 
 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
--> installed not-mentioned.1
-Done.
+[ERROR] The compilation of not-mentioned.1 failed at "test -f p.patch".
+
+
+
+
+<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
++- The following actions failed
+| - build not-mentioned 1
++- 
+- No changes have been performed
+# Return code 31 #
 ### opam source not-mentioned
 Successfully extracted to ${BASEDIR}/not-mentioned.1
 ### test -f not-mentioned.1/p.patch
+# Return code 1 #
 ### opam clean --download-cache
 Clearing cache of downloaded files
 ### :II:2: not present

diff --git a/tests/reftests/legacy-git.test b/tests/reftests/legacy-git.test
@@ -652,6 +652,21 @@ Testing optional dependencies
 ### cp packages/P5.opam       REPO/packages/P5.1/opam
 ### cp packages/P5/README     REPO/packages/P5.1/descr
 ### sh mkurl.sh P5.1 P5.tar.gz
+### <hash.sh>
+set -ue
+for nv in REPO/packages/*; do
+  nv=`echo "$nv" | cut -f3 -d/`
+  n=`echo "$nv" | cut -f1 -d.`
+  path=REPO/packages/$nv
+  if [ -d "$path/files" ]; then
+    echo "extra-files:[" >> "$path/opam"
+    for file in `ls "$path/files"`; do
+      echo "  [\"$file\" \"md5=`openssl md5 "$path/files/$file" | cut -f2 -d' '`\"]" >> "$path/opam"
+    done
+    echo   "]" >> "$path/opam"
+  fi
+done
+### sh hash.sh
 ### git -C REPO/packages/ocaml.system add *
 ### git -C REPO/packages/ocaml.system commit -qm "Adding ocaml.system"
 ### git -C REPO/packages/ocaml.20 add *

diff --git a/tests/reftests/legacy-local.test b/tests/reftests/legacy-local.test
@@ -640,6 +640,21 @@ Testing optional dependencies
 ### cp packages/P5.opam       REPO/packages/P5.1/opam
 ### cp packages/P5/README     REPO/packages/P5.1/descr
 ### sh mkurl.sh P5.1 P5.tar.gz
+### <hash.sh>
+set -ue
+for nv in REPO/packages/*; do
+  nv=`echo "$nv" | cut -f3 -d/`
+  n=`echo "$nv" | cut -f1 -d.`
+  path=REPO/packages/$nv
+  if [ -d "$path/files" ]; then
+    echo "extra-files:[" >> "$path/opam"
+    for file in `ls "$path/files"`; do
+      echo "  [\"$file\" \"md5=`openssl md5 "$path/files/$file" | cut -f2 -d' '`\"]" >> "$path/opam"
+    done
+    echo   "]" >> "$path/opam"
+  fi
+done
+### sh hash.sh
 ### <REPO/repo>
 archive-mirrors: "cache"
 ### opam update

diff --git a/tests/reftests/lint.test b/tests/reftests/lint.test
@@ -674,7 +674,7 @@ license: "ISC"
 dev-repo: "hg+https://[email protected]"
 bug-reports: "https://nobug"
 extra-files: [ "more-file-bad-md5" "md5=00000000000000000000000000000000" ]
-### <pkg:lint.1:more-file-bad-md5>
+### <REPO/packages/lint/lint.1/more-file-bad-md5>
 and there is content!
 ### <pkg:lint.2>
 opam-version: "2.0"
@@ -688,14 +688,6 @@ dev-repo: "hg+https://[email protected]"
 bug-reports: "https://nobug"
 ### <pkg:lint.2:more-file-good-md5>
 and there is content!
-### <add-hash.sh>
-hsh=`openssl md5 REPO/packages/lint/lint.2/files/more-file-good-md5 | cut -d ' ' -f 2`
-echo "extra-files: [ \"more-file-good-md5\" \"md5=$hsh\" ]" >> REPO/packages/lint/lint.2/opam
-### sh add-hash.sh
-### opam update
-
-<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><>
-[default] synchronised from file://${BASEDIR}/REPO
 ### opam lint --package lint.1
 <default>/lint.1: Errors.
              error 53: Mismatching 'extra-files:' field: "more-file-bad-md5"
@@ -1075,11 +1067,11 @@ maintainer: "[email protected]"
 license: "ISC"
 dev-repo: "hg+https://[email protected]"
 bug-reports: "https://nobug"
-### <pkg:lint.3:single-file>
+### <REPO/packages/lint/lint.3/files/single-file>
 file
-### <pkg:lint.3:double-file>
+### <REPO/packages/lint/lint.3/files/double-file>
 file
-### <pkg:lint.3:quadra-file>
+### <REPO/packages/lint/lint.3/files/quadra-file>
 file
 ### <hash.sh>
 set -ue