From 7857dd6bd21e3e02dedb1baeae50e77b72f90a77 Mon Sep 17 00:00:00 2001 From: Alejandro R Mosteo Date: Fri, 7 Aug 2020 14:56:46 +0200 Subject: [PATCH] Incrementally edit manifest on `alr with` (#484) * Append new dependencies to the end of the manifest * Conservative removal of dependencies in `alr with` This action will also not touch lines that aren't involved in the deletion. Only a simple dependency, and trivially empty [[depends-on]] array entries will be removed. As an improvement over the previous situation, dynamic expressions will not preclude using `alr with` for static dependencies, leaving the dynamic ones untouched. * Use lexicographical order for printing dependencies This was happening before by chance in several tests. The `alr with` changes have surfaced that the order on file is kept in tree; while this has no secondary effects, for presentation to the user is better to be always consistent. Solutions already use ordered sets, so for lists within conditional trees we do the same (only for presentation). * Testsuite minor output fix * Tests for new removal of dependencies --- src/alire/alire-conditional.ads | 4 +- src/alire/alire-conditional_trees-cases.adb | 7 +- src/alire/alire-conditional_trees.adb | 55 +++- src/alire/alire-conditional_trees.ads | 31 +- src/alire/alire-dependencies-containers.adb | 14 + src/alire/alire-dependencies-containers.ads | 12 +- src/alire/alire-dependencies-diffs.adb | 18 +- src/alire/alire-dependencies-diffs.ads | 10 +- src/alire/alire-dependencies.ads | 14 + src/alire/alire-directories.adb | 45 +++ src/alire/alire-directories.ads | 33 ++- src/alire/alire-manifest.adb | 265 ++++++++++++++++++ src/alire/alire-manifest.ads | 19 ++ src/alire/alire-releases.adb | 4 +- src/alire/alire-solutions.adb | 6 +- src/alire/alire-solver.adb | 6 +- src/alire/alire-solver.ads | 4 +- src/alire/alire-utils-text_files.adb | 66 +++++ src/alire/alire-utils-text_files.ads | 30 ++ src/alire/alire-workspace.adb | 2 +- src/alr/alr-commands-withing.adb | 39 ++- testsuite/tests/with/delete-two/test.py | 49 ++++ testsuite/tests/with/delete-two/test.yaml | 3 + .../tests/with/dynamic-dependencies/test.py | 57 ++++ .../tests/with/dynamic-dependencies/test.yaml | 3 + .../tests/workflows/init-with-pin/test.py | 3 +- 26 files changed, 745 insertions(+), 54 deletions(-) create mode 100644 src/alire/alire-dependencies-containers.adb create mode 100644 src/alire/alire-manifest.adb create mode 100644 src/alire/alire-manifest.ads create mode 100644 src/alire/alire-utils-text_files.adb create mode 100644 src/alire/alire-utils-text_files.ads create mode 100644 testsuite/tests/with/delete-two/test.py create mode 100644 testsuite/tests/with/delete-two/test.yaml create mode 100644 testsuite/tests/with/dynamic-dependencies/test.py create mode 100644 testsuite/tests/with/dynamic-dependencies/test.yaml diff --git a/src/alire/alire-conditional.ads b/src/alire/alire-conditional.ads index 66de8f84..b6f318c8 100644 --- a/src/alire/alire-conditional.ads +++ b/src/alire/alire-conditional.ads @@ -36,8 +36,8 @@ package Alire.Conditional with Preelaborate is -- Dependency on a version set function Enumerate is new Conditional.For_Dependencies.Enumerate - (Alire.Dependencies.Containers.Lists.List, - Alire.Dependencies.Containers.Lists.Append); + (Alire.Dependencies.Containers.List, + Alire.Dependencies.Containers.Append); function No_Dependencies return Dependencies is (For_Dependencies.Empty); diff --git a/src/alire/alire-conditional_trees-cases.adb b/src/alire/alire-conditional_trees-cases.adb index f82be8b6..6efc6b6e 100644 --- a/src/alire/alire-conditional_trees-cases.adb +++ b/src/alire/alire-conditional_trees-cases.adb @@ -45,7 +45,10 @@ package body Alire.Conditional_Trees.Cases is function Leaf_Count (This : Case_Node) return Positive; overriding - procedure Print (This : Case_Node; Prefix : String; Verbose : Boolean) + procedure Print (This : Case_Node; + Prefix : String; + Verbose : Boolean; + Sorted : Boolean) is use GNAT.IO; Tab : constant String := " "; @@ -59,7 +62,7 @@ package body Alire.Conditional_Trees.Cases is then This.Cases (I).Image_One_Line else "")); if Verbose then - Print (This.Cases (I), Prefix & Tab & Tab, Verbose); + Print (This.Cases (I), Prefix & Tab & Tab, Verbose, Sorted); end if; end if; end loop; diff --git a/src/alire/alire-conditional_trees.adb b/src/alire/alire-conditional_trees.adb index ef8ad3b7..f6c4c0a0 100644 --- a/src/alire/alire-conditional_trees.adb +++ b/src/alire/alire-conditional_trees.adb @@ -1,3 +1,5 @@ +with Ada.Containers.Indefinite_Ordered_Maps; + with GNAT.IO; package body Alire.Conditional_Trees is @@ -435,14 +437,23 @@ package body Alire.Conditional_Trees is ----------- overriding - procedure Print (This : Leaf_Node; Prefix : String; Verbose : Boolean) is - pragma Unreferenced (Verbose); + procedure Print (This : Leaf_Node; + Prefix : String; + Verbose : Boolean; + Sorted : Boolean := False) is + pragma Unreferenced (Verbose, Sorted); begin GNAT.IO.Put_Line (Prefix & Image (This.Value.Constant_Reference)); end Print; overriding - procedure Print (This : Vector_Node; Prefix : String; Verbose : Boolean) is + procedure Print (This : Vector_Node; + Prefix : String; + Verbose : Boolean; + Sorted : Boolean) + is + package Maps is new Ada.Containers.Indefinite_Ordered_Maps (String, + Node'Class); begin if Verbose then case This.Conjunction is @@ -451,22 +462,41 @@ package body Alire.Conditional_Trees is end case; end if; - for Child of This.Values loop - Print (Child, Prefix & (if Verbose then Tab else ""), Verbose); - end loop; + if Sorted then + declare + Map : Maps.Map; + begin + for Child of This.Values loop + Map.Insert (Child.Image, Child); + end loop; + for Child of Map loop + Print (Child, + Prefix & (if Verbose then Tab else ""), + Verbose, Sorted); + end loop; + end; + else + for Child of This.Values loop + Print (Child, + Prefix & (if Verbose then Tab else ""), + Verbose, Sorted); + end loop; + end if; end Print; overriding procedure Print (This : Conditional_Node; Prefix : String; - Verbose : Boolean) is + Verbose : Boolean; + Sorted : Boolean) + is use GNAT.IO; begin Put_Line (Prefix & "when " & This.Condition.Image & ":"); - Print (This.Then_Value.Root, Prefix & Tab, Verbose); + Print (This.Then_Value.Root, Prefix & Tab, Verbose, Sorted); if not This.Else_Value.Is_Empty then Put_Line (Prefix & "else:"); - Print (This.Else_Value.Root, Prefix & Tab, Verbose); + Print (This.Else_Value.Root, Prefix & Tab, Verbose, Sorted); end if; end Print; @@ -475,13 +505,14 @@ package body Alire.Conditional_Trees is ----------- procedure Print (This : Tree; - Prefix : String := ""; - And_Or : Boolean := True) is + Prefix : String := ""; + And_Or : Boolean := True; + Sorted : Boolean := False) is begin if This.Is_Empty then GNAT.IO.Put_Line (Prefix & "(empty)"); else - Print (This.Root, Prefix, And_Or); + Print (This.Root, Prefix, And_Or, Sorted); end if; end Print; diff --git a/src/alire/alire-conditional_trees.ads b/src/alire/alire-conditional_trees.ads index ba7f4d34..558b987f 100644 --- a/src/alire/alire-conditional_trees.ads +++ b/src/alire/alire-conditional_trees.ads @@ -49,9 +49,15 @@ package Alire.Conditional_Trees with Preelaborate is function Image (This : Node) return String is abstract; -- Single-line image for single-line tree image (used by Requisites). - procedure Print (This : Node; Prefix : String; Verbose : Boolean) + procedure Print (This : Node; + Prefix : String; + Verbose : Boolean; + Sorted : Boolean) is abstract; - -- Multi-line printing to stdout with tabulation (used by Props and Deps). + -- Multi-line printing to stdout with tabulation (used by Props and + -- Deps). Sorting affects only multi-value nodes (vectors, cases) and is + -- interesting for dependencies (to show them alphabetically) but not for + -- properties (some of them have order, like actions). function Leaf_Count (This : Node) return Positive is abstract; -- Return leaves under this node; for non-leaf nodes, obtain recursively. @@ -98,7 +104,8 @@ package Alire.Conditional_Trees with Preelaborate is with procedure Append (C : in out Collection; V : Values; Count : Count_Type := 1); - + -- This Append must honor "append" semantics (i.e., don't reorder); + -- otherwise actions, that have a user-defined order, would break. function Materialize (This : Tree; Against : Properties.Vector) return Collection with @@ -147,8 +154,9 @@ package Alire.Conditional_Trees with Preelaborate is -- Returns a Tree because it could result in an empty tree. procedure Print (This : Tree; - Prefix : String := ""; - And_Or : Boolean := True); + Prefix : String := ""; + And_Or : Boolean := True; + Sorted : Boolean := False); -- Use And_Or = false when only And can appear, in which case there is no -- need to distinguish and the output is slightly more compact. @@ -347,7 +355,10 @@ private function Leaf_Count (This : Leaf_Node) return Positive; overriding - procedure Print (This : Leaf_Node; Prefix : String; Verbose : Boolean); + procedure Print (This : Leaf_Node; + Prefix : String; + Verbose : Boolean; + Sorted : Boolean := False); overriding procedure To_TOML (This : Leaf_Node; Parent : TOML.TOML_Value); @@ -439,7 +450,10 @@ private overriding function Image (V : Vector_Node) return String; overriding - procedure Print (This : Vector_Node; Prefix : String; Verbose : Boolean); + procedure Print (This : Vector_Node; + Prefix : String; + Verbose : Boolean; + Sorted : Boolean); overriding procedure To_TOML (This : Vector_Node; Parent : TOML.TOML_Value); @@ -492,7 +506,8 @@ private overriding procedure Print (This : Conditional_Node; Prefix : String; - Verbose : Boolean); + Verbose : Boolean; + Sorted : Boolean); overriding procedure To_TOML (This : Conditional_Node; Parent : TOML.TOML_Value); diff --git a/src/alire/alire-dependencies-containers.adb b/src/alire/alire-dependencies-containers.adb new file mode 100644 index 00000000..afb90524 --- /dev/null +++ b/src/alire/alire-dependencies-containers.adb @@ -0,0 +1,14 @@ +package body Alire.Dependencies.Containers is + + function To_Set (This : List) return Sets.Set is + begin + return Result : Set do + for Dep of This loop + Result.Include (Dep); + -- We include instead of inserting because enumeration of case + -- expressions may give the same dependency more than once. + end loop; + end return; + end To_Set; + +end Alire.Dependencies.Containers; diff --git a/src/alire/alire-dependencies-containers.ads b/src/alire/alire-dependencies-containers.ads index 81576c7e..cde3f099 100644 --- a/src/alire/alire-dependencies-containers.ads +++ b/src/alire/alire-dependencies-containers.ads @@ -1,10 +1,20 @@ with Ada.Containers.Indefinite_Doubly_Linked_Lists; +with Ada.Containers.Indefinite_Ordered_Sets; package Alire.Dependencies.Containers with Preelaborate is package Lists is new Ada.Containers.Indefinite_Doubly_Linked_Lists (Dependency); - subtype List is Lists.List; + type List is new Lists.List with null record; + + package Sets is new + Ada.Containers.Indefinite_Ordered_Sets (Dependency, + Lexicographical_Sort); + + subtype Set is Sets.Set; + + function To_Set (This : List) return Sets.Set; + -- For presentation, we prefer dependencies to be shown in order end Alire.Dependencies.Containers; diff --git a/src/alire/alire-dependencies-diffs.adb b/src/alire/alire-dependencies-diffs.adb index 46a5e43e..aa625c1b 100644 --- a/src/alire/alire-dependencies-diffs.adb +++ b/src/alire/alire-dependencies-diffs.adb @@ -2,11 +2,18 @@ with Alire.Utils.Tables; package body Alire.Dependencies.Diffs is + ----------- + -- Added -- + ----------- + + function Added (This : Diff) return Containers.List + is (This.Added); + ------------- -- Between -- ------------- - function Between (Former, Latter : Containers.Lists.List) return Diff is + function Between (Former, Latter : Containers.List) return Diff is begin return This : Diff do @@ -51,7 +58,7 @@ package body Alire.Dependencies.Diffs is procedure Print (This : Diff) is Table : Utils.Tables.Table; - procedure Summarize (List : Containers.Lists.List; + procedure Summarize (List : Containers.List; Comment : String; Icon : String) is @@ -78,4 +85,11 @@ package body Alire.Dependencies.Diffs is end if; end Print; + ------------- + -- Removed -- + ------------- + + function Removed (This : Diff) return Containers.List + is (This.Removed); + end Alire.Dependencies.Diffs; diff --git a/src/alire/alire-dependencies-diffs.ads b/src/alire/alire-dependencies-diffs.ads index 117af13e..731e171e 100644 --- a/src/alire/alire-dependencies-diffs.ads +++ b/src/alire/alire-dependencies-diffs.ads @@ -7,7 +7,11 @@ package Alire.Dependencies.Diffs is function Between (Former, Latter : Conditional.Dependencies) return Diff; - function Between (Former, Latter : Containers.Lists.List) return Diff; + function Between (Former, Latter : Containers.List) return Diff; + + function Added (This : Diff) return Containers.List; + + function Removed (This : Diff) return Containers.List; function Contains_Changes (This : Diff) return Boolean; @@ -16,8 +20,8 @@ package Alire.Dependencies.Diffs is private type Diff is tagged record - Added : Containers.Lists.List; - Removed : Containers.Lists.List; + Added : Containers.List; + Removed : Containers.List; end record; end Alire.Dependencies.Diffs; diff --git a/src/alire/alire-dependencies.ads b/src/alire/alire-dependencies.ads index 5b406e77..3fcc02fd 100644 --- a/src/alire/alire-dependencies.ads +++ b/src/alire/alire-dependencies.ads @@ -39,6 +39,10 @@ package Alire.Dependencies with Preelaborate is function Image (Dep : Dependency) return String; -- Standard-style version image, e.g. "make^3.1" + function Manifest_Image (Dep : Dependency) return String; + -- Returns a line describing the dependency as it would appear in the + -- manifest, e.g.: my_crate = "^3.2.1" + overriding function TTY_Image (Dep : Dependency) return String; @@ -64,6 +68,9 @@ package Alire.Dependencies with Preelaborate is overriding function To_YAML (Dep : Dependency) return String; + function Lexicographical_Sort (L, R : Dependency) return Boolean; + -- By name and then version set image + private package TTY renames Utils.TTY; @@ -106,6 +113,9 @@ private function Image (Dep : Dependency) return String is ((+Dep.Crate) & Dep.Versions.Image); + function Manifest_Image (Dep : Dependency) return String is + ((+Dep.Crate) & " = " & '"' & Dep.Versions.Image & '"'); + overriding function TTY_Image (Dep : Dependency) return String is (TTY.Name (+Dep.Crate) & TTY.Version (Dep.Versions.Image)); @@ -118,4 +128,8 @@ private overriding function Key (Dep : Dependency) return String is (+Dep.Crate); + function Lexicographical_Sort (L, R : Dependency) return Boolean + is (L.Crate < R.Crate or else + (L.Crate = R.Crate and then L.Versions.Image < R.Versions.Image)); + end Alire.Dependencies; diff --git a/src/alire/alire-directories.adb b/src/alire/alire-directories.adb index 785fc306..78120a4f 100644 --- a/src/alire/alire-directories.adb +++ b/src/alire/alire-directories.adb @@ -346,4 +346,49 @@ package body Alire.Directories is Keep => <>, Name => +Name)); + -------------- + -- REPLACER -- + -------------- + + ------------------- + -- Editable_Name -- + ------------------- + + function Editable_Name (This : Replacer) return Any_Path + is (This.Temp_Copy.Filename); + + --------------------- + -- New_Replacement -- + --------------------- + + function New_Replacement (File : Any_Path; + Backup : Boolean := True) + return Replacer is + begin + return This : constant Replacer := (Length => File'Length, + Original => File, + Backup => Backup, + Temp_Copy => <>) + do + Ada.Directories.Copy_File (File, This.Temp_Copy.Filename); + end return; + end New_Replacement; + + ------------- + -- Replace -- + ------------- + + procedure Replace (This : in out Replacer) is + Backup : constant Any_Path := This.Original & ".prev"; + begin + -- Copy around, so never ceases to be a valid manifest in place + + if This.Backup then + Ada.Directories.Copy_File (This.Original, Backup); + end if; + Ada.Directories.Copy_File (This.Editable_Name, This.Original); + + -- The temporary copy will be cleaned up by This.Temp_Copy finalization + end Replace; + end Alire.Directories; diff --git a/src/alire/alire-directories.ads b/src/alire/alire-directories.ads index 2ca2d2d4..f8250d36 100644 --- a/src/alire/alire-directories.ads +++ b/src/alire/alire-directories.ads @@ -76,6 +76,8 @@ package Alire.Directories is -- Temporary files -- --------------------- + -- TEMP_FILE: obtain a temporary name with optional cleanup + type Temp_File is tagged limited private; -- A RAII scoped type to manage a temporary file name. -- Creates an instance with a unique file name. This does nothing on disk. @@ -94,6 +96,30 @@ package Alire.Directories is function With_Name (Name : String) return Temp_File; -- Allows initializing the tmp file with a desired name. + -- REPLACER: Modify a file "in place" in a safe way (keeping old copy) + + type Replacer (<>) is tagged limited private; + -- A scoped type to ensure that a file is updated and replaced without + -- trouble. In case of failure, the original file remains untouched. So + -- what happens is: 1) A copy to a temp file is made. 2) This file is + -- modified and can be tested as the client sees fit. 3) If the new file is + -- proper, the old one is renamed to .prev and the new one takes its place. + + function New_Replacement (File : Any_Path; + Backup : Boolean := True) + return Replacer; + -- Receives a file to be modified, and prepares a copy in a temporary. If + -- Backup, once the replacement is performed, the original file is kept as + -- ".prev". + + function Editable_Name (This : Replacer) return Any_Path; + -- Obtain the editable copy + + procedure Replace (This : in out Replacer); + -- Replace the original file with the edited copy. If this procedure is not + -- called, on going out of scope the Replacer will remove the temporary and + -- the original file remains untouched. + private ------------ @@ -122,8 +148,13 @@ private overriding procedure Initialize (This : in out Temp_File); - overriding procedure Finalize (This : in out Temp_File); + type Replacer (Length : Positive) is tagged limited record + Original : Any_Path (1 .. Length); + Temp_Copy : Temp_File; + Backup : Boolean := True; + end record; + end Alire.Directories; diff --git a/src/alire/alire-manifest.adb b/src/alire/alire-manifest.adb new file mode 100644 index 00000000..50817a07 --- /dev/null +++ b/src/alire/alire-manifest.adb @@ -0,0 +1,265 @@ +with Ada.Text_IO; use Ada.Text_IO; + +with Alire.Directories; +with Alire.Errors; +with Alire.Releases; +with Alire.TOML_Keys; +with Alire.Utils.Text_Files; + +package body Alire.Manifest is + + Warning : constant String := " # This line was added by `alr with`"; + + ------------ + -- Append -- + ------------ + + procedure Append (Name : Any_Path; + Deps : Dependencies.Containers.List) is + Replacer : constant Directories.Replacer := + Directories.New_Replacement (Name); + File : File_Type; + begin + if Deps.Is_Empty then + return; + end if; + + Open (File, Append_File, Replacer.Editable_Name); + + for Dep of Deps loop + New_Line (File); + Put_Line (File, "[[" & TOML_Keys.Depends_On & "]]" & Warning); + Put_Line (File, Dep.Manifest_Image & Warning); + end loop; + + Close (File); + + -- Attempt loading of the new file as a double check + if not Is_Valid (Replacer.Editable_Name) then + raise Program_Error + with Errors.Set ("Addition of dependencies to manifest failed"); + end if; + + Replacer.Replace; -- All went well, keep the changes + exception + when E : others => + Trace.Debug ("Exception attempting to append dependencies:"); + Alire.Log_Exception (E); + + if Is_Open (File) then + Close (File); + end if; + + raise; -- Let it be processed upwards, if necessary + end Append; + + -------------- + -- Is_Valid -- + -------------- + + function Is_Valid (Name : Any_Path) return Boolean is + begin + -- Check we are able to load the manifest file + if Releases.From_Manifest (Name).Version_Image /= "" then + Trace.Debug ("Checked valid manifest at " & Name); + return True; + else + raise Program_Error with "A release will always have a version"; + return False; + end if; + exception + when E : others => + Trace.Debug ("Exception trying to load manifest:"); + Log_Exception (E); + return False; + end Is_Valid; + + ------------ + -- Remove -- + ------------ + + procedure Remove (Name : Any_Path; + Deps : Dependencies.Containers.List) + is + + ------------ + -- Remove -- + ------------ + + procedure Remove (Dep : Dependencies.Dependency; + Lines : in out Utils.String_Vector) + -- Remove given Dep from Lines, or warn if impossible + is + Enter_Marker : constant String := "[[" & TOML_Keys.Depends_On & "]]"; + -- We must see a line like this before being able to remove a dep. + + Target : constant String := (+Dep.Crate) & "="""; + -- A line starting with Target is a candidate for deletion + + Armed : Boolean := False; + -- True when we are inside [[depends-on]] + + Found : Boolean := False; -- True when the dependency was found + + use Utils; + + ------------------- + -- Remove_Target -- + ------------------- + + procedure Remove_Target is + begin + for I in Lines.First_Index .. Lines.Last_Index loop + declare + Line : constant String := Replace (Lines (I), " ", ""); + begin + + if Armed and then Starts_With (Line, Target) then + -- Remove the target dependency + Trace.Debug ("Dependency to remove found at manifest line" + & I'Img); + Found := True; + Lines.Delete (I); + exit; + + elsif Starts_With (Line, "[[") then + -- Detect a plain [[depends-on]] with optional comment + Armed := + Line = Enter_Marker or else + Starts_With (Line, Enter_Marker & '#'); + + elsif Armed and then Line /= "" and then + Line (Line'First) /= '[' -- not a table or different array + then + -- We are seeing a different dependency in the same array + -- entry; we can still remove our target if later found + -- in this entry. This can happen if the user edited and + -- reused a previous [[depends-on]] added by `alr with`. + null; + + elsif Line = "" or else Starts_With (Line, "#") then + -- We still can remove a dependency found in this context + null; + + else + -- Any other sighting complicates matters and we won't + -- touch it. + Armed := False; + end if; + end; + end loop; + + if not Found then + Raise_Checked_Error + ("Could not find dependency in manifest: " & Dep.TTY_Image); + return; + end if; + end Remove_Target; + + ----------------------- + -- Remove_Empty_Deps -- + ----------------------- + + procedure Remove_Empty_Deps is + -- This might probably be done with multiline regular expressions + + Deletable : Natural := 0; + -- Tracks how many empty lines we have seen since the last [[ + + Can_Delete : Boolean := True; + -- We can delete as long as we are only seeing empty lines + begin + + -- Traverse lines backwards + + for I in reverse Lines.First_Index .. Lines.Last_Index loop + declare + Line : constant String := Replace (Lines (I), " ", ""); + begin + if Can_Delete then + -- Look for empty lines or the opening [[depends-on]] + if Line = "" then + Deletable := Deletable + 1; + + elsif + Line = Enter_Marker or else + Starts_With (Line, Enter_Marker & '#') + then + -- Now we can delete the empty [[depends-on]] plus any + -- following empty lines. + for J in 0 .. Deletable loop -- 0 for the current line + Trace.Debug ("Removing meaningless manifest line: " + & Lines (I)); + Lines.Delete (I); + end loop; + + -- Restart, we can still delete previous entries + Deletable := 0; + + else + -- We found something else, so do not remove entry + Can_Delete := False; + Deletable := 0; + end if; + + else + -- Look for a [[ that starts another array entry. We + -- cannot rely on simply [ for tables, these could be + -- nested array tables. + if Starts_With (Line, "[[") then + Can_Delete := True; + Deletable := 0; + -- We will look in next iterations for a precedent + -- empty array entry. + end if; + end if; + end; + end loop; + end Remove_Empty_Deps; + + begin + + -- First pass, remove a detected dependency in the proper location. + -- Note that this only removes the dependency line, but not the + -- enclosing [[depends-on]]. It is ok to have such an empty array + -- entry. Empty array entries are cleaned up afterwards. + + Remove_Target; + + -- Second pass, remove empty [[depends-on]] array entries. This + -- ensures that trivial add/remove of dependencies cannot grow + -- the file indefinitely with empty [[]] entries. + + Remove_Empty_Deps; + + end Remove; + + Replacer : constant Directories.Replacer := + Directories.New_Replacement (Name); + begin + if Deps.Is_Empty then + return; + end if; + + declare + File : constant Utils.Text_Files.File := + Utils.Text_Files.Load (Replacer.Editable_Name, + Backup => False); + -- Replacer takes care of backup + begin + for Dep of Deps loop + Remove (Dep, File.Lines.all); + end loop; + end; + + -- Attempt loading of the new file as a double check. This should never + -- fail because we won't touch anything that's clearly removable. + if not Is_Valid (Replacer.Editable_Name) then + raise Program_Error + with Errors.Set ("Removal of dependencies in manifest failed"); + end if; + + Replacer.Replace; -- All went well, keep the changes + end Remove; + +end Alire.Manifest; diff --git a/src/alire/alire-manifest.ads b/src/alire/alire-manifest.ads new file mode 100644 index 00000000..409adbea --- /dev/null +++ b/src/alire/alire-manifest.ads @@ -0,0 +1,19 @@ +with Alire.Dependencies.Containers; + +package Alire.Manifest is + + -- Subprograms for manipulation of the manifest file + + procedure Append (Name : Any_Path; + Deps : Dependencies.Containers.List); + + procedure Remove (Name : Any_Path; + Deps : Dependencies.Containers.List); + -- Do a best effort to remove dependencies; if we cannot locate a + -- dependency with enough guarantees of not botching the manifest, + -- no change will be applied and Checked_Error will be raised. + + function Is_Valid (Name : Any_Path) return Boolean; + -- Check that the given Name is a loadable manifest + +end Alire.Manifest; diff --git a/src/alire/alire-releases.adb b/src/alire/alire-releases.adb index 7925cfb3..6d19065d 100644 --- a/src/alire/alire-releases.adb +++ b/src/alire/alire-releases.adb @@ -515,7 +515,9 @@ package body Alire.Releases is -- DEPENDENCIES if not R.Dependencies.Is_Empty then Put_Line ("Dependencies (direct):"); - R.Dependencies.Print (" ", R.Dependencies.Contains_ORs); + R.Dependencies.Print (" ", + Sorted => True, + And_Or => R.Dependencies.Contains_ORs); end if; end Print; diff --git a/src/alire/alire-solutions.adb b/src/alire/alire-solutions.adb index 696af3f1..3f99157f 100644 --- a/src/alire/alire-solutions.adb +++ b/src/alire/alire-solutions.adb @@ -527,7 +527,7 @@ package body Alire.Solutions is (if TTY.Color_Enabled then "│ " else "| "); No_Branch : constant String := " "; - procedure Print (Deps : Dependencies.Containers.List; + procedure Print (Deps : Dependencies.Containers.Set; Prefix : String := ""; Omit : Boolean := False) -- Omit is used to remove the top-level connectors, for when the tree @@ -577,7 +577,7 @@ package body Alire.Solutions is if This.State (Dep.Crate).Has_Release then Print (Conditional.Enumerate - (This.State (Dep.Crate).Release.Dependencies), + (This.State (Dep.Crate).Release.Dependencies).To_Set, Prefix => Prefix -- Indent adding the proper running connector @@ -595,7 +595,7 @@ package body Alire.Solutions is if Print_Root then Trace.Always (Prefix & Root.Milestone.TTY_Image); end if; - Print (Conditional.Enumerate (Root.Dependencies), + Print (Conditional.Enumerate (Root.Dependencies).To_Set, Prefix, not Print_Root); end Print_Tree; diff --git a/src/alire/alire-solver.adb b/src/alire/alire-solver.adb index f679bcfa..10c115bf 100644 --- a/src/alire/alire-solver.adb +++ b/src/alire/alire-solver.adb @@ -98,7 +98,7 @@ package body Alire.Solver is -- Is_Resolvable -- ------------------- - function Is_Resolvable (Deps : Types.Platform_Dependencies; + function Is_Resolvable (Deps : Types.Abstract_Dependencies; Props : Properties.Vector; Current : Solution; Options : Query_Options := Default_Options) @@ -109,7 +109,7 @@ package body Alire.Solver is -- Resolve -- ------------- - function Resolve (Deps : Alire.Types.Platform_Dependencies; + function Resolve (Deps : Alire.Types.Abstract_Dependencies; Props : Properties.Vector; Current : Solution; Options : Query_Options := Default_Options) @@ -569,7 +569,7 @@ package body Alire.Solver is -- Otherwise expand the full dependencies Expand (Expanded => Empty, - Target => Full_Dependencies, + Target => Full_Dependencies.Evaluate (Props), Remaining => Empty, Solution => Solution); diff --git a/src/alire/alire-solver.ads b/src/alire/alire-solver.ads index 98ebeb3e..aa359028 100644 --- a/src/alire/alire-solver.ads +++ b/src/alire/alire-solver.ads @@ -90,7 +90,7 @@ package Alire.Solver is Default_Options : constant Query_Options := (others => <>); - function Resolve (Deps : Alire.Types.Platform_Dependencies; + function Resolve (Deps : Alire.Types.Abstract_Dependencies; Props : Properties.Vector; Current : Solution; Options : Query_Options := Default_Options) @@ -99,7 +99,7 @@ package Alire.Solver is -- given platform properties and lookup options. A current solution may -- be given and pinned releases will be reused. - function Is_Resolvable (Deps : Types.Platform_Dependencies; + function Is_Resolvable (Deps : Types.Abstract_Dependencies; Props : Properties.Vector; Current : Solution; Options : Query_Options := Default_Options) diff --git a/src/alire/alire-utils-text_files.adb b/src/alire/alire-utils-text_files.adb new file mode 100644 index 00000000..5b1e0728 --- /dev/null +++ b/src/alire/alire-utils-text_files.adb @@ -0,0 +1,66 @@ +with Ada.Text_IO; use Ada.Text_IO; + +with Alire.Directories; + +package body Alire.Utils.Text_Files is + + -------------- + -- Finalize -- + -------------- + + overriding + procedure Finalize (This : in out File) is + File : File_Type; + begin + if This.Lines = This.Orig then + Trace.Debug ("No changes to save in " & This.Name); + return; + end if; + + declare + Replacer : Directories.Replacer := + Directories.New_Replacement (This.Name, This.Backup); + begin + Open (File, Out_File, Replacer.Editable_Name); + for Line of This.Lines loop + Put_Line (File, Line); + end loop; + Close (File); + Replacer.Replace; + end; + end Finalize; + + ----------- + -- Lines -- + ----------- + + function Lines (This : aliased in out File) return access String_Vector + is (This.Lines'Access); + + ---------- + -- Load -- + ---------- + + function Load (From : Any_Path; + Backup : Boolean := True) + return File + is + F : File_Type; + begin + return This : File := (Ada.Finalization.Limited_Controlled with + Length => From'Length, + Name => From, + Backup => Backup, + others => <>) + do + Open (F, In_File, From); + while not End_Of_File (F) loop + This.Orig.Append (Get_Line (F)); + end loop; + Close (F); + + This.Lines := This.Orig; + end return; + end Load; + +end Alire.Utils.Text_Files; diff --git a/src/alire/alire-utils-text_files.ads b/src/alire/alire-utils-text_files.ads new file mode 100644 index 00000000..4c59f458 --- /dev/null +++ b/src/alire/alire-utils-text_files.ads @@ -0,0 +1,30 @@ +package Alire.Utils.Text_Files is + + -- A convenience type to hold a complete text file in memory as a vector of + -- lines. On destruction, changes to the contents are written back to disk. + -- A backup ".prev" file is also created by default. + + type File (<>) is tagged limited private; + + function Load (From : Any_Path; + Backup : Boolean := True) + return File; + -- Load a text file into memory. If Backup, when saving takes place the + -- original is renamed to ".prev". + + function Lines (This : aliased in out File) return access String_Vector; + +private + + type File (Length : Natural) is new Ada.Finalization.Limited_Controlled + with record + Name : Any_Path (1 .. Length); + Lines : aliased String_Vector; -- The final contents + Orig : String_Vector; -- The original contents + Backup : Boolean := True; + end record; + + overriding + procedure Finalize (This : in out File); + +end Alire.Utils.Text_Files; diff --git a/src/alire/alire-workspace.adb b/src/alire/alire-workspace.adb index 1425baab..357fed96 100644 --- a/src/alire/alire-workspace.adb +++ b/src/alire/alire-workspace.adb @@ -63,7 +63,7 @@ package body Alire.Workspace is declare To_Remove : Alire.Containers.Release_Set; function Enum (Deps : Conditional.Dependencies) - return Alire.Dependencies.Containers.Lists.List + return Alire.Dependencies.Containers.List renames Conditional.Enumerate; begin diff --git a/src/alr/alr-commands-withing.adb b/src/alr/alr-commands-withing.adb index 8245ebee..73aa42e2 100644 --- a/src/alr/alr-commands-withing.adb +++ b/src/alr/alr-commands-withing.adb @@ -6,6 +6,7 @@ with Ada.Text_IO; with Alire.Conditional; with Alire.Dependencies.Diffs; with Alire.Index; +with Alire.Manifest; with Alire.Milestones; with Alire.Releases; with Alire.Roots; @@ -178,22 +179,30 @@ package body Alr.Commands.Withing is return Filtered : Alire.Conditional.Dependencies do if Deps.Is_Iterable then for Dep of Deps loop - if Dep.Value.Crate /= Requested.Crate then - Filtered := Filtered and - Alire.Conditional.New_Dependency - (Dep.Value.Crate, Dep.Value.Versions); + if Dep.Is_Value and then Dep.Value.Crate /= Requested.Crate then + -- A regular static dependency + Filtered := Filtered and Dep; + elsif not Dep.Is_Value then + -- Something else (dynamic expression) that we cannot manage + -- programmatically. + Filtered := Filtered and Dep; + Trace.Warning + ("Skipping unsupported conditional dependency: " + & Dep.Image_One_Line); else -- Simply don't add the one we want to remove Found := True; end if; end loop; else - Trace.Warning ("Skipping unsupported conditional dependency"); + Trace.Warning ("Skipping unsupported conditional dependency: " + & Deps.Image_One_Line); end if; if not Found then Trace.Warning - ("Crate slated for removal is not among direct dependencies: " + ("Crate slated for removal is not among" + & " direct static dependencies: " & (+Requested.Crate)); end if; end return; @@ -222,13 +231,16 @@ package body Alr.Commands.Withing is Alire.Solver.Resolve (New_Deps, Platform.Properties, Old_Solution); + + Deps_Diff : constant Alire.Dependencies.Diffs.Diff := + Alire.Dependencies.Diffs.Between (Old_Deps, New_Deps); begin -- Show changes to apply Trace.Info ("Requested changes:"); Trace.Info (""); - Alire.Dependencies.Diffs.Between (Old_Deps, New_Deps).Print; + Deps_Diff.Print; -- Show the effects on the solution @@ -240,11 +252,13 @@ package body Alr.Commands.Withing is return; end if; - -- Generate the new .toml file + -- Add changes to the manifest: - Alire.Workspace.Generate_Manifest (New_Root.Release, - New_Root); - Trace.Detail ("Regeneration finished, updating now"); + Alire.Manifest.Append (Root.Current.Crate_File, + Deps_Diff.Added); + Alire.Manifest.Remove (Root.Current.Crate_File, + Deps_Diff.Removed); + Trace.Detail ("Manifest updated, fetching dependencies now"); -- And apply changes (will also generate new lockfile) @@ -401,7 +415,8 @@ package body Alr.Commands.Withing is begin Put_Line ("Dependencies (direct):"); Root_Release.Dependencies.Print (" ", - Root_Release.Dependencies.Contains_ORs); + Root_Release.Dependencies.Contains_ORs, + Sorted => True); if Cmd.Solve then Requires_Full_Index; -- Load possible hints diff --git a/testsuite/tests/with/delete-two/test.py b/testsuite/tests/with/delete-two/test.py new file mode 100644 index 00000000..871954a5 --- /dev/null +++ b/testsuite/tests/with/delete-two/test.py @@ -0,0 +1,49 @@ +""" +Check that we can delete dependencies from an array with several entries +""" + +import os +import re + +from drivers.alr import run_alr +from drivers.asserts import assert_eq, assert_match +from drivers.helpers import content_of + +manifest = "alire/xxx.toml" + +run_alr('init', '--bin', 'xxx') +os.chdir('xxx') + +# Manually add two dependencies to the same array entry +with open(manifest, 'a') as file: + file.write('[[depends-on]]\n' + 'libhello = "*"\n' + 'hello = "*"') + +# Check removal in 1st-2nd order +run_alr('with', '--del', 'libhello') +assert 'libhello = "*"' not in content_of(manifest) # Dep gone +assert 'hello = "*"' in content_of(manifest) # Dep stays + +run_alr('with', '--del', 'hello') +assert 'hello = "*"' not in content_of(manifest) # No trace of deps anymore +assert '[[depends-on]]' not in content_of(manifest) # No trace of deps anymore + +# Same tests in reverse order of dependency + +with open(manifest, 'a') as file: + file.write('[[depends-on]]\n' + 'hello = "*"\n' + 'libhello = "*"') + +# Check removal in 2nd-1st order +run_alr('with', '--del', 'libhello') +assert 'libhello = "*"' not in content_of(manifest) # Dep gone +assert 'hello = "*"' in content_of(manifest) # Dep stays + +run_alr('with', '--del', 'hello') +assert 'hello = "*"' not in content_of(manifest) # No trace of deps anymore +assert '[[depends-on]]' not in content_of(manifest) # No trace of deps anymore + + +print('SUCCESS') diff --git a/testsuite/tests/with/delete-two/test.yaml b/testsuite/tests/with/delete-two/test.yaml new file mode 100644 index 00000000..476e709f --- /dev/null +++ b/testsuite/tests/with/delete-two/test.yaml @@ -0,0 +1,3 @@ +driver: python-script +indexes: + solver_index: {} diff --git a/testsuite/tests/with/dynamic-dependencies/test.py b/testsuite/tests/with/dynamic-dependencies/test.py new file mode 100644 index 00000000..0c853e55 --- /dev/null +++ b/testsuite/tests/with/dynamic-dependencies/test.py @@ -0,0 +1,57 @@ +""" +Check that dynamic dependencies don't affect the use of `alr with` +""" + +import os +import re + +from drivers.alr import run_alr +from drivers.asserts import assert_eq, assert_match +from drivers.helpers import content_of + +manifest = "alire/xxx.toml" + +run_alr('init', '--bin', 'xxx') +os.chdir('xxx') + +# Manually add a regular and a dynamic dependency +with open(manifest, 'a') as file: + file.write('[[depends-on]]\n' + 'libhello = "*"\n\n' + '[[depends-on]]\n' + '[depends-on."case(os)"."..."]\n' + 'superhello = "*"') + +# Check adding a dependency +run_alr('with', 'hello') +assert 'hello = "*" # This line was added by `alr with`' \ + in content_of(manifest) + +# Check removal +run_alr('with', '--del', 'hello') +assert 'hello = "*" # This line was added by `alr with`' \ + not in content_of(manifest) + +# Check that the dependency that precedes the dynamic expression is removable +run_alr('with', '--del', 'libhello') +assert 'libhello = "*"' not in content_of(manifest) + +# Check that empty array entries have been cleaned up +assert content_of(manifest).count('[[depends-on]]') == 1 + +# Check that removing the dynamic dependency isn't allowed +p = run_alr('with', '--del', 'superhello', + complain_on_error=False, quiet=False) + +assert_match(".*" + + re.escape("Skipping unsupported conditional dependency: " + "(case OS is LINUX => (superhello*), " + "MACOS => (superhello*), WINDOWS => (superhello*), " + "OS_UNKNOWN => (superhello*))") + + ".*" + + re.escape("Crate slated for removal is not among" + " direct static dependencies: superhello") + + ".*", + p.out, flags=re.S) + +print('SUCCESS') diff --git a/testsuite/tests/with/dynamic-dependencies/test.yaml b/testsuite/tests/with/dynamic-dependencies/test.yaml new file mode 100644 index 00000000..476e709f --- /dev/null +++ b/testsuite/tests/with/dynamic-dependencies/test.yaml @@ -0,0 +1,3 @@ +driver: python-script +indexes: + solver_index: {} diff --git a/testsuite/tests/workflows/init-with-pin/test.py b/testsuite/tests/workflows/init-with-pin/test.py index 98d3c41a..51e5c529 100644 --- a/testsuite/tests/workflows/init-with-pin/test.py +++ b/testsuite/tests/workflows/init-with-pin/test.py @@ -16,7 +16,8 @@ os.chdir('xxx') # Make it depend on libhello session_file = os.path.join('alire', 'xxx.toml') run_alr('with', 'libhello') -check_line_in(session_file, 'libhello = "*"') +check_line_in(session_file, + 'libhello = "*" # This line was added by `alr with`') # Add the corresponding "with" line in xxx.gpr. # -- 2.39.5