Merge pull request #6219 from rjbou/lint-59

Fix lint W59 with a local path that is not an archive

master_changes.md

@@ -52,6 +52,7 @@ users)
 ## Source
 
 ## Lint
+  * [BUG] fix lint W59 with local urls that are not archives [#6219 @rjbou - fix #6218]
 
 ## Repository
 
@@ -116,6 +117,7 @@ users)
   * More exhaustive test for pin command: test different behaviour and cli options [#6135 @rjbou]
   * pin: add a test for erroneous first fetch done as local path on local VCS pinned packages [#6221 @rjbou]
   * Add cache test for installed packages cache update after an action failure [#6213 @kit-ty-kate @rjbou]
+  * Add more tests for lint W59 [#6219 @rjbou]
 
 ### Engine
   * Update print file function [#6233 @rjbou]
@@ -147,3 +149,4 @@ users)
 ## opam-core
   * `OpamStd.Sys.{get_terminal_columns,uname,getconf,guess_shell_compat}`: Harden the process calls to account for failures [#6230 @kit-ty-kate - fix #6215]
   * `OpamStd.Sys.{uname,getconf}`: now accepts only one argument as parameter, as per their documentation [#6230 @kit-ty-kate]
+  * `OpamSystem`: add `is_archive_from_string` that does the same than `is_archive` but without looking at the file, only analysing the string (extension) [#6219 @rjbou]

src/core/opamSystem.ml

@@ -869,19 +869,17 @@ module Tar = struct
   let match_ext file ext =
     List.exists (Filename.check_suffix file) ext
 
+  let get_archive_extension file =
+    OpamStd.List.find_map_opt (fun (ext, t) ->
+        if match_ext file ext then Some t else None)
+      extensions
+
   let get_type file =
-    let ext =
-      List.fold_left
-        (fun acc (ext, t) -> match acc with
-           | Some t -> Some t
-           | None   ->
-             if match_ext file ext
-             then Some t
-             else None)
-        None
-        extensions in
     if Sys.file_exists file then guess_type file
-    else ext
+    else get_archive_extension file
+
+  let is_archive_from_string f =
+    get_archive_extension f <> None
 
   let is_archive file =
     get_type file <> None
@@ -930,6 +928,11 @@ end
 
 module Zip = struct
 
+  let extension = "zip"
+
+  let is_archive_from_string file =
+    Filename.check_suffix file extension
+
   let is_archive f =
     if Sys.file_exists f then
       try
@@ -944,12 +947,16 @@ module Zip = struct
         | _ -> false
       with Sys_error _ | End_of_file -> false
     else
-      Filename.check_suffix f "zip"
+      is_archive_from_string f
 
   let extract_command file =
     Some (fun dir -> make_command "unzip" [ file; "-d"; dir ])
 end
 
+let is_archive_from_string file =
+  Tar.is_archive_from_string file
+  || Zip.is_archive_from_string file
+
 let is_archive file =
   Tar.is_archive file || Zip.is_archive file
 

src/core/opamSystem.mli

@@ -233,9 +233,14 @@ val read_command_output: ?verbose:bool -> ?env:string array ->
 
 (** END *)
 
-(** Test whether the file is an archive, by looking as its extension *)
+(** Test whether the file is an archive, by looking at its magic number
+    if the file exists, otherwise by looking as its extension *)
 val is_archive: string -> bool
 
+(** Test whether the given path is an archive, only by looking at its
+    extension *)
+val is_archive_from_string: string -> bool
+
 (** [extract ~dir:dirname filename] extracts the archive [filename] into
     [dirname]. [dirname] should not exists and [filename] should
     contain only one top-level directory.*)

src/state/opamFileTools.ml

@@ -396,9 +396,15 @@ let t_lint ?check_extra_files ?(check_upstream=false) ?(all=false) t =
         | #OpamUrl.version_control -> true
         | _ -> false)
   in
+  let url_archive =
+    let open OpamStd.Option.Op in
+    t.url >>| OpamFile.URL.url >>| (fun u ->
+        OpamSystem.is_archive_from_string u.OpamUrl.path)
+  in
   let is_url_archive =
-    not (OpamFile.OPAM.has_flag Pkgflag_Conf t) &&
-    url_vcs = Some false
+    not (OpamFile.OPAM.has_flag Pkgflag_Conf t)
+    && url_vcs = Some false
+    && url_archive = Some true
   in
   let check_upstream = check_upstream && is_url_archive in
   let check_double compare to_str lst =
@@ -1001,7 +1007,7 @@ let t_lint ?check_extra_files ?(check_upstream=false) ?(all=false) t =
               |> List.map OpamHash.to_string
               |> OpamStd.Format.pretty_list)])
           t.url)
-      (url_vcs = Some true
+      (url_vcs = Some true && url_archive = Some false
        && OpamStd.Option.Op.(t.url >>| fun u -> OpamFile.URL.checksum u <> [])
           = Some true);
     cond 68 `Warning

tests/reftests/lint.test

@@ -852,6 +852,7 @@ ${BASEDIR}/lint.opam: Warnings.
 ### opam lint ./lint.opam --check-upstream
 ${BASEDIR}/lint.opam: Warnings.
            warning 59: url doesn't contain a checksum
+### # package with conf flag
 ### <lint.opam>
 opam-version: "2.0"
 synopsis: "A word"
@@ -864,7 +865,6 @@ dev-repo: "hg+https://[email protected]"
 bug-reports: "https://nobug"
 url { src:"an-archive.tgz" }
 flags: conf
-### # package with conf flag
 ### opam lint ./lint.opam
 ${BASEDIR}/lint.opam: Errors.
              error 46: Package is flagged "conf" but has source, install or remove instructions
@@ -873,6 +873,7 @@ ${BASEDIR}/lint.opam: Errors.
 ${BASEDIR}/lint.opam: Errors.
              error 46: Package is flagged "conf" but has source, install or remove instructions
 # Return code 1 #
+### # package with git url
 ### <lint.opam>
 opam-version: "2.0"
 synopsis: "A word"
@@ -884,7 +885,53 @@ license: "ISC"
 dev-repo: "hg+https://[email protected]"
 bug-reports: "https://nobug"
 url { src:"git+https://a/repo" }
-### # package with git url
+### opam lint ./lint.opam
+${BASEDIR}/lint.opam: Passed.
+### opam lint ./lint.opam --check-upstream
+${BASEDIR}/lint.opam: Passed.
+### # package with local url
+### <repo/stuff>
+something
+### <lint.opam>
+opam-version: "2.0"
+synopsis: "A word"
+description: "Two words."
+authors: "the testing team"
+homepage: "egapemoh"
+maintainer: "[email protected]"
+license: "ISC"
+dev-repo: "hg+https://[email protected]"
+bug-reports: "https://nobug"
+### <add-url.sh>
+basedir=`echo "$BASEDIR" | sed "s/\\\\\\\\/\\\\\\\\\\\\\\\\/g"`
+cat << EOF >> lint.opam
+url { src:"file://$basedir/an-archive" }
+EOF
+### sh add-url.sh
+### opam lint ./lint.opam
+${BASEDIR}/lint.opam: Passed.
+### opam lint ./lint.opam --check-upstream
+${BASEDIR}/lint.opam: Passed.
+### # package with local archive url
+### <lint.opam>
+opam-version: "2.0"
+synopsis: "A word"
+description: "Two words."
+authors: "the testing team"
+homepage: "egapemoh"
+maintainer: "[email protected]"
+license: "ISC"
+dev-repo: "hg+https://[email protected]"
+bug-reports: "https://nobug"
+### <add-url.sh>
+basedir=`echo "$BASEDIR" | sed "s/\\\\\\\\/\\\\\\\\\\\\\\\\/g"`
+cat << EOF >> lint.opam
+url {
+  src:"file://$basedir/an-archive.tgz"
+  checksum: "md5=$(openssl md5 an-archive.tgz | cut -f2 -d' ')"
+  }
+EOF
+### sh add-url.sh
 ### opam lint ./lint.opam
 ${BASEDIR}/lint.opam: Passed.
 ### opam lint ./lint.opam --check-upstream

tests/reftests/pin.test

@@ -499,9 +499,6 @@ The following actions will be performed:
 -> installed nip-path.ved
 Done.
 ### opam pin edit nip-path
-[WARNING] The opam file didn't pass validation:
-           warning 59: url doesn't contain a checksum
-Proceed anyway ('no' will re-edit)? [y/n] y
 You can edit this file again with "opam pin edit nip-path", export it with "opam show nip-path --raw"
 Save the new opam file back to "${BASEDIR}/nip-path/nip-path.opam"? [y/n] y
 The following actions will be performed:
@@ -523,9 +520,6 @@ The following actions will be performed:
 # Return code 31 #
 ### opam pin add ./nip-path2 --edit
 Package nip-path2 does not exist, create as a NEW package? [y/n] y
-[WARNING] The opam file didn't pass validation:
-           warning 59: url doesn't contain a checksum
-Proceed anyway ('no' will re-edit)? [y/n] y
 You can edit this file again with "opam pin edit nip-path2", export it with "opam show nip-path2 --raw"
 nip-path2 is now pinned to file://${BASEDIR}/nip-path2 (version ved)