Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail when trying to pin a package whose definition could not be found instead of forcing interactive edition #6319

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fail when trying to pin a package whose definition could not be found instead of forcing interactive edition #6319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed4192a
Add tests showing behaviour of opam pin when confronted with a missin…
kit-ty-kate Nov 30, 2024
5e0a66d
Fail when trying to pin a package whose definition could not be found…
kit-ty-kate Nov 30, 2024
47a607e
fixup
kit-ty-kate Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ users)

## Pin
* [NEW] Make it so pin list display the current revision of a pinned repository in a new column [#6274 @desumn - fix #5533]
* [BUG] Fail when trying to pin a package whose definition could not be found instead of forcing interactive edition (e.g. this could happen when making a typo in the package name of a pin-depends) [#6319 @kit-ty-kate - fix ocaml/setup-ocaml#902]

## List

Expand Down
42 changes: 19 additions & 23 deletions src/client/opamPinCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,32 +571,28 @@ and source_pin
| _ -> opam_opt
in

if not need_edit && opam_opt = None then
OpamConsole.note
"No package definition found for %s: please complete the template"
(OpamConsole.colorise `bold (OpamPackage.to_string nv));

let need_edit = need_edit || opam_opt = None in

let opam_opt =
let opam_base = match opam_opt with
| None -> OpamFileTools.template nv
| Some opam -> opam
in
let opam_base =
OpamFile.OPAM.with_url_opt urlf opam_base
in
if need_edit then
(if not (OpamFile.exists temp_file) then
OpamFile.OPAM.write_with_preserved_format
?format_from:(OpamPinned.orig_opam_file st name opam_base)
temp_file opam_base;
edit_raw name temp_file >>|
(* Preserve metadata_dir so that copy_files below works *)
OpamFile.OPAM.(with_metadata_dir (metadata_dir opam_base))
)
let opam_base = match opam_opt with
| None ->
OpamConsole.note
"No package definition found for %s: please complete the template"
(OpamConsole.colorise `bold (OpamPackage.to_string nv));
OpamFileTools.template nv
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really want to keep this template around? In which situation is it useful? I feel like it doesn't really has any purpose and we might want to remove it entirely (probably in a separate PR)

| Some opam -> opam
in
let opam_base =
OpamFile.OPAM.with_url_opt urlf opam_base
in
if not (OpamFile.exists temp_file) then
OpamFile.OPAM.write_with_preserved_format
?format_from:(OpamPinned.orig_opam_file st name opam_base)
temp_file opam_base;
edit_raw name temp_file >>|
(* Preserve metadata_dir so that copy_files below works *)
OpamFile.OPAM.(with_metadata_dir (metadata_dir opam_base))
else
Some opam_base
Option.map (OpamFile.OPAM.with_url_opt urlf) opam_opt
in
match opam_opt with
| None ->
Expand Down
76 changes: 66 additions & 10 deletions tests/reftests/pin.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,17 +682,9 @@ Done.
nip-v is now pinned to file://${BASEDIR}/nip-v.1.tgz (version 2)
### opam pin add nip 1 --with-version 42 --no-action
[nip.42] synchronised (file://${BASEDIR}/nip.1.tgz)
[NOTE] No package definition found for nip.42: please complete the template
[WARNING] The opam file didn't pass validation:
error 22: Some fields are present but empty; remove or fill them: "homepage", "license", "bug_reports"
error 57: Synopsis must not be empty
warning 59: url doesn't contain a checksum
warning 62: License doesn't adhere to the SPDX standard, see https://spdx.org/licenses/ : ""
Proceed anyway ('no' will re-edit)? [Y/n] y
You can edit this file again with "opam pin edit nip", export it with "opam show nip --raw"
nip is now pinned to file://${BASEDIR}/nip.1.tgz (version 42)
[ERROR] No valid package definition found
# Return code 5 #
### opam pin list
nip.42 (uninstalled) rsync file://${BASEDIR}/nip.1.tgz
nip-path.42 rsync file://${BASEDIR}/nip-path
nip-v.2 (uninstalled) rsync file://${BASEDIR}/nip-v.1.tgz
### find OPAM/versions/lib -name '*.t' | sort
Expand Down Expand Up @@ -937,3 +929,67 @@ vcs-local is now pinned to git+file://${BASEDIR}/vcs-local#master (version dev)
### opam show vcs-local --field build:,url.src:
build: "false"
url.src: "git+file://${BASEDIR}/vcs-local#master"
### : behaviour of opam pin when confronted with a missing opam description
### OPAMEDITOR="echo iNvAlId "
### <pin-depends.sh>
#!/bin/sh
set -eu
basedir=$(echo "$BASEDIR" | sed 's/\\/\\\\\\\\/g')
cat << EOF >> "$1"
pin-depends: [
["$2" "file://$basedir/$3"]
]
EOF
### <pin:missing-opam/pkg.opam>
opam-version: "2.0"
### opam pin pkg-with-typo ./missing-opam --no-action
Package pkg-with-typo does not exist, create as a NEW package? [Y/n] y
[pkg-with-typo.dev] synchronised (file://${BASEDIR}/missing-opam)
[ERROR] No valid package definition found
# Return code 5 #
### opam pin pkg-with-typo ./missing-opam
Package pkg-with-typo does not exist, create as a NEW package? [Y/n] y
[pkg-with-typo.dev] synchronised (no changes)
[ERROR] No valid package definition found
# Return code 5 #
### <pin:missing-opam/pkg.opam>
opam-version: "2.0"
### sh ./pin-depends.sh missing-opam/pkg.opam pin-pkg.dev missing-opam
### opam pin ./missing-opam
Package pkg does not exist, create as a NEW package? [Y/n] y
The following additional pinnings are required by pkg.dev:
- pin-pkg.dev at file://${BASEDIR}/missing-opam
Pin and install them? [Y/n] y
Package pin-pkg does not exist, create as a NEW package? [Y/n] y
[pin-pkg.dev] synchronised (no changes)
[ERROR] No valid package definition found
# Return code 5 #
### <pin:missing-opam/pkg.opam>
opam-version: "2.0"
depends: [
"pin-pkg"
]
### sh ./pin-depends.sh missing-opam/pkg.opam pin-pkg.dev missing-opam
### opam pin ./missing-opam
Package pkg does not exist, create as a NEW package? [Y/n] y
The following additional pinnings are required by pkg.dev:
- pin-pkg.dev at file://${BASEDIR}/missing-opam
Pin and install them? [Y/n] y
Package pin-pkg does not exist, create as a NEW package? [Y/n] y
[pin-pkg.dev] synchronised (no changes)
[ERROR] No valid package definition found
# Return code 5 #
### : special case where the name of the pin-depend matches the name of the package itself (recursive)
### <pin:missing-opam/pkg.opam>
opam-version: "2.0"
### sh ./pin-depends.sh missing-opam/pkg.opam pkg.dev missing-opam/empty-dir
### mkdir missing-opam/empty-dir
### opam pin ./missing-opam
Package pkg does not exist, create as a NEW package? [Y/n] y
The following additional pinnings are required by pkg.dev:
- pkg.dev at file://${BASEDIR}/missing-opam/empty-dir
Pin and install them? [Y/n] y
Package pkg does not exist, create as a NEW package? [Y/n] y
[pkg.dev] synchronised (no changes)
[ERROR] No valid package definition found
# Return code 5 #