From 08e1e9ec15beb39543e183b03c0d3f7f31c7f921 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Fri, 11 Oct 2024 17:37:26 +0200 Subject: [PATCH 1/3] system: add a function to detect if a file is an archive only by analising the extension --- master_changes.md | 1 + src/core/opamSystem.ml | 31 +++++++++++++++++++------------ src/core/opamSystem.mli | 7 ++++++- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/master_changes.md b/master_changes.md index 70fdef98758..f35b88579d5 100644 --- a/master_changes.md +++ b/master_changes.md @@ -147,3 +147,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] diff --git a/src/core/opamSystem.ml b/src/core/opamSystem.ml index 74a46471512..75dbb588743 100644 --- a/src/core/opamSystem.ml +++ b/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 diff --git a/src/core/opamSystem.mli b/src/core/opamSystem.mli index f736089dff8..ca6e9da7488 100644 --- a/src/core/opamSystem.mli +++ b/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.*) From f0301f460f5bdcf3c99ba73b038da6e5fbff0d64 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Tue, 1 Oct 2024 11:39:58 +0200 Subject: [PATCH 2/3] reftest: add more test for lint w59 --- master_changes.md | 1 + tests/reftests/lint.test | 55 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/master_changes.md b/master_changes.md index f35b88579d5..3674ea7dde2 100644 --- a/master_changes.md +++ b/master_changes.md @@ -116,6 +116,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] diff --git a/tests/reftests/lint.test b/tests/reftests/lint.test index f9528eeecc2..942dad30a53 100644 --- a/tests/reftests/lint.test +++ b/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 ### opam-version: "2.0" synopsis: "A word" @@ -864,7 +865,6 @@ dev-repo: "hg+https://to@li.nt" 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 ### opam-version: "2.0" synopsis: "A word" @@ -884,7 +885,57 @@ license: "ISC" dev-repo: "hg+https://to@li.nt" 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 +### +something +### +opam-version: "2.0" +synopsis: "A word" +description: "Two words." +authors: "the testing team" +homepage: "egapemoh" +maintainer: "maint@tain.er" +license: "ISC" +dev-repo: "hg+https://to@li.nt" +bug-reports: "https://nobug" +### +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: Warnings. + warning 59: url doesn't contain a checksum +### opam lint ./lint.opam --check-upstream +${BASEDIR}/lint.opam: Errors. + warning 59: url doesn't contain a checksum + error 60: Upstream check failed: "Source not found: ${BASEDIR}/an-archive" +# Return code 1 # +### # package with local archive url +### +opam-version: "2.0" +synopsis: "A word" +description: "Two words." +authors: "the testing team" +homepage: "egapemoh" +maintainer: "maint@tain.er" +license: "ISC" +dev-repo: "hg+https://to@li.nt" +bug-reports: "https://nobug" +### +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 From 593fb2ec0ed65bcb983facc1d90fc3c4c667cfa7 Mon Sep 17 00:00:00 2001 From: Raja Boujbel Date: Tue, 1 Oct 2024 11:41:26 +0200 Subject: [PATCH 3/3] lint: fix W59 for local url (rsync) that are not archives --- master_changes.md | 1 + src/state/opamFileTools.ml | 12 +++++++++--- tests/reftests/lint.test | 8 ++------ tests/reftests/pin.test | 6 ------ 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/master_changes.md b/master_changes.md index 3674ea7dde2..024eadbeb77 100644 --- a/master_changes.md +++ b/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 diff --git a/src/state/opamFileTools.ml b/src/state/opamFileTools.ml index 804a54c6a17..c27ed93f5b7 100644 --- a/src/state/opamFileTools.ml +++ b/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 diff --git a/tests/reftests/lint.test b/tests/reftests/lint.test index 942dad30a53..d2bc8384ac4 100644 --- a/tests/reftests/lint.test +++ b/tests/reftests/lint.test @@ -909,13 +909,9 @@ url { src:"file://$basedir/an-archive" } EOF ### sh add-url.sh ### opam lint ./lint.opam -${BASEDIR}/lint.opam: Warnings. - warning 59: url doesn't contain a checksum +${BASEDIR}/lint.opam: Passed. ### opam lint ./lint.opam --check-upstream -${BASEDIR}/lint.opam: Errors. - warning 59: url doesn't contain a checksum - error 60: Upstream check failed: "Source not found: ${BASEDIR}/an-archive" -# Return code 1 # +${BASEDIR}/lint.opam: Passed. ### # package with local archive url ### opam-version: "2.0" diff --git a/tests/reftests/pin.test b/tests/reftests/pin.test index cdaecb87c81..09916bccde0 100644 --- a/tests/reftests/pin.test +++ b/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)